feat: Add evaluate_to_arrays function #18446

EmilyMatt · 2025-11-02T13:56:03Z

Which issue does this PR close?

Closes Create unified evaluate_many util function #18330 .

Rationale for this change

Reduce code duplication.

What changes are included in this PR?

A util function replacing many calls which are using the same code.

Are these changes tested?

No logic should change whatsoever, so each area which now uses this code should have it's own tests and benchmarks unmodified.

Are there any user-facing changes?

Yes, there is now a new pub function.
No other changes to API.

adriangb · 2025-11-02T18:12:58Z

datafusion/physical-expr-common/src/utils.rs

+#[inline]
+pub fn evaluate_expressions_to_arrays(
+    exprs: &[Arc<dyn PhysicalExpr>],
+    batch: &RecordBatch,
+) -> Result<Vec<ArrayRef>> {
+    let num_rows = batch.num_rows();
+    exprs
+        .iter()
+        .map(|e| e.evaluate(batch).and_then(|col| col.into_array(num_rows)))
+        .collect::<Result<Vec<ArrayRef>>>()
+}


Can we handle the footgun of This means that this should not be called on expressions that may return // arrays of different lengths by matching the ColumnarValue and blowing up Scalars, passing through arrays of the same length but erroring if the result is an array of a different length?

Also since this is going to be used pretty widely maybe a docstring example, etc. would help

Sorry, I had a few busy days.
I think this should be a function on the ColumnarArray itself that we could call here and would return an error if the length was different than num_rows

But I do think that's another feature/function that is missing in my opinion

I believe there is ColumnarValue::to_array already, but that doesn't check the length I assume? https://docs.rs/datafusion-expr-common/50.3.0/src/datafusion_expr_common/columnar_value.rs.html#145

to_array is the same as into_array except it takes by reference, which is an extra unneeded clone in this case.
No relevant method exists that I know of, in my project I just made an extension trait for ColumnarValue and added a function

To avoid bike shedding I'll just give my code suggestion:

Suggested change

#[inline]

pub fn evaluate_expressions_to_arrays(

exprs: &[Arc<dyn PhysicalExpr>],

batch: &RecordBatch,

) -> Result<Vec<ArrayRef>> {

let num_rows = batch.num_rows();

exprs

.iter()

.map(|e| e.evaluate(batch).and_then(|col| col.into_array(num_rows)))

.collect::<Result<Vec<ArrayRef>>>()

}

#[inline]

pub fn evaluate_expressions_to_arrays(

exprs: &[Arc<dyn PhysicalExpr>],

batch: &RecordBatch,

) -> Result<Vec<ArrayRef>> {

let num_rows = batch.num_rows();

exprs

.iter()

.map(|e| e.evaluate(batch).and_then(|col| {

match col {

ColumnarValue::Scalar(scalar) => scalar.to_array(num_rows),

ColumnarValue::Array(array) => {

if array.num_rows() == num_rows() {

Ok(array)

} else {

internal_err!("Expressions evaluated to arrays of mismatched lengths")

}

}

}))

.collect::<Result<Vec<ArrayRef>>>()

}

Seems reasonable to me, I do still think this should be inside ColumnarValue, I will prob make this PR in the near future

Agreed I think into_array should be returning an error and checking for this, but that might be too big of a breaking change -> might just need to be a new function and document the foot gun or deprecate into_array.

I have decided to just add the required functions in ColumnarValue, as well as make this function more generic, so it can be used directly with iterators such as the group_by and nulls exprs case

martin-g · 2025-11-06T14:03:32Z

datafusion/physical-plan/src/aggregates/mod.rs

            value.into_array(batch.num_rows())
        })
        .collect::<Result<Vec<_>>>()?;



I believe these could make use of the new helper function too:

let exprs_list: Vec<_> = group_by.expr.iter().map(|(e, _)| Arc::clone(e)).collect(); let null_exprs_list: Vec<_> = group_by.null_expr.iter().map(|(e, _)| Arc::clone(e)).collect(); let exprs: Vec<ArrayRef> = evaluate_expressions_to_arrays(&exprs_list, batch)?; let null_exprs: Vec<ArrayRef> = evaluate_expressions_to_arrays(&null_exprs_list, batch)?;

