refactor: data types in `array_expressions` #7280

izveigor · 2023-08-14T19:16:27Z

Which issue does this PR close?

Part of #7279
Part of #7288

Rationale for this change

We can use the template decorator (call_array_function) to reduce code (DRY method). I use scoping tactic (See examples: https://veykril.github.io/tlborm/decl-macros/minutiae/scoping.html)

What changes are included in this PR?

Are these changes tested?

No

Are there any user-facing changes?

Yes

izveigor · 2023-08-15T08:23:31Z

@alamb @jayzhan211 I wonder if you have time to review the cleanup.

jayzhan211

Overall LGTM

jayzhan211 · 2023-08-15T11:59:56Z

datafusion/physical-expr/src/array_expressions.rs


    let mut boolean_builder = BooleanArray::builder(array.len());
    for (arr, sub_arr) in array.iter().zip(sub_array.iter()) {
        if let (Some(arr), Some(sub_arr)) = (arr, sub_arr) {
-            let res = match (arr.data_type(), sub_arr.data_type()) {
-                (DataType::List(_), DataType::List(_)) => {
+            let res = match arr.data_type() {


Is it safe to only check arr type not sub_arr type?

Yes, check_datatypes checks the equality of arr and sub_arr

jayzhan211 · 2023-08-15T12:01:31Z

datafusion/physical-expr/src/array_expressions.rs

-                        "Array_append is not implemented for types '{array_data_type:?}' and '{element_data_type:?}'."
-                    )))
-                }
+    check_datatypes("array_append", &[arr.values(), &args[1]])?;


Nit: Use element instead of &args[1]

jayzhan211 · 2023-08-15T12:01:57Z

datafusion/physical-expr/src/array_expressions.rs

-            )))
-        }
-    };
+    check_datatypes("array_positions", &[arr.values(), &args[1]])?;


Nit: Use element instead of &args[1]

jayzhan211 · 2023-08-15T12:06:48Z

datafusion/physical-expr/src/array_expressions.rs

+    };
+    ($DATATYPE:expr, $INCLUDE_LIST:expr) => {{
+        match $DATATYPE {
+            DataType::List(_) => array_function!(ListArray),


Better if we use a flag to merge these two in one?

Suggested change

DataType::List(_) => array_function!(ListArray),

DataType::List(_) => if $INLCUDE_LIST {array_function!(ListArray)} else {

unreachable!()

}

It does not work with macro_rules! because every function (even with if) will consider List is one of possible ways to compute function (some functions can not work with List).

alamb

Thank you @izveigor -- very nice 👌

my only concern is about the change to the tests, otherwise this PR is a clear improvement in my mind

I do think we can make things even better (as a follow on PR) but I think we should move forward with this one

alamb · 2023-08-15T18:30:50Z

datafusion/physical-expr/src/array_expressions.rs

-        ))),
+    macro_rules! array_function {
+        ($ARRAY_TYPE:ident) => {
+            slice!(list_array, key, extra_key, return_element, $ARRAY_TYPE)


this code certainly looks nicer -- thank you @izveigor

I think this function still generates an individual function for each potential ListArray type, when it is fundamentally an operation on indices (and thus we don't need specialized functions for Strings, Int8s, Int16s, Int32s, etc)

However this is getting closer to a single implementation so I think it is improvement to what was here previously

alamb · 2023-08-15T18:31:40Z

datafusion/core/tests/sqllogictests/test_files/array.slt

 # array_append scalar function #1
-query ?


Is this a regression in functionality (aka something that used to work no longer does)? Is it hard to fix prior to merging this PR?

I'm now working on the problem of empty arrays, so I removed the functionality for a while. But I agree with you that it's better to leave the current null handling for now.

alamb · 2023-08-15T18:33:03Z

datafusion/physical-expr/src/array_expressions.rs

+
+        let array = array_append(&args);
+
+        assert!(array.is_err());


You might also want to check that the error is as expected with something like:

Suggested change

assert!(array.is_err());

assert!(array.unwrap_err().to_string(), "Expected error message");

alamb

Thank you @izveigor

alamb · 2023-08-16T13:07:25Z

🤔 the CI appears to be failing now

External error: query failed: DataFusion error: Optimizer rule 'simplify_expressions' failed
caused by
Error during planning: array_append received incompatible types: '[Null, Int64]'.
[SQL] select array_append(make_array(), 4);
at test_files/array.slt:868

Error: Execution("1 failures")
error: test failed, to rerun pass `-p datafusion-sqllogictest --test sqllogictests`

Caused by:
  process didn't exit successfully: `/__w/arrow-datafusion/arrow-datafusion/target/debug/deps/sqllogictests-0cd27f6536ed60ad` (exit status: 1)
Error: Process completed with exit code 1.

izveigor · 2023-08-16T16:11:38Z

@alamb I'm sorry, but we can not avoid regression with array_append and array_prepend right now. I think I can fix it in the next batch of PRs.

alamb · 2023-08-16T17:08:43Z

I took the liberty of merging up to this branch to resolve conflicts between this and #7293

izveigor added 2 commits August 13, 2023 18:25

feat: check_datatypes

17bfe54

refactor: data types in array_expressions

faaf54c

github-actions bot added the physical-expr Physical Expressions label Aug 14, 2023

fix: sqllogictests

87fffa7

github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Aug 14, 2023

fix: array_has

16155a0

izveigor marked this pull request as draft August 14, 2023 20:54

fix: array_has 2

47d16e8

izveigor marked this pull request as ready for review August 15, 2023 08:22

jayzhan211 approved these changes Aug 15, 2023

View reviewed changes

fix: use elements instead of args

9261c11

alamb approved these changes Aug 15, 2023

View reviewed changes

izveigor added 2 commits August 15, 2023 22:21

Merge branch 'main' into refactor-array

8dac820

fix: check_datatypes error output

475bb91

github-actions bot removed the core Core DataFusion crate label Aug 15, 2023

fix: sqllogictests

3413246

github-actions bot removed the sqllogictest SQL Logic Tests (.slt) label Aug 15, 2023

alamb approved these changes Aug 16, 2023

View reviewed changes

feat: add TODOs

49f95f7

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

Merge remote-tracking branch 'apache/main' into refactor-array

da4804d

alamb merged commit 3702aba into apache:main Aug 16, 2023
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: data types in `array_expressions` #7280

refactor: data types in `array_expressions` #7280

izveigor commented Aug 14, 2023 •

edited

Loading

izveigor commented Aug 15, 2023

jayzhan211 left a comment

jayzhan211 Aug 15, 2023

izveigor Aug 15, 2023

jayzhan211 Aug 15, 2023

jayzhan211 Aug 15, 2023

jayzhan211 Aug 15, 2023

jayzhan211 Aug 15, 2023

izveigor Aug 15, 2023

alamb left a comment

alamb Aug 15, 2023

alamb Aug 15, 2023

izveigor Aug 15, 2023

alamb Aug 15, 2023

alamb left a comment

alamb commented Aug 16, 2023 •

edited

Loading

izveigor commented Aug 16, 2023

alamb commented Aug 16, 2023

-            DataType::List(_) => array_function!(ListArray),
+            DataType::List(_) => if $INLCUDE_LIST {array_function!(ListArray)} else {
+            unreachable!()
+            }

	assert!(array.is_err());
	assert!(array.unwrap_err().to_string(), "Expected error message");

refactor: data types in array_expressions #7280

refactor: data types in array_expressions #7280

Conversation

izveigor commented Aug 14, 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?

izveigor commented Aug 15, 2023

jayzhan211 left a 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

Choose a reason for hiding this comment

alamb left a 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

alamb left a comment

Choose a reason for hiding this comment

alamb commented Aug 16, 2023 • edited Loading

izveigor commented Aug 16, 2023

alamb commented Aug 16, 2023

refactor: data types in `array_expressions` #7280

refactor: data types in `array_expressions` #7280

izveigor commented Aug 14, 2023 •

edited

Loading

alamb commented Aug 16, 2023 •

edited

Loading