perf(optimizer): speed up merge_subqueries#7924
Conversation
SQLGlot Integration Test Results❌ 3 unit test failures — see details belowComparing:
By Dialect
Overallmain: 192414 total, 153544 passed (pass rate: 79.8%) sqlglot:optimizer/performance-merge_subqueries: 180220 total, 142397 passed (pass rate: 79.0%) Transitions: Dialect pair changes: 0 previous results not found, 3 current results not found ❌ 3 unit test failures (view logs) |
| scopes = traverse_scope(expression) | ||
| expression = merge_ctes(expression, leave_tables_isolated, scopes=scopes) | ||
| expression = merge_derived_tables(expression, leave_tables_isolated, scopes=scopes) |
There was a problem hiding this comment.
We can't reuse scopes like this across the two passes, because they both mutate the AST heavily in a way that invalidates the scope's tree structure & the relevant state.
Try this input:
WITH c AS (SELECT a FROM (SELECT a, b FROM x) AS q WHERE q.b > 1)
SELECT c.a FROM c JOIN y ON c.a = y.aWe should add this as a test.
There was a problem hiding this comment.
We could change this a bit and reuse scopes if nothing was mutated in merge_ctes, perhaps.
There was a problem hiding this comment.
I was concerned about that, but the tests all passed (and I couldn't think up a query that would fail). I'll add that new test in. This part had the most impact in terms of performance gains, so yes, if would be worth checking if anything mutated (and re-initialize the scope if so).
There was a problem hiding this comment.
Check these out as well:
-- CTE merge displaces a derived table that outer columns reference
WITH c AS (SELECT a FROM (SELECT a, b FROM x) AS q WHERE q.b > 1) SELECT c.a FROM c JOIN y ON c.a = y.a
-- rename-conflict path
WITH c AS (SELECT q.a FROM (SELECT x.a FROM x AS x) AS q) SELECT c.a FROM c JOIN x ON c.a = x.a| def _window_projection_blocks_merge(): | ||
| def _window_projection_blocks_merge(window_aliases): |
There was a problem hiding this comment.
Let's add type hints in these closures.
|
|
||
| outer = outer_scope.expression | ||
| if outer.args.get("where") or outer.args.get("joins"): | ||
| if outer_args.get("where") or outer_args.get("joins"): |
There was a problem hiding this comment.
I'd define outer_args before the closures, _mergeable is confusing in its current form. Same goes for other variables (e.g., inner_name) that are defined after them.
| outer = outer_scope.expression | ||
| if not isinstance(outer, exp.Select): | ||
| return False | ||
| if outer.is_star: | ||
| return False | ||
| if not isinstance(inner_select, exp.Select): | ||
| return False | ||
| if any(v for k, v in inner_select.args.items() if k in UNMERGABLE_ARGS): | ||
| return False | ||
| if inner_select.args.get("from_") is None: | ||
| return False | ||
| if outer_scope.pivots: | ||
| return False |
There was a problem hiding this comment.
Why are these split across N branches instead of combined via conjunction in a single branch?
| if _outer_select_joins_on_inner_select_join(projections): | ||
| return False | ||
| if _window_projection_blocks_merge(window_aliases): | ||
| return False | ||
| if _literal_group_unmergeable(number_literal_aliases): | ||
| return False | ||
| if _literal_in_order_by(number_literal_aliases): | ||
| return False | ||
| if inner_scope.is_cte and _is_recursive(): | ||
| return False | ||
| if inner_select.args.get("order") and outer_scope.is_union: | ||
| return False | ||
| if isinstance(seq_get(inner_select.expressions, 0), exp.QueryTransform): | ||
| return False |
| return expression | ||
|
|
||
|
|
||
| def _mergeable( |
There was a problem hiding this comment.
@fivetran-kwoodbeck let's please measure the impact of this refactor alone. I'd avoid the rewrite for now if it's negligent.
There was a problem hiding this comment.
Actually, I thought about this more and I'm ok with keeping the rewrite after all, as long as we prove the two forms are equivalent. Splitting up that huge connector chain currently in main seems better for readability and reusing / factoring out logic in the future.
It'd still be interesting to bench this separately, though.
2 participants
Performance rework of
merge_subquerieswith ~1.25x speedup. No changes to output SQL.Changes
merge_subqueriescallstraverse_scopeonce and threads scopes intomerge_ctesandmerge_derived_tables(was previously built twice)._mergeable: one loop overinner_select.selectsprecomputes projections, number_literal_aliases, window_aliases and does the AggFunc/Select/Explode check, replacing several re-walks. Helpers now receive this data instead of recomputing it.outer,outer_args, andinner_name; convert the boolean and-chain to early-return guards; run_is_recursiveonly wheninner_scope.is_cte.Performance (pyperf, timing only the rule; before = origin/main, after = this branch).
Check the Jira ticket for the timing script.