Skip to content

feat: add GroupColumn support for Duration in multi-column GROUP BY#23783

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

feat: add GroupColumn support for Duration in multi-column GROUP BY#23783
tohuya6 wants to merge 1 commit into
apache:mainfrom
tohuya6:feat-22715-duration-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. A Duration key triggers exactly that today, so an
otherwise-qualifying multi-column GROUP BY pays the row-encoding tax because of
one column.

Duration shares the i64 native representation already used by Timestamp,
so supporting it is a pure slot-in of the existing PrimitiveGroupValueBuilder
no new builder type and no new comparison/hash logic.

What changes are included in this PR?

  • Accept Duration(_) in group_column_supported_type (all four TimeUnits are
    valid Arrow types, unlike the restricted Time32/Time64 set).
  • Dispatch the four Duration*Type units in make_group_column.
  • Extend the group_column_supported_typemake_group_column consistency fuzz
    with all four Duration units.
  • Add a (Duration, Int32) group-count benchmark to benches/multi_group_by.rs.

Are these changes tested?

Yes.

  • New unit test test_group_values_column_duration: a Duration key stays on the
    GroupValuesColumn path, dedups equal durations (including nulls), and
    round-trips with the Duration output type preserved (not the bare i64).
  • The consistency fuzz now asserts every Duration unit routes through the
    dispatcher.
  • New single- and multi-column Duration GROUP BY coverage in aggregate.slt.

Are there any user-facing changes?

No API changes. GROUP BY queries with a Duration 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
`Duration` key dragged an otherwise-qualifying multi-column GROUP BY onto the
slow path.

`Duration` shares the `i64` native representation of `Timestamp`, so it reuses
the existing `PrimitiveGroupValueBuilder` with no new builder type:

- dispatch the four `Duration*Type` units in `make_group_column`
- accept `Duration(_)` in `group_column_supported_type` (all four units are
  valid Arrow types, unlike Time32/Time64)
- extend the `group_column_supported_type` <-> `make_group_column` consistency
  fuzz with the four Duration units
- add an end-to-end unit test (Duration GROUP BY dedups including nulls and
  preserves the Duration output type) and a Duration GROUP BY block
  (single- and multi-column keys) in aggregate.slt
- add a `(Duration, 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
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.

1 participant