Minor: Refactor array_union function to use a generic union_arrays function #8381
Conversation
union_arrays function
Weijun-H
changed the title
| r_field_ref: &Arc<Field>, | ||
| ) -> Result<ArrayRef> { | ||
| match (l_field_ref.data_type(), r_field_ref.data_type()) { | ||
| (DataType::Null, _) => Ok(array2.clone()), |
There was a problem hiding this comment.
Awesome @Weijun-H
Makes the code cleaner
Its not a problem of this PR but I'm thinking why we need to clone arrays in case of nulls.... 🤔
There was a problem hiding this comment.
If array1 is Null, return array2 directly because it is a union function. @comphead
There was a problem hiding this comment.
nvm, thanks @Weijun-H its cloning a reference not the underlying data
There was a problem hiding this comment.
I think the whole handling of Nulls in array functions is not quite correct (DataType::Null) should not be passed in. I believe @jayzhan211 is working on this
| r_field_ref: &Arc<Field>, | ||
| ) -> Result<ArrayRef> { | ||
| match (l_field_ref.data_type(), r_field_ref.data_type()) { | ||
| (DataType::Null, _) => Ok(array2.clone()), |
There was a problem hiding this comment.
I think the whole handling of Nulls in array functions is not quite correct (DataType::Null) should not be passed in. I believe @jayzhan211 is working on this
union_arrays function
Which issue does this PR close?
Closes #.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?