fix(core): expand the wildcard before Wren rewrite rules #1145
Conversation
WalkthroughThe pull request updates the rule application order in two functions by preemptively adding an Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🔇 Additional comments (4)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
WalkthroughThe changes update the rule analysis in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Client
participant Context as Rule Analyzer
participant Rules as Rule List
Caller->>Context: Invoke analyze_rule_for_local_runtime/unparsing
Context->>Rules: Prepend ExpandWildcardRule (Arc::new(...))
Rules-->>Context: Rule list updated
Context->>Caller: Return updated rule vector
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code Graph Analysis (1)wren-core/core/src/mdl/mod.rs (5)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (7)
✨ Finishing Touches
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
WalkthroughThe pull request introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Test as "Test Function"
participant Analyzer as "Analyzer"
participant ExpandRule as "ExpandWildcardRule"
participant OtherRules as "Subsequent Rules"
Test->>Analyzer: Invoke analyze_rule_for_local_runtime/unparsing
Note right of Analyzer: Insert ExpandWildcardRule at start of rule chain
Analyzer->>ExpandRule: Process wildcard expansion
ExpandRule-->>Analyzer: Return expanded query fragment
Analyzer->>OtherRules: Apply remaining analysis rules
OtherRules-->>Analyzer: Return further modifications
Analyzer-->>Test: Return final analyzed query
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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
🔭 Outside diff range comments (2)
wren-core/core/src/mdl/context.rs (2)
111-111: 🛠️ Refactor suggestionConsider removing this duplicate rule instance.
Since you've added the
ExpandWildcardRuleas the first rule (line 95), this second instance at line 111 appears redundant and might lead to confusion. Consider removing it if the rule doesn't need to be applied twice, or add a comment explaining why two instances are necessary.
140-140: 🛠️ Refactor suggestionConsider removing this duplicate rule instance.
Since you've added the
ExpandWildcardRuleas the first rule (line 124), this second instance at line 140 appears redundant. Consider removing it if the rule doesn't need to be applied twice, or add a comment explaining why two instances are necessary.
🧹 Nitpick comments (2)
wren-core/core/src/mdl/context.rs (2)
94-95: Fix typo in the comment while adding necessary rule.The comment has a typo "lastest" which should be "latest" and also contains a duplicate "this". More importantly, I notice that there's another instance of
ExpandWildcardRule::new()at line 111. Please clarify the intention of having two instances of the same rule.- // To align the lastest change in datafusion, apply this this rule first. + // To align the latest change in datafusion, apply this rule first.
123-124: Fix typo in the comment while adding necessary rule.Similar to the previous comment, the comment has a typo "lastest" which should be "latest" and also contains a duplicate "this". Also, there's a duplicate instance of this rule at line 140.
- // To align the lastest change in datafusion, apply this this rule first. + // To align the latest change in datafusion, apply this rule first.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wren-core/core/src/mdl/context.rs(2 hunks)wren-core/core/src/mdl/mod.rs(5 hunks)wren-core/sqllogictest/test_files/model.slt(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
wren-core/core/src/mdl/mod.rs (4)
wren-core/core/src/mdl/context.rs (1)
new(232-235)wren-core/core/src/logical_plan/analyze/plan.rs (6)
new(66-78)new(104-117)new(619-621)new(735-829)new(883-930)new(989-991)wren-core/core/src/logical_plan/analyze/model_anlayze.rs (3)
new(67-75)new(788-796)analyze(45-59)wren-core/core/src/logical_plan/analyze/model_generation.rs (2)
new(33-38)analyze(232-243)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: ci
- GitHub Check: cargo test (win64)
- GitHub Check: cargo test (macos)
- GitHub Check: cargo check
- GitHub Check: test
🔇 Additional comments (2)
wren-core/sqllogictest/test_files/model.slt (1)
75-84: Good test cases added for wildcard expansion with filtering.These test cases effectively validate the fix for the issue mentioned in the PR description, where SQL queries with wildcards and filters were failing to include all necessary columns. The tests cover both multi-value (IN clause) and single-value equality filters.
wren-core/core/src/mdl/mod.rs (1)
1415-1440:❓ Verification inconclusive
Good test function for validating wildcard expansion.
This test function properly validates that wildcards are expanded correctly when combined with filtering conditions. The test creates a simple model with two columns and verifies that the transformed SQL correctly expands the wildcard to include both columns while maintaining the filter condition.
Run the following script to verify that the new test passes with the changes:
🏁 Script executed:
#!/bin/bash # Run the new test to verify that it passes with the changes cd wren-core/core cargo test --test-threads=1 mdl::test::test_wildcard_where -- --nocaptureLength of output: 191
Action: Manual Verification Required of the Rust Test Suite
The test function in
wren-core/core/src/mdl/mod.rs(lines 1415–1440) properly validates that wildcard expansion works as intended when combined with filter conditions. However, the initial attempt to run the test failed due to the absence of Cargo in the execution environment. Please ensure you run the test in an environment with Cargo available by executing:cd wren-core/core cargo test --test-threads=1 mdl::test::test_wildcard_where -- --nocaptureVerify that the test passes and produces the expected SQL transformation output.
The result of the following SQL is wrong:
The generated SQL will miss the columns because Wren Core only collects the column in the filter.
Since the
ExpandWildcardRulewill be removed from the latest DataFusion branch. To align this change, I apply the rule before all rules. This behavior is similar to expanding the wildcard in the logical plan builder.Summary by CodeRabbit
Refactor
Tests