perf: speed up SQL auto-categorization - #5924
Conversation
Signed-off-by: dirk.nilius <dirk.nilius@nc-group.net>
|
Nice find and fix. Confirmed significant speedups myself (160x on a 400 projection model). Looks like all of the categorizations are safe and / or safer (in the case of ORDER BY 2 being NON_BREAKING and now classified as undetermined). Only one additional easy win - the ORDER BY 2 fix won't kick in when ORDER BY sits after a UNION. Since you've already modified that area, worth a patch. Some other nits listed in-line |
| if len(this_projections) <= len(previous_projections): | ||
| return None | ||
| this_index = 0 | ||
| added_at: list[int] = [] |
There was a problem hiding this comment.
nit: these two lists could probably just be a boolean and a last-match index — the only question being answered at the end is "did anything get added before the last matched projection?" The appends in the trailing loop can never trip that check anyway (they're all past the last match), so they're effectively dead work. Optional — the any(...) at the bottom just takes a moment to unpack as written.
(tiny thing: list[int] here vs t.List everywhere else in the file)
| ) | ||
|
|
||
|
|
||
| def _projection_lists_match(previous_query: exp.Select, this_query: exp.Select) -> bool: |
There was a problem hiding this comment.
nit: the name is a little misleading — it returns True when the lists don't match, they just differ by safe additions. Maybe something like _projections_only_safely_added? Similar thought on _is_valid_added_projection — "valid" undersells what it checks, which is really "won't change row counts".
|
|
||
| # Be conservative about every mid-list addition when ordinals are present. Determining whether | ||
| # a particular ordinal was shifted would couple this comparison to dialect-specific semantics. | ||
| if ( |
There was a problem hiding this comment.
As mentioned:
One case slips past this guard: when the ORDER BY 2 comes after a UNION, the ordinal lives on the Union node rather than the Selects — so adding a column mid-list in both branches still comes out NON_BREAKING even though the "2" now points at a different column. The old code had the same blind spot, so it's not a regression, but since this rewrite makes it easy to reach: checking ordinal refs on the nearest set-op ancestor when there's a mid-list add would close it. A UNION + ORDER BY 2 case in the new parametrized test would pin it down.
| ), | ||
| ], | ||
| ) | ||
| def test_categorize_change_sql_nested_projection_additions( |
There was a problem hiding this comment.
nit: one more case that would be nice here: a plain SELECT a, c ... ORDER BY 2 → SELECT a, b, c ... ORDER BY 2. That one actually changed behavior in this PR (used to be NON_BREAKING, now undetermined — which seems like the right call), so pinning it down would be good.
Description
Motivation
SqlModel.is_breaking_changecurrently calls SQLGlot's general-purposediffimplementation to answer a much narrower question: whether a rendered query changed exclusively through addedSELECTprojections.SQLGlot's tree diff computes candidate matchings across both ASTs, introducing quadratic work in its matching phases.
SQLMesh performs this work synchronously for each model during auto-categorization. We encountered the resulting
amplification in a project with more than
600models: a broadly used macro change can require hundreds of models tobe categorized, repeatedly paying the superlinear cost for large rendered queries and causing
sqlmesh planto spendmore than an hour in categorization without completing; the plan was cancelled at that point.
The existing
_additive_projection_changefallback handles cases where SQLGlot emits spuriousMoveorUpdateedits, such as repeated cast types. However, the fallback runs only after the expensive tree diff has completed, so it does not address the performance problem.What changed
This replaces the general tree diff and its fallback with a specialized AST comparison:
SELECTprojection list as an ordered subsequence of the current list.CASTtypes.CTEs,EXISTSqueries,UNIONs, and other nested query structures.The comparison is linear in the traversed AST and projection lists rather than performing general candidate matching across the trees.
Safety and behavior
The specialized comparison preserves SQLMesh's conservative categorization rules:
FROM,WHERE,GROUP BY,ORDER BY,DISTINCT, or other query structure remain undetermined.UDTFs remain undetermined because they can change row cardinality.UDTFis accepted only when its nearestSubqueryancestor is contained within the added projection.GROUP BYorORDER BYuses ordinal references, because inserting a projection can shift the referenced output.None, retaining SQLMesh's conservative fallback behavior.There are no public API or configuration changes.
Performance
Measured on
mainate075200using Python3.13.13and SQLGlot30.8.0on macOS ARM64.Both ASTs were parsed before timing. Each benchmark copied the previous query and added one scalar projection. The measurements time only the existing
sqlglot.diffcall and the specialized comparison.sqlglot.diffThese are single-process wall-clock measurements intended to show the order-of-magnitude difference; parsing and rendering are excluded from both sides.
A representative breaking change on the same large model retained the conservative result while dropping comparison time from approximately
12.1seconds to20milliseconds.Test Plan
Added coverage for:
CASTtypes.UDTFs.UDTFs inside projection subqueries.UDTFs inside derived-table projections.CTEprojection additions.EXISTS.UNIONbranches.ORDER BYsafeguards.Validation performed:
make styleruff,ruff-format,mypy, and migration validation.python -m pytest tests/core/test_snapshot.py -k 'categorize_change_sql' -q8passed.500generated projection and non-projection mutations against the current implementation.make fast-test2,576tests successfully, skipped4,and reported one order-dependent error in
tests/dbt/cli/test_selectors.py::test_select_by_dbt_names[dbt_select6-expected6].macrosremains in the samexdistworker and
dbt-commonattempts to inspect it. No changed code is present in the traceback.14test_select_by_dbt_namescases serially passes.2,563passed and4skipped.make fast-testalso pass when invoked separately:3isolated,1registry_isolation, and159dialect_isolatedtests.Checklist
make styleand fixed any issues.make fast-test).git commit -s) per the DCO.