From c7611a4ed110746611fa5460174c64abb7193f33 Mon Sep 17 00:00:00 2001 From: shah Date: Mon, 3 Aug 2026 20:50:15 +0200 Subject: [PATCH 1/5] fix: normalize signed zero in nested float array comparisons Arrow's nested comparator uses total order, where -0.0 and 0.0 are distinct, but Spark's ordering.equiv treats them as equal, while still treating NaN as equal to itself. Normalize negative zero in nested float leaves before building the comparator so arrays_overlap and array_position match Spark's semantics. Ref #5191 --- .../src/array_funcs/array_position.rs | 51 ++++- .../src/array_funcs/arrays_overlap.rs | 44 ++++- native/spark-expr/src/array_funcs/mod.rs | 1 + .../src/array_funcs/nested_float_normalize.rs | 183 ++++++++++++++++++ 4 files changed, 270 insertions(+), 9 deletions(-) create mode 100644 native/spark-expr/src/array_funcs/nested_float_normalize.rs diff --git a/native/spark-expr/src/array_funcs/array_position.rs b/native/spark-expr/src/array_funcs/array_position.rs index 191091aabf9..45618f72c72 100644 --- a/native/spark-expr/src/array_funcs/array_position.rs +++ b/native/spark-expr/src/array_funcs/array_position.rs @@ -33,6 +33,8 @@ use num::Float; use std::cmp::Ordering; use std::sync::Arc; +use super::nested_float_normalize::normalize_negative_zero; + /// Spark array_position() function that returns the 1-based position of an element in an array. /// Returns 0 if the element is not found (Spark behavior differs from DataFusion which returns null). fn spark_array_position(args: &[ColumnarValue]) -> Result { @@ -273,7 +275,13 @@ fn position_fallback( let num_rows = list_array.len(); let nulls = combined_nulls(list_array.nulls(), element.nulls()); let mut result = vec![0i64; num_rows]; - let comparator = make_comparator(values.as_ref(), element.as_ref(), SortOptions::default())?; + let values_normalized = normalize_negative_zero(values); + let element_normalized = normalize_negative_zero(element); + let comparator = make_comparator( + values_normalized.as_ref(), + element_normalized.as_ref(), + SortOptions::default(), + )?; for (row_index, w) in offsets.windows(2).enumerate() { if nulls.as_ref().is_some_and(|n| n.is_null(row_index)) { @@ -301,8 +309,6 @@ mod tests { #[test] fn test_nested_float_and_null_position() -> DataFusionResult<()> { - // Arrow and the previous ScalarValue fallback distinguish signed zeros, so the second - // row matches at position 2 rather than position 1. let values = ListArray::from_iter_primitive::([ Some(vec![Some(1.0)]), Some(vec![Some(f64::NAN)]), @@ -324,7 +330,44 @@ mod tests { let result = array_position_inner(&[Arc::new(array), Arc::new(element)])?; let result = result.as_any().downcast_ref::().unwrap(); - assert_eq!(result, &Int64Array::from(vec![2, 2, 1])); + assert_eq!(result, &Int64Array::from(vec![2, 1, 1])); + Ok(()) + } + + #[test] + fn test_struct_float_field_signed_zero_position() -> DataFusionResult<()> { + use arrow::array::{Float64Builder, StructBuilder}; + + let fields = vec![Arc::new(Field::new("a", DataType::Float64, true))]; + let mut values_builder = + StructBuilder::new(fields.clone(), vec![Box::new(Float64Builder::new())]); + for v in [-0.0, 1.0] { + values_builder + .field_builder::(0) + .unwrap() + .append_value(v); + values_builder.append(true); + } + let values = Arc::new(values_builder.finish()); + let array = ListArray::new( + Arc::new(Field::new("item", values.data_type().clone(), true)), + OffsetBuffer::new(vec![0, 2].into()), + values, + None, + ); + + let mut element_builder = StructBuilder::new(fields, vec![Box::new(Float64Builder::new())]); + element_builder + .field_builder::(0) + .unwrap() + .append_value(0.0); + element_builder.append(true); + let element = element_builder.finish(); + + let result = array_position_inner(&[Arc::new(array), Arc::new(element)])?; + let result = result.as_any().downcast_ref::().unwrap(); + // {-0.0} is the first element and now matches {0.0}, matching Spark. + assert_eq!(result, &Int64Array::from(vec![1])); Ok(()) } } diff --git a/native/spark-expr/src/array_funcs/arrays_overlap.rs b/native/spark-expr/src/array_funcs/arrays_overlap.rs index 4d74718f162..8721a1e330b 100644 --- a/native/spark-expr/src/array_funcs/arrays_overlap.rs +++ b/native/spark-expr/src/array_funcs/arrays_overlap.rs @@ -51,6 +51,8 @@ use std::hash::Hash; use std::ops::Range; use std::sync::Arc; +use super::nested_float_normalize::normalize_negative_zero; + #[derive(Debug, PartialEq, Eq, Hash)] pub struct SparkArraysOverlap { signature: Signature, @@ -436,9 +438,11 @@ fn arrays_overlap_list_generic( }; let comparator = if needs_comparator(probe.data_type()) { + let probe_normalized = normalize_negative_zero(probe); + let search_normalized = normalize_negative_zero(search); Some(make_comparator( - probe.as_ref(), - search.as_ref(), + probe_normalized.as_ref(), + search_normalized.as_ref(), SortOptions::default(), )?) } else { @@ -839,8 +843,7 @@ mod tests { #[test] fn test_nested_float_total_order() -> Result<()> { - // Preserve the existing Arrow total-order behavior: NaN matches itself, while signed - // zeros are distinct. + // NaN matches itself, and signed zeros are equal, matching Spark. let left = make_nested_float_list(&[&[f64::NAN]]); let right = make_nested_float_list(&[&[f64::NAN]]); let result = arrays_overlap_list::(&left, &right)?; @@ -851,7 +854,7 @@ mod tests { let right = make_nested_float_list(&[&[-0.0]]); let result = arrays_overlap_list::(&left, &right)?; let result = result.as_any().downcast_ref::().unwrap(); - assert!(!result.value(0)); + assert!(result.value(0)); Ok(()) } @@ -1038,6 +1041,37 @@ mod tests { Ok(()) } + /// Build a single-row ListArray of structs: List> + fn make_struct_float_list(elements: Vec>) -> ListArray { + let fields = vec![Arc::new(Field::new("a", DataType::Float64, true))]; + let struct_builder = + StructBuilder::new(fields.clone(), vec![Box::new(Float64Builder::new())]); + let mut list_builder = ListBuilder::new(struct_builder); + + for elem in &elements { + let sb = list_builder.values(); + sb.field_builder::(0) + .unwrap() + .append_option(*elem); + sb.append(true); + } + list_builder.append(true); + list_builder.finish() + } + + #[test] + fn test_struct_float_field_signed_zero_overlap() -> Result<()> { + // [{-0.0}] vs [{0.0}] => true, matching Spark + let left = make_struct_float_list(vec![Some(-0.0)]); + let right = make_struct_float_list(vec![Some(0.0)]); + + let result = arrays_overlap_list::(&left, &right)?; + let result = result.as_any().downcast_ref::().unwrap(); + assert!(result.is_valid(0)); + assert!(result.value(0)); + Ok(()) + } + #[test] fn test_struct_null_element() -> Result<()> { // [NULL] vs [{1,2}] => null (null outer element) diff --git a/native/spark-expr/src/array_funcs/mod.rs b/native/spark-expr/src/array_funcs/mod.rs index b8877a93dce..2e4df8b3171 100644 --- a/native/spark-expr/src/array_funcs/mod.rs +++ b/native/spark-expr/src/array_funcs/mod.rs @@ -23,6 +23,7 @@ mod arrays_zip; mod flatten; mod get_array_struct_fields; mod list_extract; +mod nested_float_normalize; mod sequence; mod size; diff --git a/native/spark-expr/src/array_funcs/nested_float_normalize.rs b/native/spark-expr/src/array_funcs/nested_float_normalize.rs new file mode 100644 index 00000000000..494a9251e52 --- /dev/null +++ b/native/spark-expr/src/array_funcs/nested_float_normalize.rs @@ -0,0 +1,183 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use arrow::array::{ + Array, ArrayRef, AsArray, FixedSizeListArray, Float32Array, Float64Array, LargeListArray, + ListArray, StructArray, +}; +use arrow::datatypes::DataType; +use std::sync::Arc; + +/// Recursively rebuilds nested arrays with `-0.0` normalized to `0.0` in any +/// Float32/Float64 leaves, leaving NaN untouched. +pub(super) fn normalize_negative_zero(array: &ArrayRef) -> ArrayRef { + match array.data_type() { + DataType::Float32 => { + let arr = array.as_primitive::(); + let normalized: Float32Array = arr + .iter() + .map(|v| v.map(|v| if v == 0.0 { 0.0f32 } else { v })) + .collect(); + Arc::new(normalized) + } + DataType::Float64 => { + let arr = array.as_primitive::(); + let normalized: Float64Array = arr + .iter() + .map(|v| v.map(|v| if v == 0.0 { 0.0f64 } else { v })) + .collect(); + Arc::new(normalized) + } + DataType::List(field) => { + let list = array.as_list::(); + let normalized_values = normalize_negative_zero(list.values()); + Arc::new(ListArray::new( + Arc::clone(field), + list.offsets().clone(), + normalized_values, + list.nulls().cloned(), + )) + } + DataType::LargeList(field) => { + let list = array.as_list::(); + let normalized_values = normalize_negative_zero(list.values()); + Arc::new(LargeListArray::new( + Arc::clone(field), + list.offsets().clone(), + normalized_values, + list.nulls().cloned(), + )) + } + DataType::FixedSizeList(field, size) => { + let list = array.as_fixed_size_list(); + let normalized_values = normalize_negative_zero(list.values()); + Arc::new(FixedSizeListArray::new( + Arc::clone(field), + *size, + normalized_values, + list.nulls().cloned(), + )) + } + DataType::Struct(_) => { + let s = array.as_struct(); + let normalized_columns: Vec = + s.columns().iter().map(normalize_negative_zero).collect(); + Arc::new(StructArray::new( + s.fields().clone(), + normalized_columns, + s.nulls().cloned(), + )) + } + _ => Arc::clone(array), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use arrow::array::Float64Builder; + use arrow::array::ListBuilder; + use arrow::datatypes::Field; + + #[test] + fn test_normalize_flat_floats() { + let arr: ArrayRef = Arc::new(Float64Array::from(vec![ + Some(-0.0), + Some(0.0), + Some(f64::NAN), + None, + Some(1.5), + ])); + let normalized = normalize_negative_zero(&arr); + let normalized = normalized.as_primitive::(); + + assert_eq!(normalized.value(0).to_bits(), 0.0f64.to_bits()); + assert_eq!(normalized.value(1).to_bits(), 0.0f64.to_bits()); + assert!(normalized.value(2).is_nan()); + assert!(normalized.is_null(3)); + assert_eq!(normalized.value(4), 1.5); + } + + #[test] + fn test_normalize_nested_list_floats() { + let mut builder = ListBuilder::new(Float64Builder::new()); + builder.values().append_value(-0.0); + builder.values().append_value(f64::NAN); + builder.append(true); + let arr: ArrayRef = Arc::new(builder.finish()); + + let normalized = normalize_negative_zero(&arr); + let normalized = normalized.as_list::(); + let inner = normalized.value(0); + let inner = inner.as_primitive::(); + + assert_eq!(inner.value(0).to_bits(), 0.0f64.to_bits()); + assert!(inner.value(1).is_nan()); + } + + #[test] + fn test_normalize_struct_floats() { + let a = Float64Array::from(vec![Some(-0.0), Some(1.0)]); + let b = Float64Array::from(vec![Some(f64::NAN), Some(-0.0)]); + let fields = vec![ + Arc::new(Field::new("a", DataType::Float64, true)), + Arc::new(Field::new("b", DataType::Float64, true)), + ]; + let arr: ArrayRef = Arc::new(StructArray::new( + fields.into(), + vec![Arc::new(a), Arc::new(b)], + None, + )); + + let normalized = normalize_negative_zero(&arr); + let normalized = normalized.as_struct(); + let col_a = normalized + .column(0) + .as_primitive::(); + let col_b = normalized + .column(1) + .as_primitive::(); + + assert_eq!(col_a.value(0).to_bits(), 0.0f64.to_bits()); + assert_eq!(col_a.value(1), 1.0); + assert!(col_b.value(0).is_nan()); + assert_eq!(col_b.value(1).to_bits(), 0.0f64.to_bits()); + } + + #[test] + fn test_normalize_fixed_size_list_floats() { + let values = Float64Array::from(vec![Some(-0.0), Some(f64::NAN), Some(1.0), Some(-0.0)]); + let field = Arc::new(Field::new("item", DataType::Float64, true)); + let arr: ArrayRef = Arc::new(FixedSizeListArray::new( + Arc::clone(&field), + 2, + Arc::new(values), + None, + )); + + let normalized = normalize_negative_zero(&arr); + let normalized = normalized.as_fixed_size_list(); + let flat = normalized + .values() + .as_primitive::(); + + assert_eq!(flat.value(0).to_bits(), 0.0f64.to_bits()); + assert!(flat.value(1).is_nan()); + assert_eq!(flat.value(2), 1.0); + assert_eq!(flat.value(3).to_bits(), 0.0f64.to_bits()); + } +} From 00a45a620493f38d0a77a601df03f068d939bfdc Mon Sep 17 00:00:00 2001 From: shah Date: Tue, 4 Aug 2026 09:56:09 +0200 Subject: [PATCH 2/5] fix: normalize floats once per column instead of per row The comparator was rebuilding the whole float buffer on every row, since list.value(i) only narrows offsets, not the values array. Made it quadratic. Hoist normalization above the loop and skip it entirely for types with no float leaf. Also switch to the existing normalize_float helper (already used in hll_plus_plus.rs) instead of a custom -0.0 only version, since it canonicalizes NaN too. Fixes a case where [[-NaN]] vs [[NaN]] was returning false. Addresses review on #5235. --- .../src/array_funcs/array_position.rs | 12 +- .../src/array_funcs/arrays_overlap.rs | 42 ++++++- .../src/array_funcs/nested_float_normalize.rs | 109 +++++++++++------- 3 files changed, 111 insertions(+), 52 deletions(-) diff --git a/native/spark-expr/src/array_funcs/array_position.rs b/native/spark-expr/src/array_funcs/array_position.rs index 45618f72c72..b2b54c40e79 100644 --- a/native/spark-expr/src/array_funcs/array_position.rs +++ b/native/spark-expr/src/array_funcs/array_position.rs @@ -33,7 +33,7 @@ use num::Float; use std::cmp::Ordering; use std::sync::Arc; -use super::nested_float_normalize::normalize_negative_zero; +use super::nested_float_normalize::{has_float_leaf, normalize_nested_floats}; /// Spark array_position() function that returns the 1-based position of an element in an array. /// Returns 0 if the element is not found (Spark behavior differs from DataFusion which returns null). @@ -275,11 +275,13 @@ fn position_fallback( let num_rows = list_array.len(); let nulls = combined_nulls(list_array.nulls(), element.nulls()); let mut result = vec![0i64; num_rows]; - let values_normalized = normalize_negative_zero(values); - let element_normalized = normalize_negative_zero(element); + let values_normalized = + has_float_leaf(values.data_type()).then(|| normalize_nested_floats(values)); + let element_normalized = + has_float_leaf(element.data_type()).then(|| normalize_nested_floats(element)); let comparator = make_comparator( - values_normalized.as_ref(), - element_normalized.as_ref(), + values_normalized.as_ref().unwrap_or(values).as_ref(), + element_normalized.as_ref().unwrap_or(element).as_ref(), SortOptions::default(), )?; diff --git a/native/spark-expr/src/array_funcs/arrays_overlap.rs b/native/spark-expr/src/array_funcs/arrays_overlap.rs index 8721a1e330b..c83ba0380c6 100644 --- a/native/spark-expr/src/array_funcs/arrays_overlap.rs +++ b/native/spark-expr/src/array_funcs/arrays_overlap.rs @@ -51,7 +51,7 @@ use std::hash::Hash; use std::ops::Range; use std::sync::Arc; -use super::nested_float_normalize::normalize_negative_zero; +use super::nested_float_normalize::{has_float_leaf, normalize_nested_floats}; #[derive(Debug, PartialEq, Eq, Hash)] pub struct SparkArraysOverlap { @@ -397,11 +397,34 @@ where } } +fn normalize_list_element_floats( + list: &GenericListArray, +) -> GenericListArray { + let field = match list.data_type() { + DataType::List(f) | DataType::LargeList(f) => Arc::clone(f), + _ => unreachable!("GenericListArray always has List or LargeList data type"), + }; + let normalized_values = normalize_nested_floats(list.values()); + GenericListArray::new( + field, + list.offsets().clone(), + normalized_values, + list.nulls().cloned(), + ) +} + /// Fallback for nested and otherwise unhandled element types. fn arrays_overlap_list_generic( left: &GenericListArray, right: &GenericListArray, ) -> Result { + let left_owned = + has_float_leaf(left.values().data_type()).then(|| normalize_list_element_floats(left)); + let left: &GenericListArray = left_owned.as_ref().unwrap_or(left); + let right_owned = + has_float_leaf(right.values().data_type()).then(|| normalize_list_element_floats(right)); + let right: &GenericListArray = right_owned.as_ref().unwrap_or(right); + let len = left.len(); let mut builder = BooleanArray::builder(len); @@ -438,11 +461,9 @@ fn arrays_overlap_list_generic( }; let comparator = if needs_comparator(probe.data_type()) { - let probe_normalized = normalize_negative_zero(probe); - let search_normalized = normalize_negative_zero(search); Some(make_comparator( - probe_normalized.as_ref(), - search_normalized.as_ref(), + probe.as_ref(), + search.as_ref(), SortOptions::default(), )?) } else { @@ -858,6 +879,17 @@ mod tests { Ok(()) } + #[test] + fn test_nested_float_signed_nan_total_order() -> Result<()> { + // [[-NaN]] vs [[NaN]] => true + let left = make_nested_float_list(&[&[-f64::NAN]]); + let right = make_nested_float_list(&[&[f64::NAN]]); + let result = arrays_overlap_list::(&left, &right)?; + let result = result.as_any().downcast_ref::().unwrap(); + assert!(result.value(0)); + Ok(()) + } + #[test] fn test_nested_array_basic_overlap() -> Result<()> { // [[1,2], [3,4]] vs [[3,4], [5,6]] => true diff --git a/native/spark-expr/src/array_funcs/nested_float_normalize.rs b/native/spark-expr/src/array_funcs/nested_float_normalize.rs index 494a9251e52..aa348acba35 100644 --- a/native/spark-expr/src/array_funcs/nested_float_normalize.rs +++ b/native/spark-expr/src/array_funcs/nested_float_normalize.rs @@ -15,36 +15,42 @@ // specific language governing permissions and limitations // under the License. +use crate::math_funcs::internal::normalize_float; use arrow::array::{ Array, ArrayRef, AsArray, FixedSizeListArray, Float32Array, Float64Array, LargeListArray, ListArray, StructArray, }; -use arrow::datatypes::DataType; +use arrow::datatypes::{DataType, Float32Type, Float64Type}; use std::sync::Arc; -/// Recursively rebuilds nested arrays with `-0.0` normalized to `0.0` in any -/// Float32/Float64 leaves, leaving NaN untouched. -pub(super) fn normalize_negative_zero(array: &ArrayRef) -> ArrayRef { +pub(super) fn has_float_leaf(dt: &DataType) -> bool { + match dt { + DataType::Float32 | DataType::Float64 => true, + DataType::List(field) | DataType::LargeList(field) | DataType::FixedSizeList(field, _) => { + has_float_leaf(field.data_type()) + } + DataType::Struct(fields) => fields.iter().any(|f| has_float_leaf(f.data_type())), + _ => false, + } +} + +/// Recursively rebuilds nested arrays with `-0.0` normalized to `0.0` and NaN canonicalized +/// in any Float32/Float64 leaves. +pub(super) fn normalize_nested_floats(array: &ArrayRef) -> ArrayRef { match array.data_type() { DataType::Float32 => { - let arr = array.as_primitive::(); - let normalized: Float32Array = arr - .iter() - .map(|v| v.map(|v| if v == 0.0 { 0.0f32 } else { v })) - .collect(); + let normalized: Float32Array = + array.as_primitive::().unary(normalize_float); Arc::new(normalized) } DataType::Float64 => { - let arr = array.as_primitive::(); - let normalized: Float64Array = arr - .iter() - .map(|v| v.map(|v| if v == 0.0 { 0.0f64 } else { v })) - .collect(); + let normalized: Float64Array = + array.as_primitive::().unary(normalize_float); Arc::new(normalized) } DataType::List(field) => { let list = array.as_list::(); - let normalized_values = normalize_negative_zero(list.values()); + let normalized_values = normalize_nested_floats(list.values()); Arc::new(ListArray::new( Arc::clone(field), list.offsets().clone(), @@ -54,7 +60,7 @@ pub(super) fn normalize_negative_zero(array: &ArrayRef) -> ArrayRef { } DataType::LargeList(field) => { let list = array.as_list::(); - let normalized_values = normalize_negative_zero(list.values()); + let normalized_values = normalize_nested_floats(list.values()); Arc::new(LargeListArray::new( Arc::clone(field), list.offsets().clone(), @@ -64,7 +70,7 @@ pub(super) fn normalize_negative_zero(array: &ArrayRef) -> ArrayRef { } DataType::FixedSizeList(field, size) => { let list = array.as_fixed_size_list(); - let normalized_values = normalize_negative_zero(list.values()); + let normalized_values = normalize_nested_floats(list.values()); Arc::new(FixedSizeListArray::new( Arc::clone(field), *size, @@ -75,7 +81,7 @@ pub(super) fn normalize_negative_zero(array: &ArrayRef) -> ArrayRef { DataType::Struct(_) => { let s = array.as_struct(); let normalized_columns: Vec = - s.columns().iter().map(normalize_negative_zero).collect(); + s.columns().iter().map(normalize_nested_floats).collect(); Arc::new(StructArray::new( s.fields().clone(), normalized_columns, @@ -93,46 +99,71 @@ mod tests { use arrow::array::ListBuilder; use arrow::datatypes::Field; + #[test] + fn test_has_float_leaf() { + assert!(has_float_leaf(&DataType::Float64)); + assert!(has_float_leaf(&DataType::List(Arc::new(Field::new( + "item", + DataType::Float32, + true + ))))); + assert!(has_float_leaf(&DataType::Struct( + vec![ + Arc::new(Field::new("a", DataType::Int32, true)), + Arc::new(Field::new("b", DataType::Float64, true)), + ] + .into() + ))); + assert!(!has_float_leaf(&DataType::Int32)); + assert!(!has_float_leaf(&DataType::List(Arc::new(Field::new( + "item", + DataType::Int32, + true + ))))); + } + #[test] fn test_normalize_flat_floats() { let arr: ArrayRef = Arc::new(Float64Array::from(vec![ Some(-0.0), Some(0.0), Some(f64::NAN), + Some(-f64::NAN), None, Some(1.5), ])); - let normalized = normalize_negative_zero(&arr); - let normalized = normalized.as_primitive::(); + let normalized = normalize_nested_floats(&arr); + let normalized = normalized.as_primitive::(); assert_eq!(normalized.value(0).to_bits(), 0.0f64.to_bits()); assert_eq!(normalized.value(1).to_bits(), 0.0f64.to_bits()); - assert!(normalized.value(2).is_nan()); - assert!(normalized.is_null(3)); - assert_eq!(normalized.value(4), 1.5); + assert_eq!(normalized.value(2).to_bits(), f64::NAN.to_bits()); + assert_eq!(normalized.value(3).to_bits(), f64::NAN.to_bits()); + assert!(normalized.is_null(4)); + assert_eq!(normalized.value(5), 1.5); } #[test] fn test_normalize_nested_list_floats() { let mut builder = ListBuilder::new(Float64Builder::new()); builder.values().append_value(-0.0); - builder.values().append_value(f64::NAN); + builder.values().append_value(-f64::NAN); builder.append(true); let arr: ArrayRef = Arc::new(builder.finish()); - let normalized = normalize_negative_zero(&arr); + let normalized = normalize_nested_floats(&arr); let normalized = normalized.as_list::(); let inner = normalized.value(0); - let inner = inner.as_primitive::(); + let inner = inner.as_primitive::(); assert_eq!(inner.value(0).to_bits(), 0.0f64.to_bits()); - assert!(inner.value(1).is_nan()); + assert_eq!(inner.value(1).to_bits(), f64::NAN.to_bits()); } #[test] fn test_normalize_struct_floats() { let a = Float64Array::from(vec![Some(-0.0), Some(1.0)]); - let b = Float64Array::from(vec![Some(f64::NAN), Some(-0.0)]); + let b = Float64Array::from(vec![Some(-f64::NAN), Some(-0.0)]); let fields = vec![ Arc::new(Field::new("a", DataType::Float64, true)), Arc::new(Field::new("b", DataType::Float64, true)), @@ -143,24 +174,20 @@ mod tests { None, )); - let normalized = normalize_negative_zero(&arr); + let normalized = normalize_nested_floats(&arr); let normalized = normalized.as_struct(); - let col_a = normalized - .column(0) - .as_primitive::(); - let col_b = normalized - .column(1) - .as_primitive::(); + let col_a = normalized.column(0).as_primitive::(); + let col_b = normalized.column(1).as_primitive::(); assert_eq!(col_a.value(0).to_bits(), 0.0f64.to_bits()); assert_eq!(col_a.value(1), 1.0); - assert!(col_b.value(0).is_nan()); + assert_eq!(col_b.value(0).to_bits(), f64::NAN.to_bits()); assert_eq!(col_b.value(1).to_bits(), 0.0f64.to_bits()); } #[test] fn test_normalize_fixed_size_list_floats() { - let values = Float64Array::from(vec![Some(-0.0), Some(f64::NAN), Some(1.0), Some(-0.0)]); + let values = Float64Array::from(vec![Some(-0.0), Some(-f64::NAN), Some(1.0), Some(-0.0)]); let field = Arc::new(Field::new("item", DataType::Float64, true)); let arr: ArrayRef = Arc::new(FixedSizeListArray::new( Arc::clone(&field), @@ -169,14 +196,12 @@ mod tests { None, )); - let normalized = normalize_negative_zero(&arr); + let normalized = normalize_nested_floats(&arr); let normalized = normalized.as_fixed_size_list(); - let flat = normalized - .values() - .as_primitive::(); + let flat = normalized.values().as_primitive::(); assert_eq!(flat.value(0).to_bits(), 0.0f64.to_bits()); - assert!(flat.value(1).is_nan()); + assert_eq!(flat.value(1).to_bits(), f64::NAN.to_bits()); assert_eq!(flat.value(2), 1.0); assert_eq!(flat.value(3).to_bits(), 0.0f64.to_bits()); } From 16c647bf3749e7aedb840ccacccccf152f65fdb3 Mon Sep 17 00:00:00 2001 From: shah Date: Wed, 5 Aug 2026 19:21:40 +0200 Subject: [PATCH 3/5] fix: address second round review feedback Fix the -0.0 literal in arrays_overlap.sql, it was coerced through decimal and testing nothing. Add the nested double array and struct fixtures for arrays_overlap.sql and array_position.sql. Also add a comment on arrays_overlap_list_generic noting the flat path must stay untouched, Spark's flat and nested paths disagree on signed zero, and a comment on the struct position test noting it is not reachable from SQL yet. --- .../src/array_funcs/array_position.rs | 3 ++ .../src/array_funcs/arrays_overlap.rs | 4 +++ .../expressions/array/array_position.sql | 17 ++++++++++ .../expressions/array/arrays_overlap.sql | 34 ++++++++++++++++++- 4 files changed, 57 insertions(+), 1 deletion(-) diff --git a/native/spark-expr/src/array_funcs/array_position.rs b/native/spark-expr/src/array_funcs/array_position.rs index b2b54c40e79..060501c5f5b 100644 --- a/native/spark-expr/src/array_funcs/array_position.rs +++ b/native/spark-expr/src/array_funcs/array_position.rs @@ -336,6 +336,9 @@ mod tests { Ok(()) } + // array_position over array> currently falls back to Spark + // (ArraysBase.isTypeSupported rejects StructType, see #1307), so this only + // exercises position_fallback directly and isn't reachable from a SQL query. #[test] fn test_struct_float_field_signed_zero_position() -> DataFusionResult<()> { use arrow::array::{Float64Builder, StructBuilder}; diff --git a/native/spark-expr/src/array_funcs/arrays_overlap.rs b/native/spark-expr/src/array_funcs/arrays_overlap.rs index c83ba0380c6..463503c7d71 100644 --- a/native/spark-expr/src/array_funcs/arrays_overlap.rs +++ b/native/spark-expr/src/array_funcs/arrays_overlap.rs @@ -414,6 +414,10 @@ fn normalize_list_element_floats( } /// Fallback for nested and otherwise unhandled element types. +/// +/// note: Spark's flat arrays_overlap (HashSet) treats -0.0 and 0.0 as different, +/// only the nested path here treats them as equal. this normalization can't move into the +/// flat fast path in arrays_overlap_list without breaking that difference. fn arrays_overlap_list_generic( left: &GenericListArray, right: &GenericListArray, diff --git a/spark/src/test/resources/sql-tests/expressions/array/array_position.sql b/spark/src/test/resources/sql-tests/expressions/array/array_position.sql index 132158ab6d7..33c31d84d8e 100644 --- a/spark/src/test/resources/sql-tests/expressions/array/array_position.sql +++ b/spark/src/test/resources/sql-tests/expressions/array/array_position.sql @@ -254,6 +254,23 @@ INSERT INTO test_ap_nested_str VALUES query SELECT array_position(arr, val) FROM test_ap_nested_str +-- nested double array column: -0.0 and 0.0 are equal under Spark's nested ordering +statement +CREATE TABLE test_ap_nested_dbl(arr array>, val array) USING parquet + +statement +INSERT INTO test_ap_nested_dbl VALUES + (array(array(1.0), array(double('-0.0'))), array(double('0.0'))), + (array(array(double('-0.0')), array(1.0)), array(double('0.0'))), + (array(array(double('0.0')), array(1.0)), array(double('-0.0'))), + (array(array(double('NaN'))), array(double('NaN'))), + (array(array(1.0)), array(2.0)), + (NULL, array(double('0.0'))), + (array(array(double('0.0'))), NULL) + +query +SELECT array_position(arr, val) FROM test_ap_nested_dbl + -- timestamp arrays statement CREATE TABLE test_ap_ts(arr array, val timestamp) USING parquet diff --git a/spark/src/test/resources/sql-tests/expressions/array/arrays_overlap.sql b/spark/src/test/resources/sql-tests/expressions/array/arrays_overlap.sql index b7859a1092c..1ddfe14dbcd 100644 --- a/spark/src/test/resources/sql-tests/expressions/array/arrays_overlap.sql +++ b/spark/src/test/resources/sql-tests/expressions/array/arrays_overlap.sql @@ -117,7 +117,7 @@ statement CREATE TABLE test_overlap_dbl(a array, b array) USING parquet statement -INSERT INTO test_overlap_dbl VALUES (array(1.0, 2.0), array(2.0, 3.0)), (array(1.0, double('NaN')), array(double('NaN'), 2.0)), (array(double('Infinity'), 1.0), array(double('Infinity'))), (array(double('-Infinity')), array(double('Infinity'))), (array(0.0), array(double('-0.0'))), (array(1.0, NULL), array(2.0, NULL)) +INSERT INTO test_overlap_dbl VALUES (array(1.0, 2.0), array(2.0, 3.0)), (array(1.0, double('NaN')), array(double('NaN'), 2.0)), (array(double('Infinity'), 1.0), array(double('Infinity'))), (array(double('-Infinity')), array(double('Infinity'))), (array(double('0.0')), array(double('-0.0'))), (array(1.0, NULL), array(2.0, NULL)) query SELECT a, b, arrays_overlap(a, b) FROM test_overlap_dbl @@ -233,6 +233,38 @@ INSERT INTO test_overlap_nested VALUES (array(array(1, 2), array(3, 4)), array(a query SELECT a, b, arrays_overlap(a, b) FROM test_overlap_nested +-- nested double arrays: Spark's nested path uses ordering.equiv, where -0.0 == 0.0 +statement +CREATE TABLE test_overlap_nested_dbl(a array>, b array>) USING parquet + +statement +INSERT INTO test_overlap_nested_dbl VALUES + (array(array(double('-0.0'))), array(array(double('0.0')))), + (array(array(double('0.0'))), array(array(double('-0.0')))), + (array(array(1.0, double('-0.0'))), array(array(1.0, 0.0))), + (array(array(double('NaN'))), array(array(double('NaN')))), + (array(array(1.0)), array(array(2.0))), + (array(array(double('-0.0')), cast(NULL as array)), array(array(double('0.0')))), + (array(cast(NULL as array)), array(array(double('0.0')))) + +query +SELECT a, b, arrays_overlap(a, b) FROM test_overlap_nested_dbl + +-- struct element with a double field +statement +CREATE TABLE test_overlap_struct_dbl(a array>, b array>) USING parquet + +statement +INSERT INTO test_overlap_struct_dbl VALUES + (array(named_struct('x', double('-0.0'))), array(named_struct('x', double('0.0')))), + (array(named_struct('x', double('0.0'))), array(named_struct('x', double('-0.0')))), + (array(named_struct('x', double('NaN'))), array(named_struct('x', double('NaN')))), + (array(named_struct('x', 1.0)), array(named_struct('x', 2.0))), + (array(cast(NULL as struct)), array(named_struct('x', double('0.0')))) + +query +SELECT a, b, arrays_overlap(a, b) FROM test_overlap_struct_dbl + -- struct element arrays statement CREATE TABLE test_overlap_struct(a array>, b array>) USING parquet From e98a8f9c2eb61a596b1cf40bd096e769358b91a6 Mon Sep 17 00:00:00 2001 From: shah Date: Fri, 7 Aug 2026 01:37:25 +0200 Subject: [PATCH 4/5] test: rename total_order tests to spark_equality These tests assert Spark's equality semantics (-0.0 == 0.0, -NaN == NaN), which is the opposite of Arrow's total order. Rename so a future arrow-rs upgrade doesn't get "fixed" back to total order by mistake. --- native/spark-expr/src/array_funcs/arrays_overlap.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/native/spark-expr/src/array_funcs/arrays_overlap.rs b/native/spark-expr/src/array_funcs/arrays_overlap.rs index 463503c7d71..a33556fe1f9 100644 --- a/native/spark-expr/src/array_funcs/arrays_overlap.rs +++ b/native/spark-expr/src/array_funcs/arrays_overlap.rs @@ -867,7 +867,7 @@ mod tests { } #[test] - fn test_nested_float_total_order() -> Result<()> { + fn test_nested_float_spark_equality() -> Result<()> { // NaN matches itself, and signed zeros are equal, matching Spark. let left = make_nested_float_list(&[&[f64::NAN]]); let right = make_nested_float_list(&[&[f64::NAN]]); @@ -884,7 +884,7 @@ mod tests { } #[test] - fn test_nested_float_signed_nan_total_order() -> Result<()> { + fn test_nested_float_signed_nan_spark_equality() -> Result<()> { // [[-NaN]] vs [[NaN]] => true let left = make_nested_float_list(&[&[-f64::NAN]]); let right = make_nested_float_list(&[&[f64::NAN]]); From 90dc9a36ecdc967a4521a71cf7bfc096a6259704 Mon Sep 17 00:00:00 2001 From: shah Date: Thu, 17 Sep 2026 23:16:51 +0200 Subject: [PATCH 5/5] fix: guard nested float normalization at every recursion level Once an enclosing struct contained a float, normalize_nested_floats rebuilt every child regardless of whether that child held any float data. An empty struct child then reached StructArray::new, which cannot infer a length from zero columns and panics. Iceberg exposes _partition as struct<> on an unpartitioned table, so arrays_overlap over a struct element carrying that column failed with a native exception where Spark returns a result. Apply the has_float_leaf guard inside normalize_nested_floats rather than only at the call sites, so float-free subtrees are returned unchanged. This fixes the panic and keeps the rebuild proportional to the float data. --- .../src/array_funcs/arrays_overlap.rs | 41 +++++++++- .../src/array_funcs/nested_float_normalize.rs | 78 ++++++++++++++++++- 2 files changed, 116 insertions(+), 3 deletions(-) diff --git a/native/spark-expr/src/array_funcs/arrays_overlap.rs b/native/spark-expr/src/array_funcs/arrays_overlap.rs index a33556fe1f9..542687c7d16 100644 --- a/native/spark-expr/src/array_funcs/arrays_overlap.rs +++ b/native/spark-expr/src/array_funcs/arrays_overlap.rs @@ -546,10 +546,11 @@ fn needs_comparator(dt: &DataType) -> bool { mod tests { use super::*; use arrow::array::{ - Float64Builder, Int32Array, Int32Builder, ListArray, ListBuilder, StructBuilder, + Float64Array, Float64Builder, Int32Array, Int32Builder, ListArray, ListBuilder, + StructArray, StructBuilder, }; use arrow::buffer::{NullBuffer, OffsetBuffer}; - use arrow::datatypes::Field; + use arrow::datatypes::{Field, Fields}; fn make_list_array( values: &Int32Array, @@ -1095,6 +1096,42 @@ mod tests { list_builder.finish() } + /// Regression test for an empty struct alongside a float field. Iceberg exposes + /// `_partition` as `struct<>` on an unpartitioned table, so + /// `arrays_overlap(array(named_struct('x', x, 'p', _partition)), ...)` reaches the + /// nested path with a zero column struct child. Rebuilding that child used to panic. + #[test] + fn test_struct_with_empty_struct_field_overlap() -> Result<()> { + fn make_list(values: Vec) -> ListArray { + let len = values.len(); + let x: ArrayRef = Arc::new(Float64Array::from(values)); + let partition: ArrayRef = Arc::new(StructArray::new_empty_fields(len, None)); + let fields: Fields = vec![ + Arc::new(Field::new("x", DataType::Float64, true)), + Arc::new(Field::new("p", partition.data_type().clone(), true)), + ] + .into(); + let element: ArrayRef = + Arc::new(StructArray::new(fields.clone(), vec![x, partition], None)); + ListArray::new( + Arc::new(Field::new("item", DataType::Struct(fields), true)), + OffsetBuffer::new((0..=len as i32).collect::>().into()), + element, + None, + ) + } + + // One row per element, mirroring `SELECT ... FROM t` over values 1.0 and 2.0. + let left = make_list(vec![1.0, 2.0]); + let right = make_list(vec![1.0, 2.0]); + + let result = arrays_overlap_list::(&left, &right)?; + let result = result.as_any().downcast_ref::().unwrap(); + assert!(result.value(0)); + assert!(result.value(1)); + Ok(()) + } + #[test] fn test_struct_float_field_signed_zero_overlap() -> Result<()> { // [{-0.0}] vs [{0.0}] => true, matching Spark diff --git a/native/spark-expr/src/array_funcs/nested_float_normalize.rs b/native/spark-expr/src/array_funcs/nested_float_normalize.rs index aa348acba35..f70c5ca7f06 100644 --- a/native/spark-expr/src/array_funcs/nested_float_normalize.rs +++ b/native/spark-expr/src/array_funcs/nested_float_normalize.rs @@ -36,7 +36,17 @@ pub(super) fn has_float_leaf(dt: &DataType) -> bool { /// Recursively rebuilds nested arrays with `-0.0` normalized to `0.0` and NaN canonicalized /// in any Float32/Float64 leaves. +/// +/// Subtrees without a float leaf are returned as is. The guard is applied at every level, not +/// just by the caller, so that a float-free sibling of a float field is never rebuilt. That +/// keeps the rebuild proportional to the float data, and it also leaves empty structs alone: +/// `StructArray::new` cannot infer a length from zero columns and would panic. An empty struct +/// reaches this code through Iceberg's `_partition` metadata column on an unpartitioned table. pub(super) fn normalize_nested_floats(array: &ArrayRef) -> ArrayRef { + if !has_float_leaf(array.data_type()) { + return Arc::clone(array); + } + match array.data_type() { DataType::Float32 => { let normalized: Float32Array = @@ -96,8 +106,9 @@ pub(super) fn normalize_nested_floats(array: &ArrayRef) -> ArrayRef { mod tests { use super::*; use arrow::array::Float64Builder; + use arrow::array::Int32Array; use arrow::array::ListBuilder; - use arrow::datatypes::Field; + use arrow::datatypes::{Field, Fields}; #[test] fn test_has_float_leaf() { @@ -160,6 +171,71 @@ mod tests { assert_eq!(inner.value(1).to_bits(), f64::NAN.to_bits()); } + /// An empty struct sibling of a float field must survive normalization. Iceberg exposes + /// `_partition` as `struct<>` on an unpartitioned table, and rebuilding it would panic + /// because `StructArray::new` cannot infer a length from zero columns. + #[test] + fn test_normalize_struct_with_empty_struct_sibling() { + let x = Float64Array::from(vec![Some(-0.0), Some(1.0)]); + let partition = StructArray::new_empty_fields(2, None); + let fields = vec![ + Arc::new(Field::new("x", DataType::Float64, true)), + Arc::new(Field::new("p", partition.data_type().clone(), true)), + ]; + let arr: ArrayRef = Arc::new(StructArray::new( + fields.into(), + vec![Arc::new(x), Arc::new(partition)], + None, + )); + + let normalized = normalize_nested_floats(&arr); + let normalized = normalized.as_struct(); + + let col_x = normalized.column(0).as_primitive::(); + assert_eq!(col_x.value(0).to_bits(), 0.0f64.to_bits()); + assert_eq!(col_x.value(1), 1.0); + + let col_p = normalized.column(1).as_struct(); + assert_eq!(col_p.num_columns(), 0); + assert_eq!(col_p.len(), 2); + } + + /// A float-free subtree is returned as is rather than rebuilt. The child here is a nested + /// struct, which the recursive arms would otherwise rebuild into a fresh array. + #[test] + fn test_normalize_leaves_float_free_subtree_untouched() { + let inner_fields: Fields = vec![Arc::new(Field::new("i", DataType::Int32, true))].into(); + let inner: ArrayRef = Arc::new(StructArray::new( + inner_fields.clone(), + vec![Arc::new(Int32Array::from(vec![Some(1), Some(2)]))], + None, + )); + let floats = Float64Array::from(vec![Some(-0.0), Some(1.0)]); + let fields: Fields = vec![ + Arc::new(Field::new("s", DataType::Struct(inner_fields), true)), + Arc::new(Field::new("f", DataType::Float64, true)), + ] + .into(); + let arr: ArrayRef = Arc::new(StructArray::new( + fields, + vec![Arc::clone(&inner), Arc::new(floats)], + None, + )); + + let normalized = normalize_nested_floats(&arr); + let normalized = normalized.as_struct(); + + assert!(Arc::ptr_eq(normalized.column(0), &inner)); + assert_eq!( + normalized + .column(1) + .as_primitive::() + .value(0) + .to_bits(), + 0.0f64.to_bits() + ); + } + #[test] fn test_normalize_struct_floats() { let a = Float64Array::from(vec![Some(-0.0), Some(1.0)]);