You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "rluvaton (via GitHub)" <gi...@apache.org> on 2026/09/29 12:24:17 UTC
[PR] chore: remove unreachable code [datafusion]
rluvaton opened a new pull request, #25872:
URL: https://github.com/apache/datafusion/pull/25872
## Which issue does this PR close?
None
## Rationale for this change
`downcast_primitive` macro already handle all the temporal types, so no need to match them again manually
## What changes are included in this PR?
removed temporal types matching for creating group values
## What is the testing strategy for this PR?
existing tests
## Are there any user-facing changes?
no
--
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] chore: remove unreachable code and cleanup data type matching [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #25872:
URL: https://github.com/apache/datafusion/pull/25872#discussion_r4135216944
##########
datafusion/functions-aggregate/src/min_max.rs:
##########
@@ -283,76 +274,10 @@ impl AggregateUDFImpl for Max {
args: AccumulatorArgs,
) -> Result<Box<dyn GroupsAccumulator>> {
use DataType::*;
- use TimeUnit::*;
let data_type = args.return_field.data_type();
- match data_type {
- Int8 => primitive_max_accumulator!(data_type, i8, Int8Type),
- Int16 => primitive_max_accumulator!(data_type, i16, Int16Type),
- Int32 => primitive_max_accumulator!(data_type, i32, Int32Type),
- Int64 => primitive_max_accumulator!(data_type, i64, Int64Type),
- UInt8 => primitive_max_accumulator!(data_type, u8, UInt8Type),
- UInt16 => primitive_max_accumulator!(data_type, u16, UInt16Type),
- UInt32 => primitive_max_accumulator!(data_type, u32, UInt32Type),
- UInt64 => primitive_max_accumulator!(data_type, u64, UInt64Type),
- Float16 => {
- primitive_max_accumulator!(data_type, f16, Float16Type, total, u16)
- }
- Float32 => {
- primitive_max_accumulator!(data_type, f32, Float32Type, total, u32)
- }
- Float64 => {
- primitive_max_accumulator!(data_type, f64, Float64Type, total, u64)
- }
- Date32 => primitive_max_accumulator!(data_type, i32, Date32Type),
- Date64 => primitive_max_accumulator!(data_type, i64, Date64Type),
- Time32(Second) => {
- primitive_max_accumulator!(data_type, i32, Time32SecondType)
- }
- Time32(Millisecond) => {
- primitive_max_accumulator!(data_type, i32, Time32MillisecondType)
- }
- Time64(Microsecond) => {
- primitive_max_accumulator!(data_type, i64, Time64MicrosecondType)
- }
- Time64(Nanosecond) => {
- primitive_max_accumulator!(data_type, i64, Time64NanosecondType)
- }
- Timestamp(Second, _) => {
- primitive_max_accumulator!(data_type, i64, TimestampSecondType)
- }
- Timestamp(Millisecond, _) => {
- primitive_max_accumulator!(data_type, i64, TimestampMillisecondType)
- }
- Timestamp(Microsecond, _) => {
- primitive_max_accumulator!(data_type, i64, TimestampMicrosecondType)
- }
- Timestamp(Nanosecond, _) => {
- primitive_max_accumulator!(data_type, i64, TimestampNanosecondType)
- }
- Duration(Second) => {
- primitive_max_accumulator!(data_type, i64, DurationSecondType)
- }
- Duration(Millisecond) => {
- primitive_max_accumulator!(data_type, i64, DurationMillisecondType)
- }
- Duration(Microsecond) => {
- primitive_max_accumulator!(data_type, i64, DurationMicrosecondType)
- }
- Duration(Nanosecond) => {
- primitive_max_accumulator!(data_type, i64, DurationNanosecondType)
- }
- Decimal32(_, _) => {
- primitive_max_accumulator!(data_type, i32, Decimal32Type)
- }
- Decimal64(_, _) => {
- primitive_max_accumulator!(data_type, i64, Decimal64Type)
- }
- Decimal128(_, _) => {
- primitive_max_accumulator!(data_type, i128, Decimal128Type)
- }
- Decimal256(_, _) => {
- primitive_max_accumulator!(data_type, i256, Decimal256Type)
- }
+
+ downcast_primitive! {
Review Comment:
point of interest here is this now supports interval, though its not reachable since interval isnt listed in `groups_accumulator_supported`
not sure if this is intended or not
--
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] chore: remove unreachable code and cleanup data type matching + support interval in min/max groups acc [datafusion]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton merged PR #25872:
URL: https://github.com/apache/datafusion/pull/25872
--
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] chore: remove unreachable code and cleanup data type matching [datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #25872:
URL: https://github.com/apache/datafusion/pull/25872#discussion_r4135062786
##########
datafusion/functions-aggregate/src/min_max.rs:
##########
@@ -283,76 +274,10 @@ impl AggregateUDFImpl for Max {
args: AccumulatorArgs,
) -> Result<Box<dyn GroupsAccumulator>> {
use DataType::*;
- use TimeUnit::*;
let data_type = args.return_field.data_type();
- match data_type {
- Int8 => primitive_max_accumulator!(data_type, i8, Int8Type),
- Int16 => primitive_max_accumulator!(data_type, i16, Int16Type),
- Int32 => primitive_max_accumulator!(data_type, i32, Int32Type),
- Int64 => primitive_max_accumulator!(data_type, i64, Int64Type),
- UInt8 => primitive_max_accumulator!(data_type, u8, UInt8Type),
- UInt16 => primitive_max_accumulator!(data_type, u16, UInt16Type),
- UInt32 => primitive_max_accumulator!(data_type, u32, UInt32Type),
- UInt64 => primitive_max_accumulator!(data_type, u64, UInt64Type),
- Float16 => {
- primitive_max_accumulator!(data_type, f16, Float16Type, total, u16)
- }
- Float32 => {
- primitive_max_accumulator!(data_type, f32, Float32Type, total, u32)
- }
- Float64 => {
- primitive_max_accumulator!(data_type, f64, Float64Type, total, u64)
- }
- Date32 => primitive_max_accumulator!(data_type, i32, Date32Type),
- Date64 => primitive_max_accumulator!(data_type, i64, Date64Type),
- Time32(Second) => {
- primitive_max_accumulator!(data_type, i32, Time32SecondType)
- }
- Time32(Millisecond) => {
- primitive_max_accumulator!(data_type, i32, Time32MillisecondType)
- }
- Time64(Microsecond) => {
- primitive_max_accumulator!(data_type, i64, Time64MicrosecondType)
- }
- Time64(Nanosecond) => {
- primitive_max_accumulator!(data_type, i64, Time64NanosecondType)
- }
- Timestamp(Second, _) => {
- primitive_max_accumulator!(data_type, i64, TimestampSecondType)
- }
- Timestamp(Millisecond, _) => {
- primitive_max_accumulator!(data_type, i64, TimestampMillisecondType)
- }
- Timestamp(Microsecond, _) => {
- primitive_max_accumulator!(data_type, i64, TimestampMicrosecondType)
- }
- Timestamp(Nanosecond, _) => {
- primitive_max_accumulator!(data_type, i64, TimestampNanosecondType)
- }
- Duration(Second) => {
- primitive_max_accumulator!(data_type, i64, DurationSecondType)
- }
- Duration(Millisecond) => {
- primitive_max_accumulator!(data_type, i64, DurationMillisecondType)
- }
- Duration(Microsecond) => {
- primitive_max_accumulator!(data_type, i64, DurationMicrosecondType)
- }
- Duration(Nanosecond) => {
- primitive_max_accumulator!(data_type, i64, DurationNanosecondType)
- }
- Decimal32(_, _) => {
- primitive_max_accumulator!(data_type, i32, Decimal32Type)
- }
- Decimal64(_, _) => {
- primitive_max_accumulator!(data_type, i64, Decimal64Type)
- }
- Decimal128(_, _) => {
- primitive_max_accumulator!(data_type, i128, Decimal128Type)
- }
- Decimal256(_, _) => {
- primitive_max_accumulator!(data_type, i256, Decimal256Type)
- }
+
+ downcast_primitive! {
+ data_type => (primitive_max_accumulator, data_type),
Review Comment:
Preserve the float-specific branches in both factories
This dispatch replaces `total_cmp` and its sentinels with `partial_cmp` and finite bounds. Through the actual Float32 factories, MIN([+Infinity]) returns `f32::MAX` and MAX([-Infinity]) returns `f32::MIN`; NaN and signed-zero cases also regress. Retain the float-specific branches and test via the factories—the existing test calls the `total` helper directly.
--
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] chore: remove unreachable code and cleanup data type matching [datafusion]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on code in PR #25872:
URL: https://github.com/apache/datafusion/pull/25872#discussion_r4135588440
##########
datafusion/functions-aggregate/src/min_max.rs:
##########
@@ -283,76 +274,10 @@ impl AggregateUDFImpl for Max {
args: AccumulatorArgs,
) -> Result<Box<dyn GroupsAccumulator>> {
use DataType::*;
- use TimeUnit::*;
let data_type = args.return_field.data_type();
- match data_type {
- Int8 => primitive_max_accumulator!(data_type, i8, Int8Type),
- Int16 => primitive_max_accumulator!(data_type, i16, Int16Type),
- Int32 => primitive_max_accumulator!(data_type, i32, Int32Type),
- Int64 => primitive_max_accumulator!(data_type, i64, Int64Type),
- UInt8 => primitive_max_accumulator!(data_type, u8, UInt8Type),
- UInt16 => primitive_max_accumulator!(data_type, u16, UInt16Type),
- UInt32 => primitive_max_accumulator!(data_type, u32, UInt32Type),
- UInt64 => primitive_max_accumulator!(data_type, u64, UInt64Type),
- Float16 => {
- primitive_max_accumulator!(data_type, f16, Float16Type, total, u16)
- }
- Float32 => {
- primitive_max_accumulator!(data_type, f32, Float32Type, total, u32)
- }
- Float64 => {
- primitive_max_accumulator!(data_type, f64, Float64Type, total, u64)
- }
- Date32 => primitive_max_accumulator!(data_type, i32, Date32Type),
- Date64 => primitive_max_accumulator!(data_type, i64, Date64Type),
- Time32(Second) => {
- primitive_max_accumulator!(data_type, i32, Time32SecondType)
- }
- Time32(Millisecond) => {
- primitive_max_accumulator!(data_type, i32, Time32MillisecondType)
- }
- Time64(Microsecond) => {
- primitive_max_accumulator!(data_type, i64, Time64MicrosecondType)
- }
- Time64(Nanosecond) => {
- primitive_max_accumulator!(data_type, i64, Time64NanosecondType)
- }
- Timestamp(Second, _) => {
- primitive_max_accumulator!(data_type, i64, TimestampSecondType)
- }
- Timestamp(Millisecond, _) => {
- primitive_max_accumulator!(data_type, i64, TimestampMillisecondType)
- }
- Timestamp(Microsecond, _) => {
- primitive_max_accumulator!(data_type, i64, TimestampMicrosecondType)
- }
- Timestamp(Nanosecond, _) => {
- primitive_max_accumulator!(data_type, i64, TimestampNanosecondType)
- }
- Duration(Second) => {
- primitive_max_accumulator!(data_type, i64, DurationSecondType)
- }
- Duration(Millisecond) => {
- primitive_max_accumulator!(data_type, i64, DurationMillisecondType)
- }
- Duration(Microsecond) => {
- primitive_max_accumulator!(data_type, i64, DurationMicrosecondType)
- }
- Duration(Nanosecond) => {
- primitive_max_accumulator!(data_type, i64, DurationNanosecondType)
- }
- Decimal32(_, _) => {
- primitive_max_accumulator!(data_type, i32, Decimal32Type)
- }
- Decimal64(_, _) => {
- primitive_max_accumulator!(data_type, i64, Decimal64Type)
- }
- Decimal128(_, _) => {
- primitive_max_accumulator!(data_type, i128, Decimal128Type)
- }
- Decimal256(_, _) => {
- primitive_max_accumulator!(data_type, i256, Decimal256Type)
- }
+
+ downcast_primitive! {
+ data_type => (primitive_max_accumulator, data_type),
Review Comment:
great catch!
--
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] chore: remove unreachable code and cleanup data type matching [datafusion]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on code in PR #25872:
URL: https://github.com/apache/datafusion/pull/25872#discussion_r4135721323
##########
datafusion/functions-aggregate/src/min_max.rs:
##########
@@ -283,76 +274,10 @@ impl AggregateUDFImpl for Max {
args: AccumulatorArgs,
) -> Result<Box<dyn GroupsAccumulator>> {
use DataType::*;
- use TimeUnit::*;
let data_type = args.return_field.data_type();
- match data_type {
- Int8 => primitive_max_accumulator!(data_type, i8, Int8Type),
- Int16 => primitive_max_accumulator!(data_type, i16, Int16Type),
- Int32 => primitive_max_accumulator!(data_type, i32, Int32Type),
- Int64 => primitive_max_accumulator!(data_type, i64, Int64Type),
- UInt8 => primitive_max_accumulator!(data_type, u8, UInt8Type),
- UInt16 => primitive_max_accumulator!(data_type, u16, UInt16Type),
- UInt32 => primitive_max_accumulator!(data_type, u32, UInt32Type),
- UInt64 => primitive_max_accumulator!(data_type, u64, UInt64Type),
- Float16 => {
- primitive_max_accumulator!(data_type, f16, Float16Type, total, u16)
- }
- Float32 => {
- primitive_max_accumulator!(data_type, f32, Float32Type, total, u32)
- }
- Float64 => {
- primitive_max_accumulator!(data_type, f64, Float64Type, total, u64)
- }
- Date32 => primitive_max_accumulator!(data_type, i32, Date32Type),
- Date64 => primitive_max_accumulator!(data_type, i64, Date64Type),
- Time32(Second) => {
- primitive_max_accumulator!(data_type, i32, Time32SecondType)
- }
- Time32(Millisecond) => {
- primitive_max_accumulator!(data_type, i32, Time32MillisecondType)
- }
- Time64(Microsecond) => {
- primitive_max_accumulator!(data_type, i64, Time64MicrosecondType)
- }
- Time64(Nanosecond) => {
- primitive_max_accumulator!(data_type, i64, Time64NanosecondType)
- }
- Timestamp(Second, _) => {
- primitive_max_accumulator!(data_type, i64, TimestampSecondType)
- }
- Timestamp(Millisecond, _) => {
- primitive_max_accumulator!(data_type, i64, TimestampMillisecondType)
- }
- Timestamp(Microsecond, _) => {
- primitive_max_accumulator!(data_type, i64, TimestampMicrosecondType)
- }
- Timestamp(Nanosecond, _) => {
- primitive_max_accumulator!(data_type, i64, TimestampNanosecondType)
- }
- Duration(Second) => {
- primitive_max_accumulator!(data_type, i64, DurationSecondType)
- }
- Duration(Millisecond) => {
- primitive_max_accumulator!(data_type, i64, DurationMillisecondType)
- }
- Duration(Microsecond) => {
- primitive_max_accumulator!(data_type, i64, DurationMicrosecondType)
- }
- Duration(Nanosecond) => {
- primitive_max_accumulator!(data_type, i64, DurationNanosecondType)
- }
- Decimal32(_, _) => {
- primitive_max_accumulator!(data_type, i32, Decimal32Type)
- }
- Decimal64(_, _) => {
- primitive_max_accumulator!(data_type, i64, Decimal64Type)
- }
- Decimal128(_, _) => {
- primitive_max_accumulator!(data_type, i128, Decimal128Type)
- }
- Decimal256(_, _) => {
- primitive_max_accumulator!(data_type, i256, Decimal256Type)
- }
+
+ downcast_primitive! {
Review Comment:
was not indended, so marked as supported and added test
--
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] chore: remove unreachable code and cleanup data type matching [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25872:
URL: https://github.com/apache/datafusion/pull/25872#issuecomment-5892001475
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25872?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 `83.33333%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.58%. Comparing base ([`dbf62c1`](https://app.codecov.io/gh/apache/datafusion/commit/dbf62c131bfe54195fd41a683a6dec245a58555e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c2ca474`](https://app.codecov.io/gh/apache/datafusion/commit/c2ca474a351dc8e23497a1f30e30fca221b83732?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 5 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25872?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/functions-aggregate/src/min\_max.rs](https://app.codecov.io/gh/apache/datafusion/pull/25872?src=pr&el=tree&filepath=datafusion%2Ffunctions-aggregate%2Fsrc%2Fmin_max.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9mdW5jdGlvbnMtYWdncmVnYXRlL3NyYy9taW5fbWF4LnJz) | 80.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25872?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 #25872 +/- ##
==========================================
- Coverage 82.58% 82.58% -0.01%
==========================================
Files 1142 1142
Lines 440884 440818 -66
Branches 440884 440818 -66
==========================================
- Hits 364088 364029 -59
+ Misses 54799 54783 -16
- Partials 21997 22006 +9
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25872?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