feat: support 'LargeList' in array_pop_front and array_pop_back
#8569
Conversation
github-actions
bot
added
physical-expr
| let array = as_large_list_array(&args[0])?; | ||
| general_pop_list::<i64>(array, false) | ||
| } | ||
| _ => exec_err!( |
There was a problem hiding this comment.
Comparing to
https://github.com/apache/arrow-datafusion/pull/8571/files#diff-48cc9cf1bfdb0214a9f625b384d1c4fd5967a9da61e8f22a5dc1c4c5800563b4R1301
no data types check, and the output error type mismatches, we may want to make array functions to have consistent codebase?
There was a problem hiding this comment.
I didn't get your point. What do you mean by no data type check? Because here are largelist and list type checks. @comphead
There was a problem hiding this comment.
Sorry for confusion,
what I was talking is
check_datatypes("array_positions", &[arr.values(), element])?;
which you do for array_positions method, but not doing for here.
There was a problem hiding this comment.
In array_positions, there are two arguments, array and element, which should be same data type. While for pop functions, they only accept one argument.
There was a problem hiding this comment.
that is true, i missed that part. thanks @Weijun-H
what about output error types? in array positions you return not_impl and in this PR its exec_err, but likely the conditions looks same and the error type should also be the same?
There was a problem hiding this comment.
Sorry, they should be the same, as exec_err . I will change them in a follow pr.
| where | ||
| i64: TryInto<O>, | ||
| { | ||
| let (from_array, to_array) = if from_front { |
There was a problem hiding this comment.
Is it better we just split this to two function? Since they are so different, without checking true for pop front or pop back may also improve readability?
There was a problem hiding this comment.
Yes, it is better for readability and maintenance.
Weijun-H
force-pushed
the
support-largelist-pop
branch
from
034c50f
to
4252f3f
Compare
Which issue does this PR close?
Parts. #8185
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?