Minor: Support `nulls` in `array_replace`, avoid a copy #8054

alamb · 2023-11-04T20:02:27Z

Which issue does this PR close?

Rationale for this change

The change in #8050 is a huge improvement, and while reviewing it I saw a few more things to improve, specifically have it handle nulls handing and avoiding some copies.

I also wanted to try and help illustrate some patterns for other array function rewrites that I think @jayzhan211 is planning

What changes are included in this PR?

Add comments to general_array and change names of parameters and variables so it is easier (in my opinion) to follow the logic
Handles NULL values

Are these changes tested?

Covered by existing test
Added a new test for null values

Are there any user-facing changes?

array_replace now properly handles NULL values

alamb · 2023-11-05T10:33:58Z

datafusion/physical-expr/src/array_expressions.rs

-    let from_array = &args[1];
-    let to_array = &args[2];
-
+/// For each element of `list_array[i]`, replaces up to `arr_n[i]`  occurences


In general, having a clear description of what the function does, especially when it is mind bending likegeneral_replace helps a lot. I also find that writing such a description often helps improve the code as well as results in better naming

For example, I struggled to explain how this function worked when it took &[ArrayRef] because then the explanations were in terms fof arg[0], arg[1], and arg[2]. Giving the parameters names made things much clearer

alamb · 2023-11-05T10:36:09Z

datafusion/physical-expr/src/array_expressions.rs


-    for (row_index, (arr, n)) in list_array.iter().zip(arr_n.iter()).enumerate() {
+    // n is the number of elements to replace in this row
+    for (row_index, (list_array_row, n)) in


I rename these variables to better explain what they are (rather than arr a shorthand for array which is already overloaded in this function

alamb · 2023-11-05T10:36:55Z

datafusion/physical-expr/src/array_expressions.rs

-                                bool_values.push(Some(a.eq(&from_a)));
-                            } else {
-                                return internal_err!(
-                                    "Null value is not supported in array_replace"


I don't know why nulls aren't supported -- all of the kernels support them well, and the code is more concise without the special case

alamb · 2023-11-05T10:37:23Z

datafusion/physical-expr/src/array_expressions.rs

                    }
                };

                // Use MutableArrayData to build the replaced array
+                let original_data = list_array_row.to_data();


Rather than using a new array with two indexes, I found naming them helped a lot

alamb · 2023-11-05T10:40:43Z

datafusion/physical-expr/src/array_expressions.rs

+        new_empty_array(&data_type)
+    } else {
+        let new_values: Vec<_> = new_values.iter().map(|a| a.as_ref()).collect();
+        arrow::compute::concat(&new_values)?


The old logic called concat for each row, which I think is O(N^2) as it keeps copying the same values over and over again. I changed it to call concat just once

I think it is probably possible to avoid calling concat entirely (use MutableArrayData to build up values directly). We can do that as a follow on PR perhaps.

We used to replace MutableArrayData with ListArray, as it is more straightforward. However, perhaps we can also construct ListArray using MutableArrayData and still maintain readability. Particularly in this case, doing so can enhance performance. We should certainly consider building with MutableArrayData!

Yeah, what I was getting at is that the contents of values are each already created using MutableArrayData -- and so I think rather than making many small arrays and then concating them together, the could could just use the same MutableArrayData to build the results directly

alamb · 2023-11-05T10:41:51Z

datafusion/physical-expr/src/array_expressions.rs

@@ -2763,6 +2790,52 @@ mod tests {
        );
    }

