Conversation
jayzhan211
reviewed
Oct 26, 2023
jayzhan211
reviewed
Oct 27, 2023
jayzhan211
reviewed
Oct 27, 2023
edmondop
commented
Oct 27, 2023
Co-authored-by: comphead <comphead@users.noreply.github.com>
Co-authored-by: comphead <comphead@users.noreply.github.com>
jayzhan211
requested changes
Nov 8, 2023
| fn union_generic_lists<OffsetSize: OffsetSizeTrait>( | ||
| l: &GenericListArray<OffsetSize>, | ||
| r: &GenericListArray<OffsetSize>, | ||
| ) -> Result<GenericListArray<OffsetSize>, DataFusionError> { |
Contributor
There was a problem hiding this comment.
I prefer Result<GenericListArray<OffsetSize>>
| (_, DataType::Null) => Ok(array1.clone()), | ||
| (DataType::List(_), DataType::List(_)) => { | ||
| check_datatypes("array_union", &[&array1, &array2])?; | ||
| let list1 = array1.as_list::<i32>(); |
Contributor
There was a problem hiding this comment.
I prefer datafusion::common::cast as_list_array and as_large_list_array
Contributor
There was a problem hiding this comment.
datafusion::common::cast and arrow::array::cast are doing the similar thing. The difference is that common::cast return datafusion error, while arrow::array::cast return Arror error. To me, return datafusion error in datafusion project make much more senses for me. Also, mixing these two casting is quite messy, we can have arrow casting inside common::cast and call common::cast for other crate.
Contributor
There was a problem hiding this comment.
I just found that arrow::array::cast does not return Result<>
Contributor
There was a problem hiding this comment.
never mind, I don't have strong opinion on which to use yet.
Member
There was a problem hiding this comment.
Why don't we directly use as_list_array here?
Contributor
Author
There was a problem hiding this comment.
For the offset i32 branch you mean @Weijun-H ? I like the fact that the code has the same structure for LargeList and List
Member
There was a problem hiding this comment.
Yes, but I think as_list_array can avoid panic because it returns the Return type.
jayzhan211
requested changes
Nov 8, 2023
| (_, DataType::Null) => Ok(array1.clone()), | ||
| (DataType::List(_), DataType::List(_)) => { | ||
| check_datatypes("array_union", &[&array1, &array2])?; | ||
| let list1 = array1.as_list::<i32>(); |
Contributor
There was a problem hiding this comment.
datafusion::common::cast and arrow::array::cast are doing the similar thing. The difference is that common::cast return datafusion error, while arrow::array::cast return Arror error. To me, return datafusion error in datafusion project make much more senses for me. Also, mixing these two casting is quite messy, we can have arrow casting inside common::cast and call common::cast for other crate.
jayzhan211
reviewed
Nov 8, 2023
9237fb4 to
4c1bcef
Compare
comphead
reviewed
Nov 8, 2023
Contributor
comphead
left a comment
There was a problem hiding this comment.
Thanks @edmondop for keep improving the PR
Please also test
- different datatypes, fixed len(int, float, etc) / variable(strings, inner arrays)
- nulls
- empty arrays
we can put tests for every type and figure out if this needs to be fixed within this PR or we can create a followup PR
Contributor
Author
Will do! Are the other cases for empty arrays that aren't there @comphead ? |
Contributor
Now I can see would be very nice to see |
Contributor
Author
|
Adding these two tests break the sqltests, but it doesn't seem to be caused by my code. I checked in the backtrace and there is nothing that refers The stacktrace is the following |
Contributor
array union should have the same data type which including the dimension of array. Union with 2d and 1d is not allowed |
comphead
reviewed
Nov 9, 2023
|
|
||
| # array_union scalar function #6 | ||
| query ? | ||
| select array_union([], []); |
Contributor
There was a problem hiding this comment.
checked in spark
scala> spark.sql("select array_union(array(), array())").show(false)
+-----------------------------+
|array_union(array(), array())|
+-----------------------------+
|[] |
+-----------------------------+
Contributor
Author
There was a problem hiding this comment.
I am even surprised my code work here tbh, do I need to add an additional branch to pattern matching where both array have null data type and return an empty array?
Contributor
There was a problem hiding this comment.
I am even surprised my code work here tbh, do I need to add an additional branch to pattern matching where both array have null data type and return an empty array?
I prefer return empty array in this case
Contributor
There was a problem hiding this comment.
I am even surprised my code work here tbh, do I need to add an additional branch to pattern matching where both array have null data type and return an empty array?
I don't think you should check the input types (they should still be lists, they'll just have an offset buffer of [0, 1]
Maybe you just need to handle the case specially in array_union
comphead
reviewed
Nov 9, 2023
|
|
||
| # array_union scalar function #10 | ||
| query ? | ||
| select array_union(null, null); |
Contributor
There was a problem hiding this comment.
scala> spark.sql("select array_union(array(null), array(null))").show(false)
+-------------------------------------+
|array_union(array(NULL), array(NULL))|
+-------------------------------------+
|[NULL] |
+-------------------------------------+
Contributor
Author
There was a problem hiding this comment.
This is more interesting, because I am not sure null has offsets in the ListArray, I suppose my code just get into the union function, but it exits immediately with empty results, because the offsets are empty...How would you go about it?
Contributor
There was a problem hiding this comment.
scala> spark.sql("select array_union(array(null), array(null))").show(false) +-------------------------------------+ |array_union(array(NULL), array(NULL))| +-------------------------------------+ |[NULL] | +-------------------------------------+
array(null) is not the same as null.
array(null) is like array_union scalar function #8
Contributor
Author
There was a problem hiding this comment.
I tried to add an extra branch in my match case like so:
(DataType::Null, DataType::Null) => {
let data = ArrayData::new_empty(&DataType::Null);
Ok(Arc::new(FixedSizeListArray::from(data)))
} but that also produce a NULL response rather than an empty arary, do you have any guidance @comphead ? It's unclear to me how to get an [] and also a [NULL]
comphead
reviewed
Nov 9, 2023
| [hello, datafusion] | ||
|
|
||
|
|
||
|
|
alamb
approved these changes
Nov 12, 2023
Contributor
alamb
left a comment
There was a problem hiding this comment.
Thank you @edmondop -- this is a really nice (first 🤯 ) contribution and a great example of a team effort.
I suggest we merge this PR as is, and file a ticket to fix the null handling as a follow on PR. My rationale is that this is a new function, so it isn't introducing a regression and we can iteratively improve it over time
| let r_values = converter.convert_columns(&[r_values])?; | ||
|
|
||
| // Might be worth adding an upstream OffsetBufferBuilder | ||
| let mut offsets = Vec::<OffsetSize>::with_capacity(l.len() + 1); |
Contributor
There was a problem hiding this comment.
This is a really neat implementation @edmondop.
|
|
||
| # array_union scalar function #6 | ||
| query ? | ||
| select array_union([], []); |
Contributor
There was a problem hiding this comment.
I am even surprised my code work here tbh, do I need to add an additional branch to pattern matching where both array have null data type and return an empty array?
I don't think you should check the input types (they should still be lists, they'll just have an offset buffer of [0, 1]
Maybe you just need to handle the case specially in array_union
Contributor
|
Thanks again everyone who contributed to this PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #6981
Rationale for this change
Support additional array_union SQL keyword
Are these changes tested?
Additional slt tests have been added, obviously they do not pass until duplication issue is solved :)