kumarUjjawal commented on code in PR #26956:
URL: https://github.com/apache/datafusion/pull/26956#discussion_r4450799098
##########
datafusion/common/src/float_semantics.rs:
##########
@@ -274,6 +288,35 @@ mod tests {
assert!(Arc::ptr_eq(&normalize_floats(&array), &array));
}
+ #[test]
+ fn normalize_floats_finds_negative_zero_in_any_chunk() {
+ assert_finds_negative_zero_in_any_chunk::<Float16Type>(
+ f16::from_f32(1.5),
+ f16::NEG_ZERO,
+ );
+ assert_finds_negative_zero_in_any_chunk::<Float32Type>(1.5, -0.0);
+ assert_finds_negative_zero_in_any_chunk::<Float64Type>(1.5, -0.0);
+ }
+
+ /// Puts a `-0.0` at the ends of chunks and in a partial last chunk.
+ fn assert_finds_negative_zero_in_any_chunk<T>(value: T::Native, neg_zero:
T::Native)
+ where
+ T: ArrowPrimitiveType,
Review Comment:
The new chunk-boundary test only checks that the value at `pos` no longer
needs normalizing. It never checks that the rest of the array is unchanged, or
that `pos` became +0.0 rather than any other value.
##########
datafusion/common/src/float_semantics.rs:
##########
@@ -153,13 +153,25 @@ pub fn normalize_floats(array: &ArrayRef) -> ArrayRef {
}
}
+/// How many values [`normalize_floats`] checks at a time before it can stop.
+const NORMALIZATION_CHECK_CHUNK: usize = 1024;
+
fn normalize_primitive<T>(array: &ArrayRef) -> ArrayRef
where
T: ArrowPrimitiveType,
T::Native: FloatSemantics,
{
let arr = array.as_primitive::<T>();
- if !arr.values().iter().any(|v| v.needs_normalization()) {
+ // Not `any()` over the values: its early exit checks one value at a time.
+ // A fold over a chunk vectorizes, and stopping after the first chunk with
+ // a match keeps an early exit.
Review Comment:
Once a chunk with a match is found, all its position information is thrown
away and `unary` rescans and rewrites the whole array from row 0.
##########
datafusion/common/src/float_semantics.rs:
##########
@@ -153,13 +153,25 @@ pub fn normalize_floats(array: &ArrayRef) -> ArrayRef {
}
}
+/// How many values [`normalize_floats`] checks at a time before it can stop.
+const NORMALIZATION_CHECK_CHUNK: usize = 1024;
Review Comment:
NORMALIZATION_CHECK_CHUNK is a count of values, not bytes, so how far the
check reads before it can stop depends on the type: 2 KB for f16 but 8 KB for
f64.
##########
datafusion/common/src/float_semantics.rs:
##########
@@ -274,6 +288,35 @@ mod tests {
assert!(Arc::ptr_eq(&normalize_floats(&array), &array));
}
+ #[test]
+ fn normalize_floats_finds_negative_zero_in_any_chunk() {
+ assert_finds_negative_zero_in_any_chunk::<Float16Type>(
+ f16::from_f32(1.5),
+ f16::NEG_ZERO,
+ );
+ assert_finds_negative_zero_in_any_chunk::<Float32Type>(1.5, -0.0);
+ assert_finds_negative_zero_in_any_chunk::<Float64Type>(1.5, -0.0);
+ }
+
+ /// Puts a `-0.0` at the ends of chunks and in a partial last chunk.
+ fn assert_finds_negative_zero_in_any_chunk<T>(value: T::Native, neg_zero:
T::Native)
Review Comment:
The new test never uses a sliced array whose offset is not a multiple of the
chunk size, so it doesn't test that chunking is applied to the slice's view of
the values rather than the underlying buffer.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
