You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "tohuya6 (via GitHub)" <gi...@apache.org> on 2026/07/22 01:12:52 UTC
[PR] feat: add GroupColumn support for Duration in multi-column GROUP BY [datafusion]
tohuya6 opened a new pull request, #23783:
URL: https://github.com/apache/datafusion/pull/23783
## Which issue does this PR close?
- Part of #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 `TimeUnit`s 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` 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add GroupColumn support for Duration in multi-column GROUP BY [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #23783:
URL: https://github.com/apache/datafusion/pull/23783#issuecomment-5102416203
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/23783?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `92.10526%` with `3 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 80.71%. Comparing base ([`5de7f1d`](https://app.codecov.io/gh/apache/datafusion/commit/5de7f1db95191f81ce6472361785db8f63ac2db1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`9f0bb3e`](https://app.codecov.io/gh/apache/datafusion/commit/9f0bb3eba1e07626b6fc448575c70ef1c5c97cab?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 85 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/23783?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../src/aggregates/group\_values/multi\_group\_by/mod.rs](https://app.codecov.io/gh/apache/datafusion/pull/23783?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Faggregates%2Fgroup_values%2Fmulti_group_by%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9hZ2dyZWdhdGVzL2dyb3VwX3ZhbHVlcy9tdWx0aV9ncm91cF9ieS9tb2QucnM=) | 92.10% | [0 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/23783?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #23783 +/- ##
==========================================
- Coverage 80.71% 80.71% -0.01%
==========================================
Files 1089 1089
Lines 368760 368798 +38
Branches 368760 368798 +38
==========================================
+ Hits 297647 297667 +20
- Misses 53372 53382 +10
- Partials 17741 17749 +8
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/23783?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add GroupColumn support for Duration in multi-column GROUP BY [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew merged PR #23783:
URL: https://github.com/apache/datafusion/pull/23783
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add GroupColumn support for Duration in multi-column GROUP BY [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #23783:
URL: https://github.com/apache/datafusion/pull/23783#issuecomment-5102218005
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/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>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-physical-plan v54.1.0 (current)
Built [ 46.425s] (current)
Parsing datafusion-physical-plan v54.1.0 (current)
Parsed [ 0.148s] (current)
Building datafusion-physical-plan v54.1.0 (baseline)
Built [ 41.637s] (baseline)
Parsing datafusion-physical-plan v54.1.0 (baseline)
Parsed [ 0.148s] (baseline)
Checking datafusion-physical-plan v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.637s] 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:
WindowAggExec::try_from_proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/windows/window_agg_exec.rs:367
AsyncFuncExec::try_from_proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/async_func.rs:290
GroupOrderingPartial::reset, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/aggregates/order/partial.rs:191
EmptyExec::try_from_proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/empty.rs:215
ExplainExec::try_from_proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/explain.rs:217
CrossJoinExec::try_from_proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/joins/cross_join.rs:485
HashJoinExec::try_from_proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/joins/hash_join/exec.rs:1857
NestedLoopJoinExec::try_from_proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/joins/nested_loop_join.rs:810
AggregateExec::try_from_proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/aggregates/mod.rs:2349
SymmetricHashJoinExec::try_from_proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/joins/symmetric_hash_join.rs:749
GroupOrderingFull::reset, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/aggregates/order/full.rs:119
GlobalLimitExec::try_from_proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/limit.rs:274
LocalLimitExec::try_from_proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/limit.rs:498
PlaceholderRowExec::try_from_proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/placeholder_row.rs:216
AnalyzeExec::try_from_proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/analyze.rs:348
RepartitionExec::try_from_proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/repartition/mod.rs:1785
SortExec::try_from_proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/sorts/sort.rs:1606
SortPreservingMergeExec::try_from_proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/sorts/sort_preserving_merge.rs:479
UnionExec::try_from_proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/union.rs:495
InterleaveExec::try_from_proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/union.rs:741
UnnestExec::try_from_proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/unnest.rs:346
GroupOrdering::reset, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/14a066cea32bc8ad22b51dd42877d798ed1e0a3d/datafusion/physical-plan/src/aggregates/order/mod.rs:102
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 90.828s] datafusion-physical-plan
Building datafusion-sqllogictest v54.1.0 (current)
Built [ 216.843s] (current)
Parsing datafusion-sqllogictest v54.1.0 (current)
Parsed [ 0.023s] (current)
Building datafusion-sqllogictest v54.1.0 (baseline)
Built [ 193.861s] (baseline)
Parsing datafusion-sqllogictest v54.1.0 (baseline)
Parsed [ 0.023s] (baseline)
Checking datafusion-sqllogictest v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.083s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 413.642s] datafusion-sqllogictest
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add GroupColumn support for Duration in multi-column GROUP BY [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #23783:
URL: https://github.com/apache/datafusion/pull/23783#discussion_r3664157139
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs:
##########
@@ -1352,6 +1377,56 @@ mod tests {
}
}
+ /// End-to-end coverage for `Duration` group keys: a `Duration` column stays
+ /// on the `GroupValuesColumn` fast path, deduplicates equal durations
+ /// (including nulls), and round-trips with its `Duration` output type
+ /// preserved (not the bare `i64` native). The four units share one builder,
+ /// so a single unit exercises the dispatcher; the fuzz above covers routing
+ /// for every unit.
+ #[test]
+ fn test_group_values_column_duration() {
+ use arrow::datatypes::TimeUnit;
+
+ let schema = Arc::new(Schema::new(vec![Field::new(
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add GroupColumn support for Duration in multi-column GROUP BY [datafusion]
Posted by "tohuya6 (via GitHub)" <gi...@apache.org>.
tohuya6 commented on code in PR #23783:
URL: https://github.com/apache/datafusion/pull/23783#discussion_r3683587436
##########
datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs:
##########
@@ -1352,6 +1377,56 @@ mod tests {
}
}
+ /// End-to-end coverage for `Duration` group keys: a `Duration` column stays
+ /// on the `GroupValuesColumn` fast path, deduplicates equal durations
+ /// (including nulls), and round-trips with its `Duration` output type
+ /// preserved (not the bare `i64` native). The four units share one builder,
+ /// so a single unit exercises the dispatcher; the fuzz above covers routing
+ /// for every unit.
+ #[test]
+ fn test_group_values_column_duration() {
+ use arrow::datatypes::TimeUnit;
+
+ let schema = Arc::new(Schema::new(vec![Field::new(
Review Comment:
appreciate your review kosiew
updated test to use Duration(Microsecond), Int64, it should be more direct now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org