Skip to content

fix(cubesql): Support GROUP BY () empty grouping set#11331

Open
MazterQyou wants to merge 1 commit into
masterfrom
cubesql/group-by-tuple
Open

fix(cubesql): Support GROUP BY () empty grouping set#11331
MazterQyou wants to merge 1 commit into
masterfrom
cubesql/group-by-tuple

Conversation

@MazterQyou

Copy link
Copy Markdown
Member

Check List

  • Tests have been run in packages where changes have been made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Description of Changes Made

This PR bumps cube-js/arrow-datafusion@809d376, fixing GROUP BY () queries failing with Planning Error: Initial planning error: This feature is not implemented: Unsupported ast node Tuple([]) in sqltorel: GROUP BY () is a valid PostgreSQL empty grouping set equivalent to aggregation without GROUP BY. Related test is included.

@MazterQyou
MazterQyou requested review from a team as code owners July 22, 2026 18:05
@github-actions github-actions Bot added the rust Pull requests that update Rust code label Jul 22, 2026
@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MazterQyou's task in 33s —— View job


Code Review

Verdict: LGTM — small, well-scoped change.

Review details
  • Read PR diff (Cargo.toml bump + test)
  • Verify test expectations match existing patterns
  • Check for missing edge cases

Summary of changes

  • Bumps cube-js/arrow-datafusion from b01223b6809d3763 to pull in upstream support for Tuple([]) (empty grouping set) in the SQL-to-relational planner.
  • Adds cubescan_empty_group_by_tuple in rust/cubesql/cubesql/src/compile/test/test_cube_scan.rs verifying GROUP BY () yields the same V1LoadRequestQuery shape as an unaggregated query with two measures and a filter.
  • Corresponding lockfile updates in both packages/cubejs-backend-native/Cargo.lock and rust/cubesql/Cargo.lock — consistent.

Code quality

  • The test is clear, uses the existing convert_select_to_query_plan / find_cube_scan helpers, and asserts a full V1LoadRequestQuery struct rather than partial fields — good for regression detection.
  • Doc comment on the test succinctly explains the semantics of GROUP BY () as an empty grouping set. Nice.

Security / performance

  • No user-facing surface change beyond accepting a previously-rejected SQL construct. No security concerns.
  • No perf impact — parser fix only.

Testing

  • The added test covers the golden path (measures + WHERE + GROUP BY ()). Reasonable coverage for a targeted parser fix.
  • Optional follow-ups (not blocking):
    • A test with GROUP BY () and no filter to make sure the empty grouping set alone routes to the aggregate path.
    • A test combining GROUP BY () with an actual column grouping set (e.g. GROUP BY GROUPING SETS ((), (col))) if that's expected to be supported — otherwise a negative test asserting a clear error.
    • A test with dimensions/measures mixed under GROUP BY () to confirm dimensions coming from projection still resolve as expected.

Documentation

  • No docs needed — this is a bug fix for a standard SQL construct users would already expect to work.

Dependency bump risk

  • The bump is only 1 commit ahead on the fork (per the PR description) and scoped to the SQL-to-rel Tuple([]) case. Low blast radius, but worth a quick skim of 809d3763 diff before merge to confirm no unrelated changes rode along.

Nothing blocking here.

• Branch: `cubesql/group-by-tuple`

@MazterQyou
MazterQyou force-pushed the cubesql/group-by-tuple branch from a97c245 to fe501e4 Compare July 22, 2026 19:15
@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MazterQyou's task in 25s —— View job


Code Review

Verdict: LGTM — small, well-scoped fix. Already approved by @ovr; nothing blocking on re-review.

Review details
  • Re-read diff against origin/master (Cargo.toml rev bump + one new test)
  • Verified test structure matches neighboring cubescan_* tests in test_cube_scan.rs
  • Confirmed lockfile updates are consistent across both Cargo.lock files
  • Considered edge cases and follow-up test ideas