I actually considered it, just didn't want to allocate new Vecs if I could help it, but maybe worth adding anyway

datafusion/physical-plan/src/aggregates/no_grouping.rs

Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>

…alue

datafusion/expr-common/src/columnar_value.rs

rluvaton · 2025-11-10T14:42:57Z

datafusion/expr-common/src/columnar_value.rs

    /// number of rows. [`Self::Scalar`] is converted by repeating the same
    /// scalar multiple times which is not as efficient as handling the scalar
    /// directly.
+    /// This validates that [`Self::Array`], if it exists, has the expected length.


Suggested change

/// This validates that [`Self::Array`], if it exists, has the expected length.

/// This validates that if this is [`Self::Array`] it has the expected length.

rluvaton · 2025-11-10T14:44:54Z

datafusion/expr-common/src/columnar_value.rs

+        let arr = make_array(1, 3);
+        let arr_columnar_value = ColumnarValue::Array(Arc::clone(&arr));
+        let result = arr_columnar_value.into_array_of_size(5);
+        assert!(result.is_err());


can you validate the exact error as it can return different error

rluvaton

thank you @EmilyMatt, left some comments

@adriangb do you have more comments? or can I merge after my comments addressed

alamb

Looks like a great improvement to me @EmilyMatt

Thank you @rluvaton and @adriangb for the reviews

alamb · 2025-11-11T20:54:58Z

datafusion/physical-expr/src/window/standard_window_function_expr.rs

-                    .and_then(|v| v.into_array(batch.num_rows()))
-            })
-            .collect()
+        evaluate_expressions_to_arrays(&self.expressions(), batch)


rluvaton · 2025-11-12T12:11:21Z

Thank you @EmilyMatt hope to see you contributing again :)

github-actions bot added physical-expr Changes to the physical-expr crates physical-plan Changes to the physical-plan crate labels Nov 2, 2025

EmilyMatt force-pushed the eval-to-arrays branch from b984129 to ff2f57a Compare November 2, 2025 13:56

adriangb reviewed Nov 2, 2025

View reviewed changes

martin-g approved these changes Nov 6, 2025

View reviewed changes

EmilyMatt added 2 commits November 6, 2025 16:03

feat: Add evaluate_to_arrays function

0717ab9

Update doc, add note for devs

8eb7788

EmilyMatt force-pushed the eval-to-arrays branch from ff2f57a to 8eb7788 Compare November 6, 2025 14:03

martin-g reviewed Nov 6, 2025

View reviewed changes

EmilyMatt and others added 2 commits November 6, 2025 16:42

Update datafusion/physical-plan/src/aggregates/no_grouping.rs

9fb6daa

Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>

Make the function more generic, add the needed functions in ColumnarV…

396de6c

…alue

github-actions bot added the logical-expr Logical plan and expressions label Nov 6, 2025

martin-g approved these changes Nov 6, 2025

View reviewed changes

add test

0791062

rluvaton reviewed Nov 10, 2025

View reviewed changes

datafusion/expr-common/src/columnar_value.rs Show resolved Hide resolved

rluvaton reviewed Nov 10, 2025

View reviewed changes

rluvaton approved these changes Nov 10, 2025

View reviewed changes

EmilyMatt added 5 commits November 10, 2025 17:01

Merge remote-tracking branch 'origin/main' into eval-to-arrays

2541120

address CR

e13afbc

fix test when backtrace is enabled

b73ac2e

fix test

778c971

no way istg

3597bcc

alamb approved these changes Nov 11, 2025

View reviewed changes

rluvaton added this pull request to the merge queue Nov 12, 2025

Merged via the queue into apache:main with commit 76b4156 Nov 12, 2025
32 checks passed

	/// This validates that [`Self::Array`], if it exists, has the expected length.
	/// This validates that if this is [`Self::Array`] it has the expected length.

feat: Add evaluate_to_arrays function #18446

feat: Add evaluate_to_arrays function #18446

Uh oh!

Conversation

EmilyMatt commented Nov 2, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rluvaton left a comment

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rluvaton commented Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants