fix(core): disable DecorrelatePredicateSubquery rule#1297
fix(core): disable DecorrelatePredicateSubquery rule#1297douenergy merged 1 commit intoCanner:mainfrom
Conversation
WalkthroughRemoved DecorrelatePredicateSubquery from the unparse optimization rule set in mdl/context.rs. Added an async test in mdl/mod.rs to validate behavior when analyzing SQL with an IN-subquery under Unparse mode via snapshot assertion. No public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test
participant M as AnalyzedWrenMDL
participant C as Context (Unparse)
participant O as Optimizer
T->>M: analyze(manifest, Mode::Unparse, sql)
M->>C: build unparse optimization rules
Note over C: DecorrelatePredicateSubquery is excluded
C->>O: provide rule set (without decorrelation)
O-->>M: optimized plan for unparsing
M-->>T: transformed SQL (snapshot assertion)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
wren-core/core/src/mdl/context.rs (2)
228-229: Add a tracking note (with link) so we remember to re-enable later.Please enrich the comment with a reference to this PR (and ideally the upstream tracker) to make reversal straightforward when Unparser supports decorrelated predicate subqueries.
Apply this diff to improve the inline comment:
- // Unparser has some issues for handling decorrelated plans - // Arc::new(DecorrelatePredicateSubquery::new()), + // Unparser currently has issues handling decorrelated predicate-subquery plans. + // Temporarily disabled to prevent invalid SQL emission. + // TODO(wren-engine#1297): Re-enable once upstream Unparser can serialize these plans correctly. + // Arc::new(DecorrelatePredicateSubquery::new()),
228-229: Optional: make this toggleable via a session/config flag.If you foresee testing with engines that can handle the decorrelated shape, consider gating this rule behind a config (e.g., x-wren-enable-decorrelate-subquery) so we can flip it without a code change.
wren-core/core/src/mdl/mod.rs (1)
1036-1067: Broaden coverage: add correlated IN and NOT IN cases.Two additional tests will harden against future optimizer changes:
- Correlated IN: WHERE t.a IN (SELECT u.a FROM t u WHERE u.b = t.b)
- NOT IN null-semantics: WHERE c NOT IN (SELECT c FROM ...), ensuring Unparser retains subquery form.
Example additions (place near this test):
#[tokio::test] async fn test_disable_decorrelate_predicate_subquery_correlated_in() -> Result<()> { let manifest = ManifestBuilder::new() .catalog("wren").schema("test") .model( ModelBuilder::new("t") .table_reference("t") .column(ColumnBuilder::new("a", "int").build()) .column(ColumnBuilder::new("b", "int").build()) .build(), ) .build(); let analyzed_mdl = Arc::new(AnalyzedWrenMDL::analyze(manifest, Arc::new(HashMap::default()), Mode::Unparse)?); let sql = r#"SELECT * FROM wren.test.t AS t1 WHERE t1.b IN (SELECT t2.b FROM wren.test.t AS t2 WHERE t2.a = t1.a)"#; let actual = transform_sql_with_ctx(&SessionContext::new(), Arc::clone(&analyzed_mdl), &[], Arc::new(HashMap::new()), sql).await?; // Assert the predicate remains an IN-subquery (not rewritten to a join) assert!(actual.contains("IN (SELECT")); Ok(()) } #[tokio::test] async fn test_disable_decorrelate_predicate_subquery_not_in() -> Result<()> { let manifest = ManifestBuilder::new() .catalog("wren").schema("test") .model( ModelBuilder::new("t") .table_reference("t") .column(ColumnBuilder::new("c", "int").build()) .build(), ) .build(); let analyzed_mdl = Arc::new(AnalyzedWrenMDL::analyze(manifest, Arc::new(HashMap::default()), Mode::Unparse)?); let sql = r#"SELECT * FROM wren.test.t WHERE c NOT IN (SELECT c FROM wren.test.t)"#; let actual = transform_sql_with_ctx(&SessionContext::new(), Arc::clone(&analyzed_mdl), &[], Arc::new(HashMap::new()), sql).await?; // Assert NOT IN subquery shape is preserved assert!(actual.contains("NOT IN (SELECT")); Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
wren-core/core/src/mdl/context.rs(1 hunks)wren-core/core/src/mdl/mod.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: cargo test (macos-aarch64)
- GitHub Check: cargo test (win64)
- GitHub Check: cargo test (macos)
- GitHub Check: cargo check
- GitHub Check: ci
- GitHub Check: test
🔇 Additional comments (3)
wren-core/core/src/mdl/context.rs (2)
228-229: Disabling DecorrelatePredicateSubquery for Unparse mode looks correct.This avoids generating semi-join style plans that the Unparser cannot reliably serialize. Given ScalarSubqueryToJoin is already disabled, keeping IN-subqueries intact is the safer path for SQL generation.
228-229: No remaining references toDecorrelatePredicateSubqueryA search for any live occurrences of
DecorrelatePredicateSubqueryonly turned up the commented‐out line incontext.rs(lines 227–231). There are no active imports or usages elsewhere in the repo, so this cleanup is complete.wren-core/core/src/mdl/mod.rs (1)
1036-1067: Good targeted snapshot test to assert IN-subquery remains undecorrelated.Covers the intended regression surface and matches the Unparse-mode contract. Nice use of Unicode identifiers to exercise quoting paths.
2 participants
DecorrelatePredicateSubquery will generate an invalid plan for the DataFusion Unparser.
Summary by CodeRabbit
Bug Fixes
Tests