gaborgsomogyi commented on PR #28982: URL: https://github.com/apache/flink/pull/28982#issuecomment-5509390055
Thanks for the efforts! Shrinking the public API surface here is the right direction, good call splitting the runtime contract from the push-down-only algebra. That said, Flink's existing filter push-down connectors (e.g. `SupportsFilterPushDown` + `FileSystemTableSource`) generally avoid a second parallel class hierarchy for this: they keep one flat, already-serializable representation of a filter, computed once during push-down, and pass that same object through to runtime rather than shipping a richer "planning" type alongside a "runtime" type. Right now `test()` is duplicated verbatim between `RangeKeyFilter`/`RangeKeyFilterPlan` and `ExactKeyFilter`/`ExactKeyFilterPlan`, and since the SQL path only exercises the `*Plan` variants, a future fix to one won't be caught by anything and will just quietly drift from the other. Could this be done without the duplication by keeping the intersect/bounds combining logic as private computation inside `SavepointFilterTranslator` (plain local state while walking the expression tree), and only constructing the existing `SavepointKeyFilter.range()/.exact()` once at the end, so there's a single filter implementation used by both the SQL and direct API paths? What's the reasoning for a separate `*Plan` hierarchy instead? -- 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]
