Skip to content

Commit cda4dd6

Browse files
alexyclaudeadriangb
authored
perf: compare nested ScalarValues by type before converting to ArrayData (#26066)
## Which issue does this PR close? - Closes #26065. ## Rationale for this change Physical planning slows down, faster than the query grows, when projections carry many typed NULL struct literals: a `UNION ALL` of 35 branches with 35 struct columns of 20 fields takes 862 ms in `create_physical_plan` on `main`. `EquivalenceGroup::add_constant` compares each new constant with every existing class, and `ScalarValue::eq` compares nested values with arrow's array equality, which converts both arrays to `ArrayData` before it compares anything, even when the two values have different types. ## What changes are included in this PR? `ScalarValue::eq` compares the nested variants (`List`, `LargeList`, `FixedSizeList`, `ListView`, `LargeListView`, `Struct`, `Map`) through a helper that returns early for the same `Arc`, and compares lengths and data types before arrow's array equality. Results are unchanged; only values of the same type and length reach the `ArrayData` comparison. With the reproducer from the issue (release build, Apple M1 Max), `create_physical_plan`: | struct columns x fields | main | this PR | |---|---:|---:| | 10 x 20 | 31 ms | 17 ms | | 20 x 20 | 179 ms | 47 ms | | 35 x 20 | 862 ms | 147 ms | | 35 x 40 | 1,766 ms | 253 ms | ## What is the testing strategy for this PR? `test_nested_scalar_eq` covers typed NULL structs of the same and of different types, struct values that are equal and unequal, and lists of different element types. The scalar tests in `datafusion-common` pass. ## Are there any user-facing changes? No. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
1 parent c280a4b commit cda4dd6

1 file changed

Lines changed: 63 additions & 7 deletions

File tree

  • datafusion/common/src/scalar

‎datafusion/common/src/scalar/mod.rs‎

Lines changed: 63 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -481,6 +481,19 @@ impl Hash for Fl<f16> {
481481
}
482482
}
483483

484+
/// Equality of the arrays inside nested scalars.
485+
///
486+
/// Arrow's array equality converts both arrays to `ArrayData` before it
487+
/// compares anything, which is costly for wide nested types. Values of
488+
/// different lengths or types are unequal without that, and an array is
489+
/// equal to itself.
490+
fn nested_eq<T: Array + PartialEq>(a: &Arc<T>, b: &Arc<T>) -> bool {
491+
Arc::ptr_eq(a, b)
492+
|| (a.len() == b.len()
493+
&& a.data_type() == b.data_type()
494+
&& a.as_ref() == b.as_ref())
495+
}
496+
484497
// manual implementation of `PartialEq`
485498
impl PartialEq for ScalarValue {
486499
fn eq(&self, other: &Self) -> bool {
@@ -552,19 +565,19 @@ impl PartialEq for ScalarValue {
552565
(FixedSizeBinary(_, _), _) => false,
553566
(LargeBinary(v1), LargeBinary(v2)) => v1.eq(v2),
554567
(LargeBinary(_), _) => false,
555-
(FixedSizeList(v1), FixedSizeList(v2)) => v1.eq(v2),
568+
(FixedSizeList(v1), FixedSizeList(v2)) => nested_eq(v1, v2),
556569
(FixedSizeList(_), _) => false,
557-
(List(v1), List(v2)) => v1.eq(v2),
570+
(List(v1), List(v2)) => nested_eq(v1, v2),
558571
(List(_), _) => false,
559-
(LargeList(v1), LargeList(v2)) => v1.eq(v2),
572+
(LargeList(v1), LargeList(v2)) => nested_eq(v1, v2),
560573
(LargeList(_), _) => false,
561-
(ListView(v1), ListView(v2)) => v1.eq(v2),
574+
(ListView(v1), ListView(v2)) => nested_eq(v1, v2),
562575
(ListView(_), _) => false,
563-
(LargeListView(v1), LargeListView(v2)) => v1.eq(v2),
576+
(LargeListView(v1), LargeListView(v2)) => nested_eq(v1, v2),
564577
(LargeListView(_), _) => false,
565-
(Struct(v1), Struct(v2)) => v1.eq(v2),
578+
(Struct(v1), Struct(v2)) => nested_eq(v1, v2),
566579
(Struct(_), _) => false,
567-
(Map(v1), Map(v2)) => v1.eq(v2),
580+
(Map(v1), Map(v2)) => nested_eq(v1, v2),
568581
(Map(_), _) => false,
569582
(Date32(v1), Date32(v2)) => v1.eq(v2),
570583
(Date32(_), _) => false,
@@ -9929,6 +9942,49 @@ mod tests {
99299942
}
99309943
}
99319944

9945+
#[test]
9946+
fn test_nested_scalar_eq() {
9947+
let struct_type = |name: &str| {
9948+
DataType::Struct(Fields::from(vec![
9949+
Field::new(format!("{name}_a"), DataType::Int32, true),
9950+
Field::new(format!("{name}_b"), DataType::Utf8, true),
9951+
]))
9952+
};
9953+
let null_x = ScalarValue::try_from(&struct_type("x")).unwrap();
9954+
let null_x_again = ScalarValue::try_from(&struct_type("x")).unwrap();
9955+
let null_y = ScalarValue::try_from(&struct_type("y")).unwrap();
9956+
assert_eq!(null_x, null_x.clone());
9957+
assert_eq!(null_x, null_x_again);
9958+
assert_ne!(null_x, null_y);
9959+
9960+
let value = |a: i32| {
9961+
ScalarValue::Struct(Arc::new(StructArray::from(vec![
9962+
(
9963+
Arc::new(Field::new("x_a", DataType::Int32, true)),
9964+
Arc::new(Int32Array::from(vec![Some(a)])) as ArrayRef,
9965+
),
9966+
(
9967+
Arc::new(Field::new("x_b", DataType::Utf8, true)),
9968+
Arc::new(StringArray::from(vec![Some("b")])) as ArrayRef,
9969+
),
9970+
])))
9971+
};
9972+
assert_eq!(value(1), value(1));
9973+
assert_ne!(value(1), value(2));
9974+
assert_ne!(value(1), null_x);
9975+
9976+
let list_i32 = ScalarValue::List(ScalarValue::new_list_nullable(
9977+
&[ScalarValue::Int32(Some(1))],
9978+
&DataType::Int32,
9979+
));
9980+
let list_i64 = ScalarValue::List(ScalarValue::new_list_nullable(
9981+
&[ScalarValue::Int64(Some(1))],
9982+
&DataType::Int64,
9983+
));
9984+
assert_eq!(list_i32, list_i32.clone());
9985+
assert_ne!(list_i32, list_i64);
9986+
}
9987+
99329988
#[test]
99339989
fn test_struct_nulls() {
99349990
let fields_b = Fields::from(vec![

0 commit comments

Comments
 (0)