xudong963 commented on issue #22883: URL: https://github.com/apache/datafusion/issues/22883#issuecomment-5861859580
Thanks for working on this. I read the full [Optional Filters design document](https://claude.ai/artifact/SSz7t6hPyhFWp1MDPecVqt). **Overall, the direction is worth pursuing and the document is very clear, love it** Its strongest idea is separating two questions: whether a filter is required for correctness, and whether evaluating it is worthwhile on the current data. Having the producer mark a filter as `Optional` and the consumer make the runtime decision gives each side a clear responsibility. -- very clear design and abstract; Allowing a filter to be skipped only when it appears on the root `AND` chain also addresses the correctness risks posed by `NOT`, `OR`, and other expression contexts. I would raise some points in review: 1. **Make plan properties an explicit correctness prerequisite.** A scan that uses a predicate only for statistics pruning must not claim that every output row satisfies it. The recently reported [#25779](https://github.com/apache/datafusion/issues/25779) demonstrates that an invalid equivalence claim can produce wrong results for `ORDER BY … LIMIT`; 2. The latest design has moved beyond the simulator’s pass-ratio threshold to comparing evaluation cost with downstream savings. However, downstream savings for TopK and aggregate filters still rely on an estimate; 3. **Resolve the API choice.** The `Optional(...)` expression wrapper limits the initial changes, but it also enters expression rewriting, serialization, and plan-property paths. [#25760](https://github.com/apache/datafusion/pull/25760) proposes storing optionality in a `FilterConjunct` instead. My preference is to use the wrapper for this experimental stack. For a longer-lived public mechanism, I would favor FilterConjunct once the expression and its metadata can travel through rewrites as one unit, without relying solely on positional reassociation. I see there are many existing PRs related to the topic; could you list them in order? That way, it'll be easier for everyone else to track and review them. -- 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]
