adriangb commented on code in PR #21806:
URL: https://github.com/apache/datafusion/pull/21806#discussion_r3170578419
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -814,17 +814,15 @@ impl CaseBody {
}
}?;
- // `true_count` ignores `true` values where the validity bit is
not set, so there's
- // no need to call `prep_null_mask_filter`.
- let when_true_count = when_value.true_count();
-
- // If the 'when' predicate did not match any rows, continue to the
next branch immediately
- if when_true_count == 0 {
+ // If the 'when' predicate did not match any rows, continue to the
next branch immediately.
+ // Like the old `true_count == 0` guard: `has_true()` only counts
valid slots that are true
+ // (masked-null predicate slots are ignored), so no
`prep_null_mask_filter` needed here.
Review Comment:
This seems like classic LLM mistake: I don't think we should have code
comments referencing "the old implementation". IMO those should be comments on
the PR to help the reviewer, but they are not helpful to future readers of the
code. Code comments should stick to being targeted at future readers. So maybe
something like:
```suggestion
// Only counts valid slots that are true (masked-null predicate
slots are ignored),
// so no `prep_null_mask_filter` needed here.
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
