Repository navigation
fix(wren): resolve model aliases within SQL query scopes - #2798
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughCTE rewriting now resolves model aliases and column references per SQL scope. It also detects stars by scope. Regression tests cover reused aliases, correlated references, nested queries, and case-distinct columns. ChangesScope-aware CTE rewriting
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to No merge-blocking issue is identified; merge after normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks each query’s scope, Comment |
|
Added docstrings for scoped column collection and the new regression fixtures and cases in 6775299. They describe how correlated columns retain their outer binding and what each alias-reuse test checks. This update only adds documentation; the scope-resolution behavior is unchanged. The CTE rewriter, case-sensitivity, and engine suites still pass: 97 tests. Ruff check, format check, and the diff whitespace check also pass. |
goldmedal
left a comment
There was a problem hiding this comment.
Thanks @dafyy321-pixel 👍
Summary
Fixes #2795.
Resolve model aliases within each SQL query scope so valid UNION, INTERSECT, EXCEPT, and nested queries can reuse table aliases. Previously, an alias reused by another SELECT could make Wren select a column from the wrong model and reject the query during planning.
The fix uses sqlglot's existing
traverse_scope()support to record aliases separately for each scope before identifier normalization. Column collection then uses the map belonging to the column's scope. Derived-table and CTE sources resolve to scope objects, so their output columns are no longer attributed to a model merely because an alias has the same spelling elsewhere.The same scope maps are used for the case-sensitive column path and for qualified-column normalization in Oracle/Snowflake output. Star detection now checks each SELECT's own sources, including nested SELECTs:
t.*expands the model bound to thatt, and a bare*marks the models in that scope. Model expansion continues to pass stars through wren-core. This replaces the old top-level-only SELECT walker, which is no longer needed.What failure does this repair?
With an
ordersmodel exposingidandamount, and acustomersmodel exposingidandname, this SQL is valid:Before this change,
WrenEngine.dry_plan()produces the following error:_build_alias_map()previously stored both uses oftin a single dictionary. The customers branch overwrote the orders branch, and column collection built a per-model query containingcustomers.amount. SQL gives each UNION branch its own alias scope, so these references must be collected separately.The complete MDL, setup, and runnable SDK reproduction are in #2795. With orders amounts
10and20, and customer namesAliceandBob, direct DuckDB execution returns[(3,), (5,), (10,), (20,)]after sorting. The same reproduction now plans successfully and returns those values when the planned SQL is executed in DuckDB.The regression also occurs without a set operation:
Here the inner
tbelongs tocustomer; it must not change the binding of the outert.o_orderkey. Related regression tests cover an outer alias reused for a derived table or CTE and a qualified star whose alias is reused in a nested SELECT.How is it tested?
Added 19 test cases in
tests/unit/test_cte_rewriter.py. They call the real CTE rewriter and semantic engine rather than matching source text. The result tests execute rewritten SQL in a local DuckDB database.transform_sql()calls to check that the correct model receivesSELECT *.The correlated-reference case also guards an outer-scope binding that already worked before the change.
From
core/wren, the related suites pass:uv run --no-sync pytest tests/unit/test_cte_rewriter.py \ tests/unit/test_case_sensitivity.py tests/unit/test_engine.py -q # 97 passedThe changed files also pass
ruff check,ruff format --check, andgit diff --check. The new tests are in the ordinary unit-test tree, which.github/workflows/wren-ci.ymlruns without requiring a connector service or an optional extra.The PostgreSQL, Oracle, and Snowflake cases use their dialect parsers and the real semantic engine, then transpile the planned SQL to DuckDB to compare results. They do not connect to live instances of those databases. The local run used
wren-core-py 0.8.0; CI's unit-test job builds the engine bindings from source.Duplicate check
The alias-map limitation was acknowledged by AmirF194 in this comment on #2743, which describes it as a pre-existing column-resolution gap deserving a separate follow-up. The comment does not explicitly claim that follow-up. This PR implements the separate scope-resolution fix discussed there.
Checked the open #2783, which also changes
core/wren/src/wren/mdl/cte_rewriter.py. That PR repairs ClickHouse SQL translation and changes different hunks in the rendering/translation path. This PR changes alias maps, column attribution, column-case handling, and star collection; it leaves those translation hooks alone. The shared file still needs a fresh comparison when rebasing before merge.Summary by CodeRabbit