feat: convert_array_to_scalar_vec respects null elements #17891

vegarsti · 2025-10-02T19:50:50Z

Which issue does this PR close?

Closes ScalarValue::convert_array_to_scalar_vec doesn't respect null list elements #17749.

What changes are included in this PR?

Makes convert_array_to_scalar_vec return None instead of the empty list when list elements are null.

Are these changes tested?

Covered by existing tests.

Are there any user-facing changes?

I don't think so?

vegarsti · 2025-10-02T20:32:05Z

datafusion/functions-aggregate/src/array_agg.rs

+            } else {
+                partition_values.push(vec![].into());
+            }


I'm not sure about this one! With the other changes, the group_by slt tests fail without this change. It is because in merge_ordered_arrays we require that partition_values and partition_ordering_values have the same length (see code snippet below). But that seems weird since we now only do partition_ordering_values.push(ordering_value) if the element in orderings is Some.

datafusion/datafusion/functions-aggregate-common/src/merge_arrays.rs

Lines 125 to 133 in 71512e6

if values.len() != ordering_values.len()

|| values

.iter()

.zip(ordering_values.iter())

.any(|(vals, ordering_vals)| vals.len() != ordering_vals.len())

{

return exec_err!(

"Expects values arguments and/or ordering_values arguments to have same size"

);

There's also an exact same change done in nth_values.rs

vegarsti · 2025-10-02T20:33:26Z

datafusion/functions-aggregate-common/src/merge_arrays.rs

-            .iter()
-            .zip(ordering_values.iter())
-            .any(|(vals, ordering_vals)| vals.len() != ordering_vals.len())
+    if values.len() != ordering_values.len() {


I don't exactly feel strongly about these changes, but since these two conditions are different I liked the error message telling me which was the case.

comphead

Thanks @vegarsti I would love to see if there any performance degradations, you can find benches in the project.

Maybe we can have a separate test for this issues?

vegarsti · 2025-10-03T20:30:19Z

Thanks @vegarsti I would love to see if there any performance degradations, you can find benches in the project.

Maybe we can have a separate test for this issues?

Good call, I will do both. Thanks!

vegarsti · 2025-10-04T06:21:46Z

Benchmark results cc @comphead

Comparing main and convert-array-to-scalar-vec-optional
--------------------
Benchmark tpch_sf1.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query        ┃     main ┃ convert-array-to-scalar-vec-optional ┃        Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 1     │ 72.27 ms │                             73.81 ms │     no change │
│ QQuery 2     │ 21.19 ms │                             21.89 ms │     no change │
│ QQuery 3     │ 33.45 ms │                             32.10 ms │     no change │
│ QQuery 4     │ 24.40 ms │                             23.64 ms │     no change │
│ QQuery 5     │ 51.61 ms │                             50.10 ms │     no change │
│ QQuery 6     │ 21.93 ms │                             21.73 ms │     no change │
│ QQuery 7     │ 71.15 ms │                             71.24 ms │     no change │
│ QQuery 8     │ 47.88 ms │                             45.30 ms │ +1.06x faster │
│ QQuery 9     │ 63.38 ms │                             65.36 ms │     no change │
│ QQuery 10    │ 46.32 ms │                             47.16 ms │     no change │
│ QQuery 11    │ 15.68 ms │                             15.56 ms │     no change │
│ QQuery 12    │ 31.77 ms │                             31.90 ms │     no change │
│ QQuery 13    │ 35.49 ms │                             34.81 ms │     no change │
│ QQuery 14    │ 27.31 ms │                             27.71 ms │     no change │
│ QQuery 15    │ 38.38 ms │                             37.60 ms │     no change │
│ QQuery 16    │ 14.13 ms │                             14.26 ms │     no change │
│ QQuery 17    │ 87.88 ms │                             89.00 ms │     no change │
│ QQuery 18    │ 98.37 ms │                             97.68 ms │     no change │
│ QQuery 19    │ 42.45 ms │                             41.87 ms │     no change │
│ QQuery 20    │ 33.75 ms │                             33.08 ms │     no change │
│ QQuery 21    │ 75.81 ms │                             76.60 ms │     no change │
│ QQuery 22    │ 10.94 ms │                             11.10 ms │     no change │
└──────────────┴──────────┴──────────────────────────────────────┴───────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━┓
┃ Benchmark Summary                                   ┃          ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━┩
│ Total Time (main)                                   │ 965.54ms │
│ Total Time (convert-array-to-scalar-vec-optional)   │ 963.50ms │
│ Average Time (main)                                 │  43.89ms │
│ Average Time (convert-array-to-scalar-vec-optional) │  43.80ms │
│ Queries Faster                                      │        1 │
│ Queries Slower                                      │        0 │
│ Queries with No Change                              │       21 │
│ Queries with Failure                                │        0 │
└─────────────────────────────────────────────────────┴──────────┘

vegarsti · 2025-10-04T06:39:44Z

datafusion/common/src/scalar/mod.rs

+        // 4: Offsets + Values looks like this: [[1, 2], [], [5]]
+        // But with NullBuffer it's like this: [[1, 2], NULL, [5]]
+        // The converted result is: [[1, 2], None, [5]]
+        let array4 = ListArray::new(
+            Field::new_list_field(DataType::Int64, true).into(),
+            OffsetBuffer::new(vec![0, 2, 2, 5].into()),
+            Arc::new(Int64Array::from(vec![1, 2, 3, 4, 5, 6])),
+            Some(NullBuffer::from(vec![true, false, true])),
+        );
+        let converted = ScalarValue::convert_array_to_scalar_vec(&array4).unwrap();
+        assert_eq!(
+            converted,
+            vec![
+                Some(vec![
+                    ScalarValue::Int64(Some(1)),
+                    ScalarValue::Int64(Some(2))
+                ]),
+                None,
+                Some(vec![
+                    ScalarValue::Int64(Some(3)),
+                    ScalarValue::Int64(Some(4)),
+                    ScalarValue::Int64(Some(5)),
+                ]),
+            ]
+        );


On main the middle element here would be an empty vec, not None.

vegarsti · 2025-10-04T06:39:53Z

datafusion/common/src/scalar/mod.rs

+        // 5: Offsets + Values looks like this: [[1, 2], [], [5]]
+        // Same as 4, but the middle array is not null, so after conversion it's empty.
+        let array5 = ListArray::new(
+            Field::new_list_field(DataType::Int64, true).into(),
+            OffsetBuffer::new(vec![0, 2, 2, 5].into()),
+            Arc::new(Int64Array::from(vec![1, 2, 3, 4, 5, 6])),
+            Some(NullBuffer::from(vec![true, true, true])),
+        );
+        let converted = ScalarValue::convert_array_to_scalar_vec(&array5).unwrap();
+        assert_eq!(
+            converted,
+            vec![
+                Some(vec![
+                    ScalarValue::Int64(Some(1)),
+                    ScalarValue::Int64(Some(2))
+                ]),
+                Some(vec![]),
+                Some(vec![
+                    ScalarValue::Int64(Some(3)),
+                    ScalarValue::Int64(Some(4)),
+                    ScalarValue::Int64(Some(5)),
+                ]),
            ]


On main the middle element here would also be an empty vec, not None.

vegarsti · 2025-10-04T06:40:57Z

Thanks @vegarsti I would love to see if there any performance degradations, you can find benches in the project.

Maybe we can have a separate test for this issues?

I added two test cases to show the change in this PR. On main these both return an empty vector, but with this PR the nullability is preserved, so in the first one (4) it's None and in the second one (5) it's Some (an empty vec).

https://github.com/apache/datafusion/pull/17891/files#diff-49e275af8f09685c7bbc491db8ab3b9479960878f42ac558ec0e3e39570590bd

https://github.com/vegarsti/datafusion/blob/e23271991bb2726df3ced25011cff0b08501b7b0/datafusion/common/src/scalar/mod.rs#L9128-L9177

github-actions bot added core Core DataFusion crate common Related to common crate functions Changes to functions implementation labels Oct 2, 2025

vegarsti changed the title ~~feat: convert_array_to_scalar_vec returns optional arrays~~ feat: convert_array_to_scalar_vec respects null elements Oct 2, 2025

vegarsti mentioned this pull request Oct 2, 2025

ScalarValue::convert_array_to_scalar_vec doesn't respect null list elements #17749

Open

vegarsti force-pushed the convert-array-to-scalar-vec-optional branch from 9c6db36 to b1bb29c Compare October 2, 2025 20:03

vegarsti commented Oct 2, 2025

View reviewed changes

vegarsti force-pushed the convert-array-to-scalar-vec-optional branch 2 times, most recently from 217287c to 35358d6 Compare October 2, 2025 20:35

vegarsti mentioned this pull request Oct 2, 2025

Support LargeList in array_has simplification to InList #17732

Merged

comphead reviewed Oct 3, 2025

View reviewed changes

feat: convert_array_to_scalar_vec returns optional arrays

e232719

vegarsti force-pushed the convert-array-to-scalar-vec-optional branch from 35358d6 to e232719 Compare October 4, 2025 06:27

vegarsti commented Oct 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: convert_array_to_scalar_vec respects null elements #17891

feat: convert_array_to_scalar_vec respects null elements #17891

vegarsti commented Oct 2, 2025 •

edited

Loading

Uh oh!

vegarsti Oct 2, 2025 •

edited

Loading

Uh oh!

vegarsti Oct 2, 2025

Uh oh!

vegarsti Oct 2, 2025

Uh oh!

comphead left a comment

Uh oh!

vegarsti commented Oct 3, 2025

Uh oh!

vegarsti commented Oct 4, 2025

Uh oh!

vegarsti Oct 4, 2025

Uh oh!

vegarsti Oct 4, 2025

Uh oh!

vegarsti commented Oct 4, 2025 •

edited

Loading

Uh oh!

Uh oh!

	if values.len() != ordering_values.len()
	\|\| values
	.iter()
	.zip(ordering_values.iter())
	.any(\|(vals, ordering_vals)\| vals.len() != ordering_vals.len())
	{
	return exec_err!(
	"Expects values arguments and/or ordering_values arguments to have same size"
	);

feat: convert_array_to_scalar_vec respects null elements #17891

Are you sure you want to change the base?

feat: convert_array_to_scalar_vec respects null elements #17891

Conversation

vegarsti commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

vegarsti Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vegarsti Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

vegarsti Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

vegarsti commented Oct 3, 2025

Uh oh!

vegarsti commented Oct 4, 2025

Uh oh!

vegarsti Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

vegarsti Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

vegarsti commented Oct 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

vegarsti commented Oct 2, 2025 •

edited

Loading

vegarsti Oct 2, 2025 •

edited

Loading

vegarsti commented Oct 4, 2025 •

edited

Loading