You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "foskey51 (via GitHub)" <gi...@apache.org> on 2025/11/20 09:54:28 UTC
[PR] chore: enforce clippy lint needless_pass_by_value to datafusion-functions-nested [datafusion]
foskey51 opened a new pull request, #18839:
URL: https://github.com/apache/datafusion/pull/18839
## Which issue does this PR close?
- Closes #18835.
- Part of #18503.
## What changes are included in this PR?
enforce clippy lint `needless_pass_by_value` to `datafusion-functions-nested`
## Are these changes tested?
yes
## Are there any user-facing changes?
no
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: enforce clippy lint needless_pass_by_value to datafusion-functions-nested [datafusion]
Posted by "foskey51 (via GitHub)" <gi...@apache.org>.
foskey51 commented on code in PR #18839:
URL: https://github.com/apache/datafusion/pull/18839#discussion_r2550416640
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -607,8 +607,8 @@ where
fn string_to_array_inner_3<'a, StringArrType, DelimiterArrType, StringBuilderType>(
args: &'a [ArrayRef],
- string_array: StringArrType,
- delimiter_array: DelimiterArrType,
+ string_array: &StringArrType,
+ delimiter_array: &DelimiterArrType,
Review Comment:
But since rust performs de-ref coercion implicitly, so it shouldn't be a problem. I had the same question initially, but after digging through some sources, it seems expected behavior.
[1] https://stackoverflow.com/a/28552082
[2] https://users.rust-lang.org/t/double-references-what-they-mean/56668/2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: enforce clippy lint needless_pass_by_value to datafusion-functions-nested [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #18839:
URL: https://github.com/apache/datafusion/pull/18839#discussion_r2547246915
##########
datafusion/functions-nested/src/set_ops.rs:
##########
@@ -428,7 +428,7 @@ fn generic_set_lists<OffsetSize: OffsetSizeTrait>(
fn general_set_op(
array1: &ArrayRef,
array2: &ArrayRef,
- set_op: SetOp,
+ set_op: &SetOp,
Review Comment:
I think a better change would be to make `SetOp` implement the `Copy` trait, as it is an enum with values -- there is no real benefit to passing a reference
https://github.com/apache/datafusion/blob/30b0982f24eef7aa73b088b1e118e6f849f7fcb4/datafusion/functions-nested/src/set_ops.rs#L331-L330
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: enforce clippy lint needless_pass_by_value to datafusion-functions-nested [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #18839:
URL: https://github.com/apache/datafusion/pull/18839#discussion_r2551615823
##########
datafusion/functions-nested/src/map.rs:
##########
@@ -126,10 +126,10 @@ fn get_first_array_ref(columnar_value: &ColumnarValue) -> Result<ArrayRef> {
}
fn make_map_batch_internal(
- keys: ArrayRef,
- values: ArrayRef,
+ keys: &ArrayRef,
+ values: &ArrayRef,
Review Comment:
Yep it seems all usages of `&ArrayRef` do need the Arc and can't be reduced to just `&dyn Array`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: enforce clippy lint needless_pass_by_value to datafusion-functions-nested [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #18839:
URL: https://github.com/apache/datafusion/pull/18839#discussion_r2548275309
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -607,8 +607,8 @@ where
fn string_to_array_inner_3<'a, StringArrType, DelimiterArrType, StringBuilderType>(
args: &'a [ArrayRef],
- string_array: StringArrType,
- delimiter_array: DelimiterArrType,
+ string_array: &StringArrType,
+ delimiter_array: &DelimiterArrType,
Review Comment:
I still find this especially weird since `StringArrayType<'a>` is implemented for references, so this becomes a `&&` at the callsite?
##########
datafusion/functions-nested/src/array_has.rs:
##########
@@ -317,8 +317,8 @@ impl<'a> ArrayWrapper<'a> {
}
}
-fn array_has_dispatch_for_array(
- haystack: ArrayWrapper<'_>,
+fn array_has_dispatch_for_array<'a>(
+ haystack: &ArrayWrapper<'a>,
Review Comment:
This I find a bit interesting considering `ArrayWrapper` is already an enum over references 🤔
##########
datafusion/functions-nested/src/map.rs:
##########
@@ -126,10 +126,10 @@ fn get_first_array_ref(columnar_value: &ColumnarValue) -> Result<ArrayRef> {
}
fn make_map_batch_internal(
- keys: ArrayRef,
- values: ArrayRef,
+ keys: &ArrayRef,
+ values: &ArrayRef,
Review Comment:
Are there places where `&dyn Array` might work instead of `&ArrayRef`? Or do we need to keep the `Arc`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: enforce clippy lint needless_pass_by_value to datafusion-functions-nested [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #18839:
URL: https://github.com/apache/datafusion/pull/18839#discussion_r2550670080
##########
datafusion/functions-nested/src/map.rs:
##########
@@ -126,10 +126,10 @@ fn get_first_array_ref(columnar_value: &ColumnarValue) -> Result<ArrayRef> {
}
fn make_map_batch_internal(
- keys: ArrayRef,
- values: ArrayRef,
+ keys: &ArrayRef,
+ values: &ArrayRef,
Review Comment:
Looking at the code below, it needs the `Arc` to call `Arc::clone`
##########
datafusion/functions-nested/src/set_ops.rs:
##########
@@ -346,7 +346,7 @@ fn generic_set_lists<OffsetSize: OffsetSizeTrait>(
l: &GenericListArray<OffsetSize>,
r: &GenericListArray<OffsetSize>,
field: Arc<Field>,
- set_op: SetOp,
+ set_op: &SetOp,
Review Comment:
since you added Copy to `SetOp` I think you can revert these changes as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: enforce clippy lint needless_pass_by_value to datafusion-functions-nested [datafusion]
Posted by "foskey51 (via GitHub)" <gi...@apache.org>.
foskey51 commented on code in PR #18839:
URL: https://github.com/apache/datafusion/pull/18839#discussion_r2550425593
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -607,8 +607,8 @@ where
fn string_to_array_inner_3<'a, StringArrType, DelimiterArrType, StringBuilderType>(
args: &'a [ArrayRef],
- string_array: StringArrType,
- delimiter_array: DelimiterArrType,
+ string_array: &StringArrType,
+ delimiter_array: &DelimiterArrType,
Review Comment:
But since rust performs de-ref coercion implicitly, so it shouldn't be a problem. I had the same question initially, so I went through some sources and ended up learning about it.
[1] https://stackoverflow.com/a/28552082
[2] https://users.rust-lang.org/t/double-references-what-they-mean/56668/2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: enforce clippy lint needless_pass_by_value to datafusion-functions-nested [datafusion]
Posted by "foskey51 (via GitHub)" <gi...@apache.org>.
foskey51 commented on code in PR #18839:
URL: https://github.com/apache/datafusion/pull/18839#discussion_r2550396690
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -607,8 +607,8 @@ where
fn string_to_array_inner_3<'a, StringArrType, DelimiterArrType, StringBuilderType>(
args: &'a [ArrayRef],
- string_array: StringArrType,
- delimiter_array: DelimiterArrType,
+ string_array: &StringArrType,
+ delimiter_array: &DelimiterArrType,
Review Comment:
yes, and since rust handles this de-ref coercion implicitly, so it shouldn't be a problem. I had the same question initially, but after digging through a few sources
[1] https://stackoverflow.com/a/28552082
[2] https://users.rust-lang.org/t/double-references-what-they-mean/56668/2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: enforce clippy lint needless_pass_by_value to datafusion-functions-nested [datafusion]
Posted by "foskey51 (via GitHub)" <gi...@apache.org>.
foskey51 commented on code in PR #18839:
URL: https://github.com/apache/datafusion/pull/18839#discussion_r2550416640
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -607,8 +607,8 @@ where
fn string_to_array_inner_3<'a, StringArrType, DelimiterArrType, StringBuilderType>(
args: &'a [ArrayRef],
- string_array: StringArrType,
- delimiter_array: DelimiterArrType,
+ string_array: &StringArrType,
+ delimiter_array: &DelimiterArrType,
Review Comment:
But since rust performs de-ref coercion implicitly, so it shouldn't be a problem. I had the same question initially, but after digging through some sources, it seems expected behavior.
[1] https://stackoverflow.com/a/28552082
[2] https://users.rust-lang.org/t/double-references-what-they-mean/56668/2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: enforce clippy lint needless_pass_by_value to datafusion-functions-nested [datafusion]
Posted by "foskey51 (via GitHub)" <gi...@apache.org>.
foskey51 commented on code in PR #18839:
URL: https://github.com/apache/datafusion/pull/18839#discussion_r2551487438
##########
datafusion/functions-nested/src/map.rs:
##########
@@ -126,10 +126,10 @@ fn get_first_array_ref(columnar_value: &ColumnarValue) -> Result<ArrayRef> {
}
fn make_map_batch_internal(
- keys: ArrayRef,
- values: ArrayRef,
+ keys: &ArrayRef,
+ values: &ArrayRef,
Review Comment:
but ArrayRef type alias shows that
> pub type ArrayRef = Arc\<dyn Array>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: enforce clippy lint needless_pass_by_value to datafusion-functions-nested [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #18839:
URL: https://github.com/apache/datafusion/pull/18839#discussion_r2551637050
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -607,8 +607,8 @@ where
fn string_to_array_inner_3<'a, StringArrType, DelimiterArrType, StringBuilderType>(
args: &'a [ArrayRef],
- string_array: StringArrType,
- delimiter_array: DelimiterArrType,
+ string_array: &StringArrType,
+ delimiter_array: &DelimiterArrType,
Review Comment:
Thanks for checking this. I still find it odd and see it as a false positive by clippy, but I guess its easier to just follow the lint than manually ignoring it 🙁
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: enforce clippy lint needless_pass_by_value to datafusion-functions-nested [datafusion]
Posted by "foskey51 (via GitHub)" <gi...@apache.org>.
foskey51 commented on code in PR #18839:
URL: https://github.com/apache/datafusion/pull/18839#discussion_r2550331403
##########
datafusion/functions-nested/src/set_ops.rs:
##########
@@ -428,7 +428,7 @@ fn generic_set_lists<OffsetSize: OffsetSizeTrait>(
fn general_set_op(
array1: &ArrayRef,
array2: &ArrayRef,
- set_op: SetOp,
+ set_op: &SetOp,
Review Comment:
right !! I've marked SetOp enum with `#[derive(Copy, Clone)]`
##########
datafusion/functions-nested/src/array_has.rs:
##########
@@ -317,8 +317,8 @@ impl<'a> ArrayWrapper<'a> {
}
}
-fn array_has_dispatch_for_array(
- haystack: ArrayWrapper<'_>,
+fn array_has_dispatch_for_array<'a>(
+ haystack: &ArrayWrapper<'a>,
Review Comment:
marked `ArrayWrapper` enum with `#[derive(Copy, Clone)]`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: enforce clippy lint needless_pass_by_value to datafusion-functions-nested [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #18839:
URL: https://github.com/apache/datafusion/pull/18839
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: enforce clippy lint needless_pass_by_value to datafusion-functions-nested [datafusion]
Posted by "foskey51 (via GitHub)" <gi...@apache.org>.
foskey51 commented on code in PR #18839:
URL: https://github.com/apache/datafusion/pull/18839#discussion_r2550396690
##########
datafusion/functions-nested/src/string.rs:
##########
@@ -607,8 +607,8 @@ where
fn string_to_array_inner_3<'a, StringArrType, DelimiterArrType, StringBuilderType>(
args: &'a [ArrayRef],
- string_array: StringArrType,
- delimiter_array: DelimiterArrType,
+ string_array: &StringArrType,
+ delimiter_array: &DelimiterArrType,
Review Comment:
yes, and since rust handles this de-ref coercion implicitly, so it shouldn't be a problem. I had the same question initially, but after digging through a few sources
[1] https://stackoverflow.com/a/28552082
[2] https://users.rust-lang.org/t/double-references-what-they-mean/56668/2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org