Repository navigation
PruningPredicate Use Physical not Logical Predicate #4695
Description
Is your feature request related to a problem or challenge? Please describe what you are trying to do.
Related to #4693, currently PruningPredicate accepts a logical Expr which it then converts to a PhysicalExpr. The comments appear to indicate this at one point related to serialization. However, looking at the code in datafusion-proto, we appear to be able to serialize physical expressions.
Describe the solution you'd like
I would like PruningPredicate to be created with a PhysicalExpr instead of Expr, this would in turn mean ParquetExec would be created with PhysicalExpr instead of Expr.
Not only would this avoid issues like #4693 but would also be more consistent for the physical plan to contain PhysicalExpr, and not logical Expr
Describe alternatives you've considered
Additional context
This relates to #4629 which tracks making ExecutionProps a trait to ensure all stages of planning/execution take place against the same configuration.
FYI @alamb in case there is something fundamental I have missed here
Activity
alamb commentedon Dec 21, 2022
I think using PhysicalExpr would be fine for pruning predicates.
If we are going to rewrite pruning anyways, I think we should consider changing the approach -- rather than doing algebriac rewrite I would love to change it to a range analysis -- using the base that @isidentical has created (for predicate selectivity analysis)
tustvold commentedon Dec 21, 2022
This wouldn't require a rewrite, it already uses PhysicalExpr internally, it is just its external interface is Expr. This is the cause of the issue 😅
tustvold commentedon Dec 22, 2022
I didn't dig deep enough into the code, it is actually performing a rewrite of the
Exprwhich would require a non-trivial rewrite to port to PhysicalExprtustvold commentedon Feb 22, 2023
I believe @crepererum plans to pick this one up
alamb commentedon Feb 23, 2023
If anyone is going to significantly revamp the pruning predicate logic, I recommend to please coordinate with @mingmwang @ozankabak @metesynnada -- it turns out there is already another range analysis implementation (other than PruningPredicate) and we are in discussions about possibly adding a third: #5322 (comment)
If possible it would be great to avoid investing a lot of time into PruningPredicate only to then have to do some other major surgery on it to avoid duplication with the range analysis
The other two range analysis are based on PhysicalExpr
crepererum commentedon Feb 23, 2023
It's not really a revamp. It's mostly a straight-forward porting of the
PruningPredicateandParquetExecfrom logical to phys. expressions. I would like to perform this port first before compressing/de-duping the two implementations. I agree that ideally there should be a single way to express ranges etc.ozankabak commentedon Feb 23, 2023
Sounds good, if it is a straightforward porting it makes sense to do it early. We can subsequently merge them in the upcoming weeks as we mature the interval arithmetic library.
fix: display of nested phys. binary expr should use parenthesis
fix: misc phys. expression display bugs (#5387)
refactor: `ParquetExec` logical expr. => phys. expr.
refactor: `ParquetExec` logical expr. => phys. expr.
refactor: `ParquetExec` logical expr. => phys. expr.
refactor: ParquetExec logical expr. => phys. expr. (#5419)