fix: Fix bug with structurally equal correlated subqueries#22313
fix: Fix bug with structurally equal correlated subqueries#22313neilconway wants to merge 1 commit into
Conversation
github-actions
Bot
added
optimizer
There was a problem hiding this comment.
@neilconway
Thanks for the fix here. I walked through the rewrite flow and the approach makes sense overall. I just had one small suggestion that could make the bookkeeping a bit more explicit and easier to reason about in the future.
| #[allow(clippy::allow_attributes, clippy::mutable_key_type)] | ||
| // Expr contains Arc with interior mutability but is intentionally used as hash key | ||
| let mut subquery_to_expr_map = HashMap::new(); | ||
| let mut alias_to_expr_map: HashMap<String, Expr> = HashMap::new(); |
There was a problem hiding this comment.
Nice catch on the alias mapping issue. One thought: it might be a little simpler and more robust to key the projection rewrite bookkeeping by projection index, or store alias -> projection index, instead of alias -> cloned Expr plus Expr -> rewrite Expr.
The current fix works because the generated aliases are unique, but using indexes would encode the ownership relationship directly and avoid another structural Expr lookup in this already pretty subtle rewrite path.
2 participants
Which issue does this PR close?
Rationale for this change
If a query contained two or more structurally equal correlated subqueries, we previously would produce incorrect query results. This was caused by using
Subqueryas a hashmap key; if we use the subquery alias instead, we can avoid the unintended key collision. Using the alias (a string) as the hashmap key is also safer and more efficient than hashing on a complex type likeSubquery.What changes are included in this PR?
ScalarSubqueryToJoinrewrite passAre these changes tested?
Yes, with new test added.
Are there any user-facing changes?
No, aside from fixing the previously incorrect query results.