Re: [PR] Enable `used_underscore_binding` clippy lint [datafusion]
alamb merged PR #15189: URL: https://github.com/apache/datafusion/pull/15189 -- 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-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] Enable `used_underscore_binding` clippy lint [datafusion]
Shreyaskr1409 commented on code in PR #15189:
URL: https://github.com/apache/datafusion/pull/15189#discussion_r1994200344
##
datafusion/optimizer/src/eliminate_limit.rs:
##
@@ -77,6 +77,7 @@ impl OptimizerRule for EliminateLimit {
} else if matches!(limit.get_skip_type()?,
SkipType::Literal(0)) {
// If fetch is `None` and skip is 0, then Limit takes no
effect and
// we can remove it. Its input also can be Limit, so we
should apply again.
+#[allow(clippy::used_underscore_binding)]
Review Comment:
I am getting an error as:
```
error: parameter is only used in recursion
--> datafusion\optimizer\src\eliminate_limit.rs:59:9
|
59 | config: &dyn OptimizerConfig,
| ^^ help: if this is intentional, prefix it with an
underscore: `_config`
|
note: parameter used here
--> datafusion\optimizer\src\eliminate_limit.rs:81:76
|
81 | return
self.rewrite(Arc::unwrap_or_clone(limit.input), config);
|
^^
= help: for further information visit
https://rust-lang.github.io/rust-clippy/master/index.html#only_used_in_recursion
= note: `-D clippy::only-used-in-recursion` implied by `-D warnings`
= help: to override `-D warnings` add
`#[allow(clippy::only_used_in_recursion)]
```
That was the reason why I made underscore allowed in this case. Do you have
any suggestions to fix this part in any other way?
--
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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] Enable `used_underscore_binding` clippy lint [datafusion]
Shreyaskr1409 commented on code in PR #15189:
URL: https://github.com/apache/datafusion/pull/15189#discussion_r1994203750
##
datafusion/physical-plan/src/joins/nested_loop_join.rs:
##
@@ -75,21 +75,22 @@ struct JoinLeftData {
probe_threads_counter: AtomicUsize,
/// Memory reservation for tracking batch and bitmap
/// Cleared on `JoinLeftData` drop
-_reservation: MemoryReservation,
+#[allow(dead_code)]
+reservation: MemoryReservation,
Review Comment:
ooh, this is nice
--
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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] Enable `used_underscore_binding` clippy lint [datafusion]
Shreyaskr1409 commented on code in PR #15189:
URL: https://github.com/apache/datafusion/pull/15189#discussion_r1994201360
##
datafusion/optimizer/src/push_down_filter.rs:
##
@@ -799,6 +799,7 @@ impl OptimizerRule for PushDownFilter {
new_predicate,
child_filter.input,
)?);
+#[allow(clippy::used_underscore_binding)]
Review Comment:
The reason for allowing underscore here is the same as in eliminate_limit.rs
--
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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
Re: [PR] Enable `used_underscore_binding` clippy lint [datafusion]
alamb commented on code in PR #15189:
URL: https://github.com/apache/datafusion/pull/15189#discussion_r1992221050
##
datafusion/optimizer/src/push_down_filter.rs:
##
@@ -799,6 +799,7 @@ impl OptimizerRule for PushDownFilter {
new_predicate,
child_filter.input,
)?);
+#[allow(clippy::used_underscore_binding)]
Review Comment:
likewise here, I suggest we rename the variable rather than allow the lint
##
datafusion/physical-plan/src/joins/nested_loop_join.rs:
##
@@ -75,21 +75,22 @@ struct JoinLeftData {
probe_threads_counter: AtomicUsize,
/// Memory reservation for tracking batch and bitmap
/// Cleared on `JoinLeftData` drop
-_reservation: MemoryReservation,
+#[allow(dead_code)]
+reservation: MemoryReservation,
Review Comment:
```suggestion
/// reservation is cleared on Drop
#[expect(dead_code)]
reservation: MemoryReservation,
```
Also if you use `expect(` rather than `allow` clippy will error if the code
ever changes (and we can remove the unecessary annotation). Allow will still run
##
datafusion/physical-optimizer/src/aggregate_statistics.rs:
##
@@ -45,7 +45,7 @@ impl PhysicalOptimizerRule for AggregateStatistics {
fn optimize(
&self,
plan: Arc,
-_config: &ConfigOptions,
+config: &ConfigOptions,
Review Comment:
👍
##
datafusion/optimizer/src/eliminate_limit.rs:
##
@@ -77,6 +77,7 @@ impl OptimizerRule for EliminateLimit {
} else if matches!(limit.get_skip_type()?,
SkipType::Literal(0)) {
// If fetch is `None` and skip is 0, then Limit takes no
effect and
// we can remove it. Its input also can be Limit, so we
should apply again.
+#[allow(clippy::used_underscore_binding)]
Review Comment:
I think instead oif allowing this, we should change the name of the argument
to `config` as it is actually used
--
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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