+    #[test]
+    fn test_array_replace_with_null() {


Since the null cases were not covered, I added new coverage. Maybe this style of test could help as we clean up the other aray functions

I would prefer having test in slt file if possible, we can cleanup test in the follow on PR.

slt is a good idea -- I will move the tests

Done in c6dabcf

jayzhan211 · 2023-11-06T00:04:45Z

datafusion/physical-expr/src/array_expressions.rs

+    #[test]
+    fn test_array_replace_with_null() {
+        // ([3, 1, NULL, 3], 3,    4,    2)  => [4, 1, NULL, 4] NULL not matched
+        // ([3, 1, NULL, 3], NULL, 5,    2)  => [3, 1, NULL, 3]  NULL  not replaced (not eq)


why null is not replaced with 5 => [3, 1, 5, 3]?

It seems that arrow_ord::cmp::eq([3,1,null,3], null) return [null,null,null,null] which I expect to see [false,false,true,false]

I believe it's reasonable for arrow-rs to handle null comparisons

I think it is because in SQL, NULL = NULL is NOT true like you might expect, but is actually NULL

If we want the NULL = NULL = true behavior, we could use the not_distinct kernel instead: https://docs.rs/arrow/latest/arrow/compute/kernels/cmp/fn.not_distinct.html

I think it is because in SQL, NULL = NULL is NOT true like you might expect, but is actually NULL

If we want the NULL = NULL = true behavior, we could use the not_distinct kernel instead: https://docs.rs/arrow/latest/arrow/compute/kernels/cmp/fn.not_distinct.html

I would prefer having not_distinct here, it can let us replace null value with another, if we use eq here, replace with null value is meaningless, also there is no other DB have the similar function #7072, so I think we can have our own definition of what array_replace do.

ok, sounds good. I will update this PR

Done in acbffa0

jayzhan211 · 2023-11-06T00:05:21Z

datafusion/physical-expr/src/array_expressions.rs

@@ -2763,6 +2790,52 @@ mod tests {
        );
    }

+    #[test]
+    fn test_array_replace_with_null() {


I would prefer having test in slt file if possible, we can cleanup test in the follow on PR.

jayzhan211

LGTM!

alamb · 2023-11-08T16:34:17Z

@jackwener or @liukun4515 can I trouble you for an approval of this PR so I can merge it? @jayzhan211 has already reviewed it

jackwener

Nice job! Thanks @alamb

datafusion/physical-expr/src/array_expressions.rs

alamb · 2023-11-09T11:05:28Z

Nice job! Thanks @alamb

Thank you for the review @jackwener and @jayzhan211 -- very much appreciated.

Co-authored-by: jakevin <jakevingoo@gmail.com>

Minor: clean up array_replace

ef69a13

github-actions bot added the physical-expr Physical Expressions label Nov 4, 2023

alamb added 2 commits November 5, 2023 05:28

null test

05bdf1d

remove println

812bd0b

alamb commented Nov 5, 2023

View reviewed changes

alamb marked this pull request as ready for review November 5, 2023 10:42

alamb changed the title ~~Minor: clean up array_replace~~ Support nulls in array_replace, avoid a copy Nov 5, 2023

alamb mentioned this pull request Nov 5, 2023

General approach for Array replace #8050

Merged

Fix doc test

0e6c5c8

jayzhan211 requested changes Nov 6, 2023

View reviewed changes

alamb self-assigned this Nov 6, 2023

This was referenced Nov 6, 2023

DataFusion weekly project plan (Andrew Lamb) - Nov 6, 2023 #8063

Closed

Minor: improve error message by adding types to message #8065

Merged

port test to sqllogictest

c6dabcf

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Nov 6, 2023

Use not_distinct

acbffa0

alamb changed the title ~~Support nulls in array_replace, avoid a copy~~ Minor: Support nulls in array_replace, avoid a copy Nov 6, 2023

alamb mentioned this pull request Nov 6, 2023

Minor: remove duplicated array_replace tests #8066

Merged

alamb requested a review from jayzhan211 November 6, 2023 19:14

jayzhan211 approved these changes Nov 6, 2023

View reviewed changes

jayzhan211 mentioned this pull request Nov 7, 2023

Lots of code duplication in array_functions (array_replace_all, array_replace_n, array_replace, etc) #7988

Closed

jackwener approved these changes Nov 9, 2023

View reviewed changes

datafusion/physical-expr/src/array_expressions.rs Outdated Show resolved Hide resolved

datafusion/physical-expr/src/array_expressions.rs Outdated Show resolved Hide resolved

alamb and others added 2 commits November 9, 2023 06:06

Apply suggestions from code review

cafc404

Co-authored-by: jakevin <jakevingoo@gmail.com>

Merge remote-tracking branch 'apache/main' into alamb/cleanup_cleanup

58a6ab9

alamb merged commit 93af440 into apache:main Nov 9, 2023
22 checks passed

alamb mentioned this pull request Nov 13, 2023

DataFusion weekly project plan (Andrew Lamb) - Nov 13, 2023 #8151

Closed

9 tasks

matthewgapp mentioned this pull request Jan 11, 2024

matt/feat/recursive ctes/config flag matthewgapp/arrow-datafusion#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor: Support `nulls` in `array_replace`, avoid a copy #8054

Minor: Support `nulls` in `array_replace`, avoid a copy #8054

alamb commented Nov 4, 2023 •

edited

Loading

alamb Nov 5, 2023

alamb Nov 5, 2023

alamb Nov 5, 2023

alamb Nov 5, 2023

alamb Nov 5, 2023

jayzhan211 Nov 6, 2023

alamb Nov 6, 2023

alamb Nov 5, 2023

jayzhan211 Nov 6, 2023 •

edited

Loading

alamb Nov 6, 2023

alamb Nov 6, 2023

jayzhan211 Nov 6, 2023

jayzhan211 Nov 6, 2023

jayzhan211 Nov 6, 2023

alamb Nov 6, 2023

jayzhan211 Nov 6, 2023 •

edited

Loading

alamb Nov 6, 2023

alamb Nov 6, 2023

jayzhan211 Nov 6, 2023 •

edited

Loading

jayzhan211 left a comment

alamb commented Nov 8, 2023

jackwener left a comment

alamb commented Nov 9, 2023

Minor: Support nulls in array_replace, avoid a copy #8054

Minor: Support nulls in array_replace, avoid a copy #8054

Conversation

alamb commented Nov 4, 2023 • edited Loading

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

jayzhan211 left a comment

Choose a reason for hiding this comment

alamb commented Nov 8, 2023

jackwener left a comment

Choose a reason for hiding this comment

alamb commented Nov 9, 2023

Minor: Support `nulls` in `array_replace`, avoid a copy #8054

Minor: Support `nulls` in `array_replace`, avoid a copy #8054

alamb commented Nov 4, 2023 •

edited

Loading

jayzhan211 Nov 6, 2023 •

edited

Loading

jayzhan211 Nov 6, 2023 •

edited

Loading

jayzhan211 Nov 6, 2023 •

edited

Loading