search

apache / iceberg-rust

Apache Iceberg
https://rust.iceberg.apache.org/
Apache License 2.0
474 stars 97 forks source link

add `InclusiveProjection` Visitor #335

Closed sdd closed 2 months ago

sdd commented 2 months ago

This PR has been broken out of https://github.com/apache/iceberg-rust/pull/241 as it was getting too large.

The InclusiveProjection is used in the process of filtering manifest files in table scans. It projects the BoundPredicate that is provided as the filter in a table scan into a Predicate that can be used for filtering the partition specs of manifests.

This also updates BoundPredicateVisitor to add a parameter consisting of a reference to the BoundPredicate itself to the unary/binary/set operation visitor methods, as a convenience. Without this, the implementation for the InclusiveProjection visitor would be much more complex and consist of more boilerplate, as it would need to construct a BoundPredicate itself in each method, in order to be able to pass a reference to a BoundPredicate to PartitionField.transform.project

sdd commented 2 months ago

@liurenjie1024 @Fokko @marvinlanhenke @ZENOTME PTAL, I've refactored this on top of the BoundPredicateVisitor that was merged a few hours ago and it is ready for review

sdd commented 2 months ago

FAO @Fokko @marvinlanhenke @liurenjie1024: Comments addressed, ready for re-review

sdd commented 2 months ago

@marvinlanhenke I agree that rewrite-not should get applied. But it is not the responsibility of the InclusiveProjection itself. In this design, it happens already earlier on on the process, at the point where the TableScan gets built.

See here, in my other open PR: https://github.com/apache/iceberg-rust/pull/323/files#diff-bbfbe5e334be6c501ba2ca0ddd84d658ff0f3f84f2b5b532212f4e096a09e09bR76

marvinlanhenke commented 2 months ago

This looks great!

I think @marvinlanhenke has a valid concern on when to apply the rewrite-not. Let's defer that discussion when we start wiring everything together. This look good, thanks @sdd for working on this and thanks @marvinlanhenke for the review 🙌

Thanks @sdd @Fokko ... I think its reasonable to defer the discussion, once we tie everything together.