Fix global CompileState plugin_for breaking with_loader_criteria on non-Snowflake dialects#677
Conversation
…on-Snowflake dialects The BCR-1057 CompileState plugin overrides were globally replacing ORM SELECT compilation for all dialects. This caused with_loader_criteria to silently drop filters for any non-Snowflake dialect when the criteria targeted entities inside sealed subqueries with ORM joins. Two changes: 1. Add dialect detection to SnowflakeSelectState and SnowflakeORMSelectCompileState — delegate to super() for non-Snowflake dialects so standard SA behavior is preserved. 2. Replace stale copied __init__ methods in _Snowflake_ORMJoin and _Snowflake_Selectable_Join with thin wrappers that delegate to super().__init__() and only intercept NoForeignKeysError for the Lateral-without-onclause case. Fixes snowflakedb#676 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
| class SnowflakeSelectState(SelectState): | ||
| def __init__(self, statement, compiler, **kw): | ||
| self._is_snowflake = ( | ||
| compiler is not None and compiler.dialect.name == "snowflake" |
There was a problem hiding this comment.
There is const DIALECT_NAME
| compiler is not None and compiler.dialect.name == "snowflake" | |
| compiler is not None and compiler.dialect.name == DIALECT_NAME |
|
|
||
| def _init_global_attributes(self, statement, compiler, **kw): | ||
| self._is_snowflake = ( | ||
| compiler is not None and compiler.dialect.name == "snowflake" |
There was a problem hiding this comment.
There is const DIALECT_NAME
| compiler is not None and compiler.dialect.name == "snowflake" | |
| compiler is not None and compiler.dialect.name == DIALECT_NAME |
| except NoForeignKeysError: | ||
| # BCR-1057: Snowflake lateral joins don't require ON clause. | ||
| # Join.__init__ set self.left and self.right before | ||
| # _match_primaries raised. | ||
| if onclause is None and isinstance(self.right, Lateral): | ||
| self.onclause = None | ||
| self.isouter = isouter | ||
| self.full = full | ||
| else: | ||
| raise |
There was a problem hiding this comment.
Consider different approach — verified against the installed SA source where expression.Join.__init__ calls self._match_primaries() dynamically through the instance:
class _Snowflake_Selectable_Join(Join):
def _match_primaries(self, left, right):
try:
return super()._match_primaries(left, right)
except NoForeignKeysError:
if isinstance(right, Lateral):
return None # BCR-1057: lateral joins don't require FK relationships
raiseThis lets Join.__init__ run to completion cleanly — no manual attribute patching. And because _ORMJoin.__init__ calls expression.Join.__init__(self, ...), Python dispatches self._match_primaries() through the MRO at runtime. So _Snowflake_ORMJoin can use multiple inheritance and needs no __init__ override at all:
class _Snowflake_ORMJoin(_Snowflake_Selectable_Join, sa_orm_util_ORMJoin):
pass # _match_primaries from _Snowflake_Selectable_Join propagates through MROThis replaces ~80 lines of fragile patching with ~10 lines of clean OOP.
…ate-breaks-loader-criteria
…gins Snowflake's dialect needs to support BCR-1057 — a Snowflake backend change that forbids an ON clause on lateral joins. Standard SQLAlchemy always requires one. To bridge this, the dialect installed three pieces of machinery as global SQLAlchemy plugins: 1. SnowflakeSelectState / SnowflakeORMSelectCompileState — registered via CompileState.plugin_for(...), which replaces SQLAlchemy's compile-state classes for every dialect loaded in the process (not just Snowflake). 2. _Snowflake_Selectable_Join / _Snowflake_ORMJoin — subclasses that relaxed the "ON clause required" rule for lateral joins. The _Snowflake_ORMJoin implementation was a stale line-by-line copy of SQLAlchemy's internal _ORMJoin.__init__. Over time SQLAlchemy's original had evolved (most importantly, to propagate _extra_criteria from with_loader_criteria into subqueries) while the copy did not. What Needed To Change Two independent problems: 1. Cross-dialect leakage (the regression). Because the plugins are global, any app that merely imports snowflake-sqlalchemy silently runs Snowflake's compile logic against PostgreSQL/MySQL/SQLite too. Combined with the stale copy above, with_loader_criteria filters were dropped inside .subquery() constructs on all dialects. This is the bug snowflakedb#676 reports. 2. Divergence risk. Because _Snowflake_ORMJoin was a stale copy rather than a delegating subclass, every future SQLAlchemy bug fix or feature addition in _ORMJoin silently bypasses Snowflake users. Architecture of the Fix The fix has two layers: Layer 1 — Dialect gate. The Snowflake compile-state classes now detect whether the active compilation targets the Snowflake dialect. If not, they short-circuit to super() and let standard SQLAlchemy run untouched. Non-Snowflake dialects are no longer affected by the import at all. Layer 2 — Thin-wrapper joins. _Snowflake_Selectable_Join and _Snowflake_ORMJoin are rewritten from stale copies into minimal subclasses that delegate to SQLAlchemy's originals and intercept only the lateral-without-ON case. Future SQLAlchemy improvements to _ORMJoin now flow through automatically. Why BCR-1057 Still Matters BCR-1057 is a Snowflake server contract, not a client choice. The server rejects JOIN LATERAL ... ON <anything>. Standard SQLAlchemy unconditionally emits an ON clause (matching FK-based inference) for any join. Without the lateral-specific intercept, every ORM query using .lateral() against Snowflake would fail server-side — so the narrow intercept must remain even after the broader cleanup. Impact on SQLAlchemy 2.0 Full fix: both the regression (with_loader_criteria) and the divergence risk are resolved. Non-Snowflake dialects behave like stock SQLAlchemy. Snowflake behavior is preserved, now inheriting future SQLAlchemy fixes automatically. Impact on SQLAlchemy 1.4 The dialect gate's hook (_init_global_attributes) does not exist in SA 1.4, so the gate cannot activate there. SA 1.4 retains the pre-PR behavior — the Snowflake compile path runs unconditionally. This is acceptable because: - The reported regression (with_loader_criteria + DeclarativeBase) is an SA 2.0-only API surface — the class doesn't exist in 1.4, so the bug doesn't either. - The thin-wrapper refactor of the join classes still applies to 1.4, so SA 1.4 users also benefit from the divergence-risk cleanup. The regression test is marked SA 2.0-only for the same reason. No behavior change for existing SA 1.4 users.
sfc-gh-mraba
left a comment
There was a problem hiding this comment.
Snowflake's dialect needs to support BCR-1057 — a Snowflake backend change that forbids an ON clause on lateral joins. Standard SQLAlchemy always requires one. To bridge this, the dialect
installed three pieces of machinery as global SQLAlchemy plugins:
SnowflakeSelectState/SnowflakeORMSelectCompileState— registered viaCompileState.plugin_for(...), which replaces SQLAlchemy's compile-state classes for every dialect loaded in
the process (not just Snowflake)._Snowflake_Selectable_Join/_Snowflake_ORMJoin— subclasses that relaxed the "ON clause required" rule for lateral joins.
The _Snowflake_ORMJoin implementation was a stale line-by-line copy of SQLAlchemy's internal _ORMJoin.__init__. Over time SQLAlchemy's original had evolved (most importantly, to propagate
_extra_criteria from with_loader_criteria into subqueries) while the copy did not.
What Needed To Change
Two independent problems:
- Cross-dialect leakage (the regression). Because the plugins are global, any app that merely imports snowflake-sqlalchemy silently runs Snowflake's compile logic against
PostgreSQL/MySQL/SQLite too. Combined with the stale copy above,with_loader_criteriafilters were dropped inside.subquery()constructs on all dialects. This is the bug #676 reports. - Divergence risk. Because
_Snowflake_ORMJoinwas a stale copy rather than a delegating subclass, every future SQLAlchemy bug fix or feature addition in_ORMJoinsilently bypasses
Snowflake users.
Architecture of the Fix
The fix has two layers:
Layer 1 — Dialect gate. The Snowflake compile-state classes now detect whether the active compilation targets the Snowflake dialect. If not, they short-circuit to super() and let standard
SQLAlchemy run untouched. Non-Snowflake dialects are no longer affected by the import at all.
Layer 2 — Thin-wrapper joins. _Snowflake_Selectable_Join and _Snowflake_ORMJoin are rewritten from stale copies into minimal subclasses that delegate to SQLAlchemy's originals and intercept
only the lateral-without-ON case. Future SQLAlchemy improvements to _ORMJoin now flow through automatically.
Why BCR-1057 Still Matters
BCR-1057 is a Snowflake server contract, not a client choice. The server rejects JOIN LATERAL ... ON <anything>. Standard SQLAlchemy unconditionally emits an ON clause (matching FK-based
inference) for any join. Without the lateral-specific intercept, every ORM query using .lateral() against Snowflake would fail server-side — so the narrow intercept must remain even after the
broader cleanup.
Impact on SQLAlchemy 2.0
Full fix: both the regression (with_loader_criteria) and the divergence risk are resolved. Non-Snowflake dialects behave like stock SQLAlchemy. Snowflake behavior is preserved, now inheriting
future SQLAlchemy fixes automatically.
Impact on SQLAlchemy 1.4
The dialect gate's hook (_init_global_attributes) does not exist in SA 1.4, so the gate cannot activate there. SA 1.4 retains the pre-PR behavior — the Snowflake compile path runs
unconditionally. This is acceptable because:
- The reported regression (
with_loader_criteria+DeclarativeBase) is an SA 2.0-only API surface — the class doesn't exist in 1.4, so the bug doesn't either. - The thin-wrapper refactor of the join classes still applies to 1.4, so SA 1.4 users also benefit from the divergence-risk cleanup.
The regression test is marked SA 2.0-only for the same reason. No behavior change for existing SA 1.4 users.
…ate-breaks-loader-criteria
…ate-breaks-loader-criteria
Summary
Fixes #676
The BCR-1057
CompileState.plugin_foroverrides globally replace ORM SELECT compilation for all dialects, silently breakingwith_loader_criteriaon non-Snowflake dialects (PostgreSQL, MySQL, SQLite, etc.). The_extra_criteriafromwith_loader_criteriais not propagated into entities inside sealed.subquery()calls because_Snowflake_ORMJoin.__init__is a stale copy of_ORMJoin.__init__that bypasses the standard initialization chain.Changes
base.py— Dialect guards: Added dialect detection toSnowflakeSelectStateandSnowflakeORMSelectCompileState. All overridden methods now delegate tosuper()for non-Snowflake dialects, ensuring standard SQLAlchemy behavior is fully preserved.util.py— Replace stale copies with thin wrappers:_Snowflake_ORMJoin.__init__and_Snowflake_Selectable_Join.__init__now delegate tosuper().__init__()and only interceptNoForeignKeysErrorfor the Lateral-without-onclause case (BCR-1057). This eliminates the stale copy problem and ensures future SA improvements (including_extra_criteriahandling) are inherited automatically.Regression test: Added
tests/test_loader_criteria_regression.pyverifyingwith_loader_criteriaworks correctly on both non-Snowflake and Snowflake dialects.Test plan
filter injected: Trueafter fixtest_outer_lateral_joinpasses (Snowflake lateral join without ON clause still works)🤖 Generated with Claude Code