fix: update PrimitiveGroupValueBuilder to match NaN correctly in scalar equal_to#17979
Merged
rluvaton merged 1 commit intoapache:mainfrom Oct 9, 2025
Merged
Conversation
rluvaton
force-pushed
the
fix-bugs-in-primitive-group
branch
from
9076457 to
7dc8b9f
Compare
Contributor
|
🤖 |
alamb
reviewed
Oct 9, 2025
| #[test] | ||
| fn test_nullable_primitive_equal_to() { | ||
| let append = |builder: &mut PrimitiveGroupValueBuilder<Int64Type, true>, | ||
| let append = |builder: &mut PrimitiveGroupValueBuilder<Float32Type, true>, |
Member
Author
There was a problem hiding this comment.
I changed the helper method type to use float32 instead of int64 so all tests in the future that uses the helper will test that as well
| #[test] | ||
| fn test_nullable_primitive_vectorized_equal_to() { | ||
| let append = |builder: &mut PrimitiveGroupValueBuilder<Int64Type, true>, | ||
| let append = |builder: &mut PrimitiveGroupValueBuilder<Float32Type, true>, |
Contributor
There was a problem hiding this comment.
likewise it seems unecessary to change this test
Member
Author
There was a problem hiding this comment.
I changed the helper method type to use float32 instead of int64 so all tests in the future that uses the helper will test that as well
| fn test_nullable_primitive_equal_to_internal<A, E>(mut append: A, mut equal_to: E) | ||
| where | ||
| A: FnMut(&mut PrimitiveGroupValueBuilder<Int64Type, true>, &ArrayRef, &[usize]), | ||
| A: FnMut(&mut PrimitiveGroupValueBuilder<Float32Type, true>, &ArrayRef, &[usize]), |
Contributor
There was a problem hiding this comment.
I verified this test fails without the code change in this PR
assertion failed: equal_to_results[5]
thread 'aggregates::group_values::multi_group_by::primitive::tests::test_nullable_primitive_equal_to' panicked at datafusion/physical-plan/src/aggregates/group_values/multi_group_by/primitive.rs:346:9:
assertion failed: equal_to_results[5]
stack backtrace:
0: __rustc::rust_begin_unwind
at /rustc/1159e78c4747b02ef996e55082b704c09b970588/library/std/src/panicking.rs:697:5
1: core::panicking::panic_fmt
at /rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/panicking.rs:75:14
2: core::panicking::panic
at /rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/panicking.rs:145:5
3: datafusion_physical_plan::aggregates::group_values::multi_group_by::primitive::tests::test_nullable_primitive_equal_to_internal
at ./src/aggregates/group_values/multi_group_by/primitive.rs:346:9
4: datafusion_physical_plan::aggregates::group_values::multi_group_by::primitive::tests::test_nullable_primitive_equal_to
at ./src/aggregates/group_values/multi_group_by/primitive.rs:246:9
5: datafusion_physical_plan::aggregates::group_values::multi_group_by::primitive::tests::test_nullable_primitive_equal_to::{{closure}}
at ./src/aggregates/group_values/multi_group_by/primitive.rs:226:42
6: core::ops::function::FnOnce::call_once
at /Users/andrewlamb/.rustup/toolchains/1.90.0-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:253:5
7: core::ops::function::FnOnce::call_once
at /rustc/1159e78c4747b02ef996e55082b704c09b970588/library/core/src/ops/function.rs:253:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
alamb
approved these changes
Oct 9, 2025
Contributor
|
🤖: Benchmark completed Details
|
Member
Author
|
@alamb |
Member
Author
|
Merged as no degradation was found other than this: |
Contributor
yeah, I agree this is likely noise |
Contributor
will rerun to find out |
Contributor
|
🤖 |
Contributor
|
🤖: Benchmark completed Details
|
Member
Author
|
There are too much noise, making the benchmark results unreliable, can your script also print the machine you are working on (like also, is your machine a dedicated bare metal? |
Member
Author
|
is it possible to get apache to have bare metal machines in cloud for benchmarks? It would be great if we could have like in node.js and just allow to run the specific benchmarks on dedicated machines and it will make sure that all benchmarks across PR are running on the same machine with the same config making the benchmark result more reliable Example of how node.js does it: 
|
Contributor
It is not, it is a shared google VM
That would be sweet. I filed a ticket to track the idea: I am sorry it took me so long to respond, I wanted to give this a proper writeup |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
N/A
Rationale for this change
NaNare not considered equal in the scalarequal_toWhat changes are included in this PR?
Update instead of using
==tois_eqand update the tests to tests f32 instead of int64 with NaNAre these changes tested?
Yes
Are there any user-facing changes?
Just bug fix