feat(core): add BigQuery EXTRACT function support and window frame validation#1279
Conversation
WalkthroughBigQuery-specific rewrite of date_part to EXTRACT, a dialect hook for window-frame support with WrenDialect forwarding, added BigQuery tests, and small refactors: explicit Cow lifetime, Option accumulation refactor, and a test allocation change. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test/Client
participant Analyzer as Analyzer
participant Dialect as BigQueryDialect
participant Helper as datetime_field_from_*
participant Unparser as Unparser
Test->>Analyzer: analyze/unparse scalar function call date_part(kind, expr)
Analyzer->>Dialect: scalar_function_to_sql_overrides("date_part", args)
Dialect->>Helper: datetime_field_from_expr(kind)
Helper-->>Dialect: DateTimeField or plan_err
Dialect->>Unparser: build EXTRACT(DateTimeField FROM unparse(expr))
Unparser-->>Analyzer: SQL AST / SQL string
Analyzer-->>Test: transformed SQL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Graph Analysis (1)wren-core/core/src/mdl/dialect/inner_dialect.rs (1)
🔇 Additional comments (7)
✨ 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: 2
🧹 Nitpick comments (3)
wren-core/core/src/mdl/dialect/inner_dialect.rs (1)
145-195: Avoid allocation and locale-sensitive case transforms in mapping
to_uppercase()allocates and is locale-sensitive. These tokens are ASCII; preferto_ascii_uppercase()to avoid surprises (e.g., Turkish locale) and reduce alloc churn.Apply this minimal diff:
-fn datetime_field_from_str(s: &str) -> Result<ast::DateTimeField> { - match s.to_uppercase().as_str() { +fn datetime_field_from_str(s: &str) -> Result<ast::DateTimeField> { + let s_upper = s.to_ascii_uppercase(); + match s_upper.as_str() {Optionally, consider switching to
eq_ignore_ascii_caseper arm, but the above change keeps the current structure with safer semantics.wren-core/core/src/mdl/mod.rs (1)
933-959: Good BigQuery EXTRACT roundtrip coverageThis test validates the intended unparse behavior for BigQuery. Consider adding a case that starts with
date_part('year', o_orderdate)to ensure it is also rendered asEXTRACT(YEAR FROM ...), and a couple more fields (e.g., MONTH, WEEK) to broaden coverage.wren-core/core/src/logical_plan/analyze/model_generation.rs (1)
100-108: Simplify filter combination by flattening firstYour rewrite is correct and avoids unwraps. You can make it more concise and maintain identical semantics by flattening first and reducing with
Expr::and.Apply this diff:
- let rls_filter = filters - .into_iter() - .reduce(|acc, filter| { - if let Some(acc) = acc { - if let Some(filter) = filter { - Some(acc.and(filter)) - } else { - Some(acc) - } - } else { - filter - } - }) - .flatten(); + let rls_filter = filters + .into_iter() + .flatten() + .reduce(Expr::and);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wren-core-base/src/mdl/utils.rs(1 hunks)wren-core/core/src/logical_plan/analyze/model_generation.rs(2 hunks)wren-core/core/src/mdl/dialect/inner_dialect.rs(2 hunks)wren-core/core/src/mdl/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
wren-core/core/src/mdl/dialect/inner_dialect.rs (2)
wren-core/core/src/mdl/dialect/wren_dialect.rs (3)
scalar_function_to_sql_overrides(60-89)args(114-117)args(153-168)wren-core/core/src/mdl/dialect/utils.rs (1)
args(30-48)
wren-core/core/src/mdl/mod.rs (3)
wren-core/core/src/mdl/dialect/wren_dialect.rs (2)
new(107-111)default(101-103)wren-core/core/src/mdl/context.rs (1)
new(339-361)wren-core-base/manifest-macro/src/lib.rs (1)
manifest(26-56)
⏰ 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: test
- GitHub Check: ci
- GitHub Check: cargo test (macos-aarch64)
- GitHub Check: cargo test (macos)
- GitHub Check: cargo test (win64)
- GitHub Check: cargo check
🔇 Additional comments (2)
wren-core-base/src/mdl/utils.rs (1)
46-52: No downstream impacts from explicit lifetime onquote_identifier
The change topub fn quote_identifier(s: &str) -> Cow<'_, str>is semantically identical to the elided form. A quick scan shows only one Rust call site which immediately calls.to_string(), converting theCow<'_, str>into an ownedString:• wren-core-base/src/mdl/manifest.rs:153 —
.map(|x| quote_identifier(&x).to_string())Since
.to_string()always produces an ownedString, there’s no behavioral change or breakage.wren-core/core/src/logical_plan/analyze/model_generation.rs (1)
236-237: Return a planning error when measure lacks alias (style/flow)Switching to a direct
plan_err!in the else branch is fine and keeps control flow clear. No change requested.
goldmedal
changed the title
EXTRACT for BigQuery
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wren-core/core/src/mdl/dialect/inner_dialect.rs (1)
172-180: LGTM! Proper handling of both UTF8 variants.The function correctly handles both
Utf8andLargeUtf8scalar value types, preventing potential issues with large string literals.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wren-core/core/src/mdl/dialect/inner_dialect.rs(3 hunks)wren-core/core/src/mdl/dialect/wren_dialect.rs(2 hunks)wren-core/core/src/mdl/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wren-core/core/src/mdl/mod.rs
🧰 Additional context used
🧬 Code Graph Analysis (2)
wren-core/core/src/mdl/dialect/wren_dialect.rs (1)
wren-core/core/src/mdl/dialect/inner_dialect.rs (2)
window_func_support_window_frame(57-64)window_func_support_window_frame(157-169)
wren-core/core/src/mdl/dialect/inner_dialect.rs (2)
wren-core/core/src/mdl/dialect/wren_dialect.rs (4)
window_func_support_window_frame(100-108)scalar_function_to_sql_overrides(60-89)args(125-128)args(164-179)wren-core/core/src/mdl/dialect/utils.rs (1)
args(30-48)
⏰ 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)
- GitHub Check: test
- GitHub Check: cargo test (macos-aarch64)
- GitHub Check: cargo test (win64)
- GitHub Check: cargo check
- GitHub Check: ci
🔇 Additional comments (4)
wren-core/core/src/mdl/dialect/wren_dialect.rs (1)
100-108: LGTM! Clean delegation pattern for window frame support.The implementation correctly delegates window frame support checks to the inner dialect, maintaining consistency with the existing delegation pattern used throughout the WrenDialect implementation.
wren-core/core/src/mdl/dialect/inner_dialect.rs (3)
57-64: LGTM! Well-designed trait method with sensible default.The new trait method follows the established pattern with underscore-prefixed parameters and provides a reasonable default of
truefor window frame support.
130-152: LGTM! Robust implementation with proper error handling.The
date_parttoEXTRACTconversion correctly addresses the BigQuery compatibility issue mentioned in the PR objectives. The implementation includes proper arity validation and leverages the helper functions for field parsing.
155-169: LGTM! Comprehensive list of unsupported window functions for BigQuery.The implementation correctly identifies window functions that don't support framing in BigQuery, aligning with BigQuery's documented limitations.
|
BigQuery will show an error message like: |
2 participants
Description
EXTRACTstatement to a function calldate_part. However, it's not a valid function for BigQuery. We should unparse thedate_partfunction back toEXTRACTstatementwindow_func_support_window_frameto check if a window function call should generate the window frame clause.Summary by CodeRabbit
New Features
Refactor
Tests