-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
move array_replace family functions to datafusion-function-array crate #9651
Conversation
a6c47ec
to
ba2a344
Compare
4ff9ba0
to
c199e4c
Compare
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.
lgtm thanks @Weijun-H
f8ec815
to
b0324cd
Compare
/// [[1, 2, 3], [2, 3, 4], [2, 3, 4]], [[1, 2, 3], [2, 3, 4], [3, 4, 5]], 1, false => [true, false, false] | ||
/// ) | ||
/// ``` | ||
pub(crate) fn compare_element_to_list( |
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.
We can move it to utils
, not only replace functions rely on it.
@@ -115,6 +119,9 @@ pub fn register_all(registry: &mut dyn FunctionRegistry) -> Result<()> { | |||
set_ops::array_union_udf(), | |||
position::array_position_udf(), | |||
position::array_positions_udf(), | |||
array_replace::array_replace_n_udf(), |
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.
nit: I think we can name it replace
from_array.clone(), | ||
to_array.clone() | ||
), | ||
criterion::black_box(&expected_array).clone() |
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.
is it possible to avoid the clone here? it is already cloned in L38
c771529
to
69a286a
Compare
Thanks @Weijun-H ! I pushed a commit to this branch to resolve the CI failure |
Which issue does this PR close?
Relate #9285
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?