fix: add `ArrayAndElementAndOptionalIndex` for proper casting in `array_position` #9233

tshauck · 2024-02-15T02:05:14Z

Which issue does this PR close?

Rationale for this change

Currently throws an error when doing SELECT array_position(arrow_cast([1, 2, 3, 4, 5], 'List(Int32)'), 5), and after this PR it won't.

What changes are included in this PR?

Change recommended in #9093 (comment)

Are these changes tested?

Added an additional sql logic test that failed w/o the change, and doesn't fail w/ the change.

Are there any user-facing changes?

No

tshauck · 2024-02-15T02:16:42Z

Ah, actually seeing that won't work in the case of the optional 3rd arg for array position. Not sure if there's something built-in, but will look around. Or maybe add a ArrayAndElementPlusArgs? 🤔

jayzhan211 · 2024-02-15T03:14:08Z

Ah, actually seeing that won't work in the case of the optional 3rd arg for array position. Not sure if there's something built-in, but will look around. Or maybe add a ArrayAndElementPlusArgs? 🤔

No built-in signature, maybe we need ArrayAndElementAndOptionalIndex 😢

tshauck · 2024-02-15T04:11:11Z

That'd definitely be a better name... I'll let this PR sit for a couple of days in case there's a better idea or compelling reason not to do this in the first place, then'll see what ArrayAndElementAndOptionalIndex looks like.

tshauck · 2024-02-16T01:51:34Z

I think this is ready for a review. It adds ArrayAndElementAndOptionalIndex and associated code -- thanks!

jayzhan211 · 2024-02-16T11:37:11Z

datafusion/expr/src/type_coercion/functions.rs

+        let first_two_types = &current_types[0..2];
+        let valid_types = array_append_or_prepend_valid_types(first_two_types, true)?;
+
+        let valid_types_with_index = valid_types


We can optionally append valid_types_with_index if the length is 3, otherwise, return here.

Thanks! Addressed in 5086f31.

jayzhan211 · 2024-02-16T11:39:07Z

datafusion/expr/src/type_coercion/functions.rs

@@ -130,6 +130,31 @@ fn get_valid_types(
            _ => Ok(vec![vec![]]),
        }
    }
+    fn array_append_and_optional_index(


array_element_and_optional_index seems a better name

Also in 5086f31

alamb

Thanks @tshauck and @jayzhan211 -- this PR is looking very close

alamb · 2024-02-16T11:53:43Z

datafusion/sqllogictest/test_files/array.slt

@@ -2603,6 +2603,16 @@ select array_position(arrow_cast(make_array([1, 2, 3], [4, 5, 6], [5, 5, 5], [4,
 ----
 2 2

+query I
+SELECT array_position(arrow_cast([5, 2, 3, 4, 5], 'List(Int32)'), 5)


For the record, this test fails on main like this:

Error during planning: array_position received incompatible types: '[Int32, Int64]'.

alamb · 2024-02-16T11:56:55Z

datafusion/sqllogictest/test_files/array.slt

+1
+
+query I
+SELECT array_position(arrow_cast([5, 2, 3, 4, 5], 'List(Int32)'), 5, 2)


Can we please add a few more tests:

For LargeList

Error cases (e.g. for types that are still not compatible like array_position(arrow_cast([5, 2, 3, 4, 5], 'List(Int32)'), 'foo')

Thanks for the feedback... LargeList is added and works as expected. For array_position(arrow_cast([5, 2, 3, 4, 5], 'List(Int32)'), 'foo'), this actually doesn't fail right now on this branch and returns NULL:

❯ SELECT array_position(arrow_cast([5, 2, 3, 4, 5], 'List(Int32)'), 'foo') IS NULL; +----------------------------------------------------------------------------------------------+ | array_position(make_array(Int64(5),Int64(2),Int64(3),Int64(4),Int64(5)),Utf8("foo")) IS NULL | +----------------------------------------------------------------------------------------------+ | true | +----------------------------------------------------------------------------------------------+ 1 row in set. Query took 0.004 seconds.

Because this PR is leveraging the append's casting functionality, this seems to be the current behavior on main for append:

arrow-datafusion/datafusion-cli main ➜ ./target/debug/datafusion-cli DataFusion CLI v35.0.0 ❯ SELECT array_append(arrow_cast([5, 2, 3, 4, 5], 'List(Int32)'), 'foo'); +------------------------------------------------------------------------------------+ | array_append(make_array(Int64(5),Int64(2),Int64(3),Int64(4),Int64(5)),Utf8("foo")) | +------------------------------------------------------------------------------------+ | [5, 2, 3, 4, 5, foo] | +------------------------------------------------------------------------------------+ 1 row in set. Query took 0.028 seconds.

I guess as I user I'd think if the array_append case would work, I'd also expect array_position to work like that. FWIW duckdb works for both array_append and array_position([1,2,3], 'foo') it just returns 0 rather than NULL.

Anyways, happy to go either way, just thought I'd bring it up before delving further.

For the second case, we return NULL which is compatible with Postgres behavior, so I think it is acceptable. You can just add the test case to make sure we got NULL

query I select array_position(arrow_cast([5, 2, 3, 4, 5], 'List(Int32)'), 'foo') ---- NULL

Sounds good... added in 4ec6289

Thanks for adding the tests and doing the background research

jayzhan211

LGTM

alamb

Thanks @tshauck and @jayzhan211 for the reviews 🙏

alamb · 2024-02-19T06:58:25Z

datafusion/sqllogictest/test_files/array.slt

+1
+
+query I
+SELECT array_position(arrow_cast([5, 2, 3, 4, 5], 'List(Int32)'), 5, 2)


Thanks for adding the tests and doing the background research

tshauck · 2024-02-19T17:49:35Z

Great, thank you both!

fix: use array_and_element for proper casting in array_position

c2f76f9

github-actions bot added logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) labels Feb 15, 2024

fix: fix typo

2d7b78b

tshauck added 4 commits February 15, 2024 15:53

feat: add ArrayAndElementAndOptionalIndex

64b4be3

refactor: cleanup

3cc94ac

docs: add docs to enum variants

b19e3c8

doc: fix cargo doc formatting snafu

2a83ab4

tshauck marked this pull request as ready for review February 16, 2024 01:51

tshauck changed the title ~~fix: use array_and_element for proper casting in array_position~~ fix: add ArrayAndElementAndOptionalIndex for proper casting in array_position Feb 16, 2024

jayzhan211 reviewed Feb 16, 2024

View reviewed changes

alamb reviewed Feb 16, 2024

View reviewed changes

alamb mentioned this pull request Feb 16, 2024

DataFusion weekly project plan (Andrew Lamb) - Feb 12, 2024 #9200

Closed

8 tasks

tshauck added 3 commits February 16, 2024 08:17

test: add a couple of tests

466f2c9

refactor: update names, early exit logic

5086f31

test: add null test for array_position

4ec6289

jayzhan211 approved these changes Feb 18, 2024

View reviewed changes

alamb approved these changes Feb 19, 2024

View reviewed changes

alamb merged commit 60ee91e into apache:main Feb 19, 2024
23 checks passed

tshauck deleted the update-array-position branch February 19, 2024 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add `ArrayAndElementAndOptionalIndex` for proper casting in `array_position` #9233

fix: add `ArrayAndElementAndOptionalIndex` for proper casting in `array_position` #9233

tshauck commented Feb 15, 2024

tshauck commented Feb 15, 2024 •

edited

Loading

jayzhan211 commented Feb 15, 2024

tshauck commented Feb 15, 2024

tshauck commented Feb 16, 2024

jayzhan211 Feb 16, 2024 •

edited

Loading

tshauck Feb 16, 2024

jayzhan211 Feb 16, 2024

tshauck Feb 16, 2024

alamb left a comment

alamb Feb 16, 2024

alamb Feb 16, 2024

tshauck Feb 16, 2024 •

edited

Loading

jayzhan211 Feb 17, 2024 •

edited

Loading

tshauck Feb 17, 2024

alamb Feb 19, 2024

jayzhan211 left a comment

alamb left a comment

alamb Feb 19, 2024

tshauck commented Feb 19, 2024

fix: add ArrayAndElementAndOptionalIndex for proper casting in array_position #9233

fix: add ArrayAndElementAndOptionalIndex for proper casting in array_position #9233

Conversation

tshauck commented Feb 15, 2024

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?

tshauck commented Feb 15, 2024 • edited Loading

jayzhan211 commented Feb 15, 2024

tshauck commented Feb 15, 2024

tshauck commented Feb 16, 2024

jayzhan211 Feb 16, 2024 • 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

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

tshauck Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

jayzhan211 Feb 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

tshauck commented Feb 19, 2024

fix: add `ArrayAndElementAndOptionalIndex` for proper casting in `array_position` #9233

fix: add `ArrayAndElementAndOptionalIndex` for proper casting in `array_position` #9233

tshauck commented Feb 15, 2024 •

edited

Loading

jayzhan211 Feb 16, 2024 •

edited

Loading

tshauck Feb 16, 2024 •

edited

Loading

jayzhan211 Feb 17, 2024 •

edited

Loading