ZENOTME commented on issue #1355: URL: https://github.com/apache/iceberg-rust/issues/1355#issuecomment-2967494138
> I haven't verified, but it does sound plausible to me that `rewrite_not` is always called before running `ManifestEvaluator::New`. > > If that were the case, I would be inclined to change the implementation for the `manifest_evaluator.rs` to return `ROWS_MIGHT_MATCH` with an explanatory comment that: > > 1. This is necessary for correctness, because of the `not(x < 3)` case for data `x = [1, 2, 4, 5]` > 2. But it doesn't cause over-scanning because in practice the `not()` predicates are always removed by the `rewrite_not` transformation > > This keeps the implementation of `ManifestEvaluator::New` independently correct in case it's ever (accidentally?) used with a predicate that has a `not()` in it, but also explains why it doesn't need to be made more complex to accommodate them. I think directly refuse to process the not may make the semantics clearer🤔 1. call `rewrite_not` in ManifestEvaluator::New to gurantee that there is not possible `not` in expression 2. return Error in `not` visitor -- 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]
