You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "tustvold (via GitHub)" <gi...@apache.org> on 2023/02/22 16:24:06 UTC
[GitHub] [arrow-datafusion] tustvold opened a new issue, #4695: PruningPredicate Use Physical not Logical Predicate
tustvold opened a new issue, #4695:
URL: https://github.com/apache/arrow-datafusion/issues/4695
**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
--
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-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on issue #4695: PruningPredicate Use Physical not Logical Predicate
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on issue #4695:
URL: https://github.com/apache/arrow-datafusion/issues/4695#issuecomment-1440352059
I believe @crepererum plans to pick this one up
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on issue #4695: PruningPredicate Use Physical not Logical Predicate
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #4695:
URL: https://github.com/apache/arrow-datafusion/issues/4695#issuecomment-1441708658
If anyone is going ti significantly revamp the pruning predicate logic, let's 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: https://github.com/apache/arrow-datafusion/pull/5322#discussion_r1114324054
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 work with the range analysis
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on issue #4695: PruningPredicate Use Physical not Logical Predicate
Posted by GitBox <gi...@apache.org>.
alamb commented on issue #4695:
URL: https://github.com/apache/arrow-datafusion/issues/4695#issuecomment-1362058253
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)
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb closed issue #4695: PruningPredicate Use Physical not Logical Predicate
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed issue #4695: PruningPredicate Use Physical not Logical Predicate
URL: https://github.com/apache/arrow-datafusion/issues/4695
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] ozankabak commented on issue #4695: PruningPredicate Use Physical not Logical Predicate
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on issue #4695:
URL: https://github.com/apache/arrow-datafusion/issues/4695#issuecomment-1442312049
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.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on issue #4695: PruningPredicate Use Physical not Logical Predicate
Posted by GitBox <gi...@apache.org>.
tustvold commented on issue #4695:
URL: https://github.com/apache/arrow-datafusion/issues/4695#issuecomment-1362833034
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
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on issue #4695: PruningPredicate Use Physical not Logical Predicate
Posted by GitBox <gi...@apache.org>.
tustvold commented on issue #4695:
URL: https://github.com/apache/arrow-datafusion/issues/4695#issuecomment-1362087410
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 😅
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold closed issue #4695: PruningPredicate Use Physical not Logical Predicate
Posted by GitBox <gi...@apache.org>.
tustvold closed issue #4695: PruningPredicate Use Physical not Logical Predicate
URL: https://github.com/apache/arrow-datafusion/issues/4695
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] crepererum commented on issue #4695: PruningPredicate Use Physical not Logical Predicate
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on issue #4695:
URL: https://github.com/apache/arrow-datafusion/issues/4695#issuecomment-1441737909
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.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org