Skip to content

feat: add GroupColumn support for Interval in multi-column GROUP BY#23786

Open
tohuya6 wants to merge 1 commit into
apache:mainfrom
tohuya6:feat-22715-interval-group-column
Open

feat: add GroupColumn support for Interval in multi-column GROUP BY#23786
tohuya6 wants to merge 1 commit into
apache:mainfrom
tohuya6:feat-22715-interval-group-column

Conversation

@tohuya6

@tohuya6 tohuya6 commented Jul 22, 2026

Copy link
Copy Markdown

Which issue does this PR close?

Rationale for this change

multi_group_by::group_column_supported_type gates which GROUP BY columns may
use the column-wise GroupValuesColumn fast path, and the gate is
all-or-nothing: a single unsupported column forces the entire grouping onto
the byte-encoded GroupValuesRows fallback, even when every other key column
would have qualified. An Interval key triggers exactly that today.

All three Interval units reuse the existing PrimitiveGroupValueBuilder with
no new builder type and no new HashValue impl — the i32 / IntervalDayTime
/ IntervalMonthDayNano natives already implement it.

What changes are included in this PR?

  • Dispatch the three Interval*Type units in make_group_column. IntervalUnit
    has exactly three variants, so the match is exhaustive with no fallback arm
    (unlike Time32/Time64, which must reject invalid unit combinations).
  • Accept Interval(_) in group_column_supported_type.
  • Extend the group_column_supported_typemake_group_column consistency
    fuzz with the three Interval units.
  • Add an (Interval, Int32) group-count benchmark to benches/multi_group_by.rs.

Are these changes tested?

Yes.

  • New unit test: an Interval key stays on the GroupValuesColumn path, dedups
    including nulls, keeps "1 month" and "30 days" as distinct groups (no
    cross-unit folding), and round-trips with the Interval output type preserved.
  • The consistency fuzz now asserts all three Interval units route through the
    dispatcher.
  • New single- and multi-column Interval GROUP BY coverage in aggregate.slt.

Are there any user-facing changes?

No API changes. GROUP BY queries with an Interval key now use the
column-wise fast path instead of the row-encoded fallback; results are unchanged.

…lumn GROUP BY

`multi_group_by::group_column_supported_type` gates which GROUP BY columns can
use the column-wise `GroupValuesColumn` fast path. Any unsupported column forces
the entire grouping onto the byte-encoded `GroupValuesRows` fallback, so a single
`Interval` key dragged an otherwise-qualifying multi-column GROUP BY onto the
slow path.

All three `Interval` units reuse the existing `PrimitiveGroupValueBuilder` with
no new builder type and no new `HashValue` impl (the `i32` / `IntervalDayTime` /
`IntervalMonthDayNano` natives already implement it):

- dispatch the three `Interval*Type` units in `make_group_column`; `IntervalUnit`
  has exactly three variants, so the match is exhaustive with no fallback arm
  (unlike Time32 / Time64, which must reject invalid unit combinations)
- accept `Interval(_)` in `group_column_supported_type`
- extend the `group_column_supported_type` <-> `make_group_column` consistency
  fuzz with the three Interval units
- add an end-to-end unit test (Interval GROUP BY dedups including nulls, keeps
  "1 month" and "30 days" as distinct groups with no cross-unit folding, and
  preserves the Interval output type) and an Interval GROUP BY block (single-
  and multi-column keys) in aggregate.slt
- add an `(Interval, Int32)` group-count benchmark to `benches/multi_group_by.rs`

Part of apache#22715
@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Jul 22, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.73684% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.71%. Comparing base (5de7f1d) to head (b9753c4).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
.../src/aggregates/group_values/multi_group_by/mod.rs 94.73% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #23786      +/-   ##
==========================================
- Coverage   80.71%   80.71%   -0.01%     
==========================================
  Files        1089     1089              
  Lines      368760   368798      +38     
  Branches   368760   368798      +38     
==========================================
+ Hits       297647   297666      +19     
- Misses      53372    53382      +10     
- Partials    17741    17750       +9     

☔ 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.

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

Labels

physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants