-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix: align array_has null buffer for scalar
#17272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @comphead -- this fix makes sense to me. I have some small test suggestion but otherwise this looks good to me
| CREATE EXTERNAL TABLE array_has STORED AS PARQUET location 'test_files/scratch/array/array_has/single_file.parquet'; | ||
|
|
||
| query B | ||
| select array_contains(a, b) from array_has; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This panics for me locally without the fix:
> select array_contains(a, b) from '/tmp/t.parquet';
thread 'main' panicked at /Users/andrewlamb/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/arrow-array-56.0.0/src/array/boolean_array.rs:91:13:
assertion `left == right` failed
left: 2
right: 1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend we also test with array_contains (b, a) as well to covert the other argument order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @alamb I'm not sure I'm following, the reverse order would violate array_has function signature.
Error during planning: Failed to coerce arguments to satisfy a call to 'array_has' function: coercion from [List(Field { name: "item", data_type: Boolean, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), List(Field { name: "item", data_type: List(Field { name: "item", data_type: Boolean, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} })] to the signature ArraySignature(Array { arguments: [Array, Element], array_coercion: Some(FixedSizedListToList) }) failed No function matches the given name and argument types 'array_has(List(Field { name: "item", data_type: Boolean, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), List(Field { name: "item", data_type: List(Field { name: "item", data_type: Boolean, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }))'. You might need to add explicit type casts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry -- I was just noticing in the code that it has two code paths when left and right are constants, but this query only exercises one of the paths, I think
* fix: align `array_has` null buffer for scalar
* fix: align `array_has` null buffer for scalar (#17272) * fix: align `array_has` null buffer for scalar * merge
Which issue does this PR close?
array_hasfails on null buffer union #17263 .Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?