laserninja commented on PR #16110: URL: https://github.com/apache/iceberg/pull/16110#issuecomment-4349466977
Yes, I agree NOOP is the wrong semantic. NOOP means "pass everything through", which is the opposite of AlwaysFalse. My rationale (that Iceberg's manifest filtering would have already excluded all relevant files) is an assumption about callers that shouldn't be baked into this method. The challenge is that Parquet's FilterApi doesn't expose a native alwaysFalse() predicate. A few approaches I can see: Visitor-level fix — have ConvertFilterToParquet.alwaysFalse() return a UserDefinedPredicate that always returns false. This keeps the semantics correct at the Parquet level but requires picking a concrete column type. Propagate as a special case — detect the AlwaysFalse.INSTANCE sentinel in convert() and return a RecordFilter / throw, making it the caller's responsibility to avoid this path. Open to your suggestion — is there an existing pattern in the codebase you'd prefer? I'll update the PR with whichever direction makes most sense. Happy to hear your preference. -- 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]
