PruningPredicate Use Physical not Logical Predicate

[tustvold]

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

[alamb]

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]

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]

I didn't dig deep enough into the code, it is actually performing a rewrite of the Expr which would require a non-trivial rewrite to port to PhysicalExpr

[tustvold]

I believe @crepererum plans to pick this one up

[alamb]

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]

It's not really a revamp. It's mostly a straight-forward porting of the PruningPredicate and ParquetExec from 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]

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.