fix(core): remove default window frame clause

[goldmedal]

Given a SQL:

SELECT rank() OVER (PARTITION BY o_custkey ORDER BY o_orderdate) FROM orders

DataFusion unparser will generate the window frame clause with the default value RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW for it. However, it's not only unnecessary but makes the SQL be invalid for some window functions (e.g rank, dense_rank). Some databases (e.g BigQuery, redshift,.. ) don't support invoke rank with the window frame.

This PR checks if the window frame is the default value and skip generating window frame if required.

Summary by CodeRabbit

• Bug Fixes

  • Improved handling of window function frames to prevent an unsupported frame variant, resulting in clearer errors and consistent behavior across other cases.

• Tests

  • Added comprehensive tests for window function frame generation, covering default and explicit frames, with snapshot verification to ensure stable SQL output.

[coderabbitai]

Walkthrough

Adds a guard in WrenDialect.window_func_support_window_frame to return false for frames from Preceding(None) to CurrentRow; otherwise delegates to the inner dialect. Introduces a new test test_window_function_frame (duplicated) verifying SQL transformation for default and explicit window frames using assert_snapshot.

Changes

Cohort / File(s)	Summary
Dialect window-frame guard wren-core/core/src/mdl/dialect/wren_dialect.rs	Adds conditional check blocking support when start is Preceding(None) and end is CurrentRow; delegates all other cases to inner_dialect.window_func_support_window_frame.
MDL SQL transform tests wren-core/core/src/mdl/mod.rs	Adds test_window_function_frame covering default window function behavior and explicit ROWS BETWEEN frame; test appears twice (duplicate).

Sequence Diagram(s)

sequenceDiagram
    participant C as Caller
    participant WD as WrenDialect
    participant ID as inner_dialect

    C->>WD: window_func_support_window_frame(func, start, end)
    alt start=Preceding(None) and end=CurrentRow
        note over WD: Guard triggers → unsupported
        WD-->>C: false
    else other window-frame shapes
        WD->>ID: window_func_support_window_frame(func, start, end)
        ID-->>WD: bool
        WD-->>C: bool
    end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• feat(core): add BigQuery EXTRACT function support and window frame validation #1279 — Adjusts the same window_func_support_window_frame method with a similar guard, indicating a directly related change path.

Suggested labels

core, rust

Suggested reviewers

• douenergy

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title “fix(core): remove default window frame clause” clearly and concisely captures the main change of skipping the default window frame clause in the SQL unparser without including extraneous details or file lists.

Poem

I hop through frames, both near and far,
From PRECEDING void to CURRENT star—
A tiny guard, a cautious pose,
No bounds unbound where logic goes.
Two tests echo, thump-thump cheer,
SQL blooms—windows clear.
Carrots up! The path is here. 🥕

✨ Finishing Touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

wren-core/core/src/mdl/dialect/wren_dialect.rs (1)

87-97: Restrict default-frame elision to ranking functions in WrenDialect
Eliding UNBOUNDED PRECEDING…CURRENT ROW for every function can turn explicit ROWS bounds into RANGE semantics. In wren-core/core/src/mdl/dialect/wren_dialect.rs, within fn window_func_support_window_frame, only skip emitting the default frame when func_name (lowercased) is “rank”, “dense_rank”, or “row_number”.

wren-core/core/src/mdl/mod.rs (1)

3392-3428: Add explicit default RANGE frame elision test

No duplicate test_window_function_frame found. Inside the existing test in wren-core/core/src/mdl/mod.rs, add:

         // assert default won't generate the window frame
         let sql = "SELECT rank() OVER (PARTITION BY o_custkey ORDER BY o_orderdate) FROM orders";
         assert_snapshot!(
             transform_sql_with_ctx(&ctx, Arc::clone(&analyzed_mdl), &[], Arc::clone(&headers), sql).await?,
             @"SELECT rank() OVER (PARTITION BY orders.o_custkey ORDER BY orders.o_orderdate ASC NULLS LAST) FROM (SELECT orders.o_custkey, orders.o_orderdate FROM (SELECT __source.o_custkey AS o_custkey, __source.o_orderdate AS o_orderdate FROM orders AS __source) AS orders) AS orders"
         );
+
+        // explicit default RANGE frame should also be elided
+        let sql = "SELECT rank() OVER (PARTITION BY o_custkey ORDER BY o_orderdate RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) FROM orders";
+        assert_snapshot!(
+            transform_sql_with_ctx(&ctx, Arc::clone(&analyzed_mdl), &[], Arc::clone(&headers), sql).await?,
+            @"SELECT rank() OVER (PARTITION BY orders.o_custkey ORDER BY orders.o_orderdate ASC NULLS LAST) FROM (SELECT orders.o_custkey, orders.o_orderdate FROM (SELECT __source.o_custkey AS o_custkey, __source.o_orderdate AS o_orderdate FROM orders AS __source) AS orders) AS orders"
+        );

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 551f2c8 and d2c1535.

📒 Files selected for processing (2)

• wren-core/core/src/mdl/dialect/wren_dialect.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: ci
• GitHub Check: cargo test (macos)
• GitHub Check: cargo test (macos-aarch64)
• GitHub Check: test
• GitHub Check: cargo check
• GitHub Check: cargo test (win64)