feat: add GroupColumn support for Duration in multi-column GROUP BY by tohuya6 · Pull Request #23783 · apache/datafusion

tohuya6 · 2026-07-22T01:12:48Z

Which issue does this PR close?

Part of EPIC: complete GroupValuesColumn type coverage (nested types + remaining primitives) #22715.

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_type ↔ make_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(Microsecond), Int64) key stays on the GroupValuesColumn path,
dedups on the composite key (including the (null, null) pair), 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 group_by.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

kosiew

@tohuya6
Thanks for the fix! I didn't find any blocking issues. I have one small suggestion that could make the regression test align even more closely with the scenario being fixed.

kosiew · 2026-07-28T08:56:19Z

+    fn test_group_values_column_duration() {
+        use arrow::datatypes::TimeUnit;
+
+        let schema = Arc::new(Schema::new(vec![Field::new(


Nice addition to cover Duration group keys. One small thought: this regression is specifically about the multi-column all-or-nothing gate, but this unit test exercises a single-column Duration GroupValuesColumn. The SQL logic test already covers the multi-column behaviour end to end, but it might be worth making this unit test use (Duration(Microsecond), Int64) input, or adding a small second unit test that does. That would exercise the GroupValuesColumn helper at the same boundary as the fix and make the regression a little more direct.

appreciate your review kosiew

updated test to use Duration(Microsecond), Int64, it should be more direct now

github-actions · 2026-07-28T09:14:50Z

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details

     Cloning apache/main
    Building datafusion-physical-plan v54.1.0 (current)
       Built [  44.889s] (current)
     Parsing datafusion-physical-plan v54.1.0 (current)
      Parsed [   0.152s] (current)
    Building datafusion-physical-plan v54.1.0 (baseline)
       Built [  38.519s] (baseline)
     Parsing datafusion-physical-plan v54.1.0 (baseline)
      Parsed [   0.153s] (baseline)
    Checking datafusion-physical-plan v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.944s] 223 checks: 222 pass, 1 fail, 0 warn, 30 skip

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/inherent_method_missing.ron

Failed in:
  ExecutionPlanDecodeCtx::expr_ctx, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/0b0f5244e930748b51242fd8a282cef534e7c632/datafusion/physical-plan/src/proto.rs:352
  ExecutionPlanEncodeCtx::expr_ctx, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/0b0f5244e930748b51242fd8a282cef534e7c632/datafusion/physical-plan/src/proto.rs:227

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  86.142s] datafusion-physical-plan
    Building datafusion-sqllogictest v54.1.0 (current)
       Built [ 196.571s] (current)
     Parsing datafusion-sqllogictest v54.1.0 (current)
      Parsed [   0.025s] (current)
    Building datafusion-sqllogictest v54.1.0 (baseline)
       Built [ 194.801s] (baseline)
     Parsing datafusion-sqllogictest v54.1.0 (baseline)
      Parsed [   0.025s] (baseline)
    Checking datafusion-sqllogictest v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.114s] 223 checks: 223 pass, 30 skip
     Summary no semver update required
    Finished [ 395.447s] datafusion-sqllogictest

codecov-commenter · 2026-07-28T09:34:29Z

Codecov Report

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

Files with missing lines	Patch %	Lines
.../src/aggregates/group_values/multi_group_by/mod.rs	93.75%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #23783      +/-   ##
==========================================
+ Coverage   80.71%   80.85%   +0.14%     
==========================================
  Files        1089     1099      +10     
  Lines      368760   374350    +5590     
  Branches   368760   374350    +5590     
==========================================
+ Hits       297647   302684    +5037     
- Misses      53372    53605     +233     
- Partials    17741    18061     +320

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

…he GroupColumn unit test Per kosiew's review: a single-column Duration test doesn't exercise the all-or-nothing multi-column gate this PR is actually about. Switching to (Duration(Microsecond), Int64) does — rows 3 and 4 now repeat row 0 and the (null, null) pair across both columns, so dedup is proven on the composite key rather than the Duration value alone. Also renumber the bench to Experiment 9 (float16 apache#23785 takes 8) and trim the slt/test comments to match the sibling PRs.

The 30-days case duplicated the slt, which already proves 1 month and 30 days don't fold. Dropping it leaves the assertions unique to the unit level: null keys dedup, the Int32 key splits equal intervals, and emit gives back Interval rather than the raw native. Also renumber the bench to Experiment 10 (float16 apache#23785 takes 8, duration apache#23783 takes 9), trim comments to match the sibling PRs, and normalize the blank lines after the new slt block to two, matching upstream.

Duration apache#23783 landed on main and touched the same regions as this PR. Kept both sides: - mod.rs: unioned the arrow imports, kept both unit tests, Float16 added to the supported arms - benches: Float16 stays Experiment 8, Duration keeps 9 - group_by.slt: both coverage blocks

Duration apache#23783 landed on main and touched the same regions as this PR. Kept both sides: - mod.rs: unioned the arrow imports, kept both unit tests, Interval added to the supported arms and the dispatcher - benches: Duration keeps Experiment 9, Interval stays 10 - group_by.slt: both coverage blocks

github-actions Bot added sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Jul 22, 2026

tohuya6 mentioned this pull request Jul 23, 2026

EPIC: complete GroupValuesColumn type coverage (nested types + remaining primitives) #22715

Open

7 tasks

kosiew approved these changes Jul 28, 2026

View reviewed changes

github-actions Bot added the auto detected api change Auto detected API change label Jul 28, 2026

tohuya6 added 2 commits July 30, 2026 17:32

test: move Duration GROUP BY coverage to group_by.slt

de9a92b

kosiew added this pull request to the merge queue Jul 31, 2026

Merged via the queue into apache:main with commit aa9b7bb Jul 31, 2026
41 checks passed

tohuya6 deleted the feat-22715-duration-group-column branch August 1, 2026 10:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

feat: add GroupColumn support for Duration in multi-column GROUP BY#23783
kosiew merged 3 commits into
apache:mainfrom
tohuya6:feat-22715-duration-group-column

tohuya6 commented Jul 22, 2026 •

edited

Loading

Uh oh!

kosiew left a comment •

edited

Loading

Uh oh!

kosiew Jul 28, 2026

Uh oh!

tohuya6 Jul 30, 2026

Uh oh!

github-actions Bot commented Jul 28, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented Jul 28, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

tohuya6 commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

kosiew left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kosiew Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

tohuya6 Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tohuya6 commented Jul 22, 2026 •

edited

Loading

kosiew left a comment •

edited

Loading

github-actions Bot commented Jul 28, 2026 •

edited

Loading

codecov-commenter commented Jul 28, 2026 •

edited

Loading