Skip to content

Fix SQLAlchemy query cache collisions for bound parameters - #8886

Open
gorkadabo1 wants to merge 1 commit into
huggingface:mainfrom
gorkadabo1:fix/sql-bound-parameter-cache-8877
Open

gorkadabo1 wants to merge 1 commit into
huggingface:mainfrom
gorkadabo1:fix/sql-bound-parameter-cache-8877

Conversation

@gorkadabo1

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #8877

Dataset.from_sql() can silently return incorrect cached rows when two SQLAlchemy queries differ only in their bound parameter values. The existing cache identity includes the compiled SQL placeholders but not their values, causing queries with different WHERE, LIMIT/OFFSET, or IN parameters to share a cache.

This change includes the compiled SQL and its effective bound parameters when calculating the configuration hash.

It also handles:

  • Explicit parameter overrides.
  • Callable parameters without evaluating them during hashing.
  • Binary memoryview values, including nested structures.

Raw SQL string behavior remains unchanged.

Validation

Added 24 regression tests for cache collisions, cache reuse with equivalent queries, parameter overrides, callable parameters, binary values, and compatibility.

  • Before the fix: 18 failures, 22 passes with the final SQL tests.
  • After the fix: 40 passed with SQLAlchemy 2.0.55 and 2.1.6.
  • SQLAlchemy 1.4.54: All 24 new cases passed. Two unrelated existing tests failed during Arrow/Dill serialization. They also fail without the fix.
  • Adjacent test suites: 475 passed, 3 skipped.
  • Full unit suite: 3,627 passed, 196 skipped.
  • make quality and git diff --check passed.

Compatibility

SQLAlchemy Selectable queries get new cache keys, so they won't reuse potentially incorrect existing caches. Previously saved incorrect datasets need to be regenerated. Cache reuse is intentionally skipped for callable parameters that cannot be resolved. Some statement-level callable overrides in SQLAlchemy 2.1 may also miss cache reuse, even when it would be safe.

This fix is independent of #8752, which modifies the same method for connection handling. Both changes were tested together successfully (47 SQL tests passed), although merging them may require resolving the overlapping code. Other SQL dialects were tested by compiling queries, but not against live databases.

@gorkadabo1
gorkadabo1 force-pushed the fix/sql-bound-parameter-cache-8877 branch from d6631a2 to 04ec1bd Compare October 23, 2026 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dataset.from_sql caches different SQLAlchemy queries under the same key when bound values change

1 participant