crepererum commented on code in PR #15995:
URL: https://github.com/apache/datafusion/pull/15995#discussion_r2081340054
##########
datafusion/physical-optimizer/src/pruning.rs:
##########
@@ -3585,12 +3605,10 @@ mod tests {
prune_with_expr(
// false
- // constant literals that do NOT refer to any columns are
currently not evaluated at all, hence the result is
- // "all true"
Review Comment:
The docstring says:
```rust
/// Translate logical filter expression into pruning predicate
/// expression that will evaluate to FALSE if it can be determined no
/// rows between the min/max values could pass the predicates.
```
Hence I think the existing behavior was "correct", but not "precise". Like
even if the method would just always return `true`, it would be "correct" but
maximally imprecise.
So the "improvement" here (technically not a "fix") is to check the constant
`lit(false)` case explicitly.
##########
datafusion/physical-optimizer/src/pruning.rs:
##########
@@ -1538,13 +1550,21 @@ fn build_predicate_expression(
build_predicate_expression(&right, schema, required_columns,
unhandled_hook);
// simplify boolean expression if applicable
let expr = match (&left_expr, op, &right_expr) {
+ (left, Operator::And, right)
Review Comment:
I think we should leave the and/or folding to a dedicated optimizer pass
(IIRC that's "expression simplification") instead of re-implementing it here
again. It's unnecessarily complex, potentially slow, requires more tests to
write and maintain, and is a potential source of additional bugs.
--
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]