Summary of changes

  • Bumps cube-js/arrow-datafusion from d203d877809d3763 to pick up upstream support for Tuple([]) (empty grouping set) in the SQL→relational planner. (Note: the base rev here is d203d877, not b01223b6 as I said in the prior review — I misread the earlier diff.)
  • Adds cubescan_empty_group_by_tuple in rust/cubesql/cubesql/src/compile/test/test_cube_scan.rs asserting GROUP BY () with two measures, a filter, and a limit produces the expected V1LoadRequestQuery.
  • packages/cubejs-backend-native/Cargo.lock and rust/cubesql/Cargo.lock updated in lockstep — consistent.

Code quality

  • Test asserts the full V1LoadRequestQuery struct (via ..Default::default()) rather than picking fields — good for regression detection.
  • Doc comment explains the semantics of GROUP BY () succinctly. Reads well.
  • No security or performance surface; parser-only fix.

Testing coverage

  • Golden path (measures + WHERE + GROUP BY () + LIMIT) is covered.
  • Non-blocking follow-up ideas:
    • GROUP BY () without a WHERE clause, to confirm the empty grouping set alone routes to the aggregate path.
    • GROUP BY GROUPING SETS ((), (col)) if supported — or a negative test if not.
    • Mixed dimensions + measures under GROUP BY () to confirm projected dimensions still resolve.

Dependency bump risk

  • Scoped upstream change to Tuple([]) handling — low blast radius. Worth a quick skim of the 809d3763 diff before merge to confirm nothing unrelated rode along.

Nothing blocking.

• Branch: `cubesql/group-by-tuple`

@codecov

codecov Bot commented Jul 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.52%. Comparing base (69e8728) to head (70ac81a).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #11331   +/-   ##
=======================================
  Coverage   79.51%   79.52%           
=======================================
  Files         473      473           
  Lines       98023    98064   +41     
  Branches     3549     3549           
=======================================
+ Hits        77942    77981   +39     
- Misses      19572    19574    +2     
  Partials      509      509           
Flag Coverage Δ
cube-backend 59.52% <ø> (ø)
cubesql 83.87% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Alex Qyoun-ae <4062971+MazterQyou@users.noreply.github.com>
@MazterQyou
MazterQyou force-pushed the cubesql/group-by-tuple branch from fe501e4 to 70ac81a Compare July 23, 2026 08:54
@claude

claude Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MazterQyou's task in 27s —— View job


Code Review

Verdict: LGTM — small, well-scoped fix. Already approved by @ovr.

  • Re-read diff vs origin/master
  • Verified Cargo.toml rev + lockfile updates are consistent
  • Reviewed new test structure
Review details

Diff summary

  • Bumps cube-js/arrow-datafusion rev from d203d877ee20ad38 (note: rev changed since prior review, which referenced 809d3763) to pick up upstream Tuple([]) support in the SQL→relational planner.
  • Adds cubescan_empty_group_by_tuple in rust/cubesql/cubesql/src/compile/test/test_cube_scan.rs:229 asserting GROUP BY () + two measures + WHERE + LIMIT produces the expected V1LoadRequestQuery.
  • packages/cubejs-backend-native/Cargo.lock and rust/cubesql/Cargo.lock updated in lockstep — consistent.

Code quality

  • Test asserts the full V1LoadRequestQuery struct via ..Default::default() — good for regression detection.
  • Concise doc comment explains GROUP BY () semantics.
  • No security or performance surface; parser-only fix.

Testing

  • Golden path is covered. Non-blocking follow-up ideas (from prior reviews, still applicable):
    • GROUP BY () without a WHERE clause.
    • GROUP BY GROUPING SETS ((), (col)) — positive or negative test depending on expected support.
    • Mixed projected dimensions + measures under GROUP BY ().

Dependency bump risk

  • Rev moved from the one earlier reviews inspected — worth a quick skim of the d203d877..ee20ad38 diff on the fork to confirm only the Tuple([]) change (and any related test/parser tweaks) rode along.

Nothing blocking.

• Branch: `cubesql/group-by-tuple`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants