Fix subqueries in interpolated join wheres#4765
Conversation
| # The right side may be a QueryExpr (an explicit join `on`), | ||
| # which holds no subqueries |
There was a problem hiding this comment.
Can we make the right-side be a BooleanExpr as well always?
| {source, acc} = prewalk_source(join.source, :join, query, join, acc, adapter) | ||
| {on, acc} = prewalk(:join, query, join.on, acc, adapter) | ||
| {%{join | on: on, source: source, params: nil}, acc} | ||
| {%{join | on: on_to_query_expr(on), source: source, params: nil}, acc} |
There was a problem hiding this comment.
Why do we need to rollback? Let's keep it as boolean everywhere... if necessary, we change ecto_sql.
There was a problem hiding this comment.
Currently ecto_sql pattern matches on QueryExpr. Relaxing that is doable. I created a draft PR. The tests pass but I'm not convinced this is the right way. Providers copy the code patterns from ecto and will remain with silent failures (for expressions will filter out %BooleanExpr{}) if ecto_sql is loosly pinned.
Besides, after this change join.on would be mixed-type: BooleanExpr/QueryExpr. If you want booleans everywhere we would need a bigger refactor making the join builder emit BooleanExpr for all ons
There was a problem hiding this comment.
Besides, after this change join.on would be mixed-type: BooleanExpr/QueryExpr. If you want booleans everywhere we would need a bigger refactor making the join builder emit BooleanExpr for all ons
Correct. That's what we should do. We already did this change a while ago to support subqueries in where. This is just a natural continuation of it.
There was a problem hiding this comment.
Is that what you had in mind?
Make the join builder construct every JoinExpr.on as a BooleanExpr,
instead of only the ons synthesized from interpolated join queries.
This removes the mixed QueryExpr/BooleanExpr on types and the
conversion back to QueryExpr during normalization: adapters now
receive a BooleanExpr on for every join.
With a single on type, merge_expr_and_params collapses to one clause
and attach_on no longer needs to upgrade the explicit on.
Join entries in cache keys change shape from `expr` to `{op, expr}`
as they now go through the BooleanExpr expr_to_cache clause.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The map patterns and comment date from the intermediate design where join.on was mixed-type; it is always a BooleanExpr now. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
josevalim
left a comment
josevalim
left a comment
There was a problem hiding this comment.
@greg-rychlewski you also happy with this one?
2 participants
Summary
Fix planning an interpolated join query whose
whereexpression contains a subquery.This is an alternative implementation to #4764 that incorporates the review suggestion to keep subqueries off
QueryExpr:BooleanExpr, which already owns subqueries.BooleanExprback toQueryExprafter normalization, preserving the contract expected by SQL adapters.Root cause
Interpolated query filters are folded into the join
on. Previously that produced aQueryExprwithout thesubqueriesmetadata required to resolve{:subquery, index}placeholders, causing aKeyErrorduring parameter planning.The join cache key also needs the nested subquery cache keys; otherwise structurally different subqueries can collide and reuse the wrong prepared SQL.
Validation
mix compile --warnings-as-errorsmix test— 1,581 tests passedRegression coverage verifies planning, subquery parameter/index merging, cache-key differentiation, normalization-time inlining, and restoration of
JoinExpr.ontoQueryExpr.