Skip to content

[FLINK-40399][state] Separate SavepointKeyFilter runtime contract from push-down planning - #28982

Open
soin08 wants to merge 3 commits into
apache:masterfrom
soin08:FLINK-40399
Open

[FLINK-40399][state] Separate SavepointKeyFilter runtime contract from push-down planning#28982
soin08 wants to merge 3 commits into
apache:masterfrom
soin08:FLINK-40399

Conversation

@soin08

@soin08 soin08 commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

What is the purpose of the change

SavepointKeyFilter carries two unrelated responsibilities: the contract the savepoint scan needs (test, getExactKeys), and the algebra used only while combining predicates during filter push-down (isEmpty, getLowerBound, getUpperBound, intersect, filterKeys, empty, plus BoundInfo).

The second group is public despite being unusable outside push-down translation, to the point that its javadoc has to disclaim it: "Used only while combining filters during push-down translation, not during the scan." Anyone implementing the interface for the DataStream API has to read past six methods that do not apply to them.

This separates the two. There is no behaviour change.

Brief change log

  • org.apache.flink.state.api.filter.SavepointKeyFilter keeps only the runtime contract, test(K) and getExactKeys(), plus the exact(..) / range(..) factories.
  • The planning algebra moves to a new @Internal SavepointKeyFilterPlan in org.apache.flink.state.table.filter, next to the savepoint connector that is its only user. BoundInfo and EmptyKeyFilter move with it, and RangeKeyFilter sheds the intersection logic it only needed for push-down.
  • SavepointFilterTranslator is retargeted to the new type.
  • The unit tests are split the same way: plan-algebra cases move to a new SavepointKeyFilterPlanTest, leaving SavepointFilterTranslatorTest to cover only expression translation.
  • Adds coverage for push-down paths that had no test at all (see below).
  • docs/content/docs/libs/state_processor_api.md and its content.zh counterpart drop the SavepointKeyFilter.empty() factory and the reference to "remaining interface methods", neither of which exists on the public interface any more.

Verifying this change

This change is already covered by existing tests, such as SavepointFilterTranslatorTest, SavepointDynamicTableSourceTest and SavepointReaderKeyedStateITCase, which exercise filter push-down end to end and are unchanged in behaviour.

It also adds tests for behaviour that had no coverage:

  • Predicates that cannot be pushed are returned in remaining() so the runtime still evaluates them. Nothing covered this before: every apply() case asserted remaining() was empty, so a regression here would have silently returned extra rows.
  • An untranslatable child aborts the whole AND / OR rather than being skipped.
  • key = NULL, whose literal has no readable value.
  • Non-Comparable literals on a BYTES key, covering the two guards in fromBetween and fromComparison that no test reached.
  • The two comparison flip directions that were missing (GTE, LT), and comparison arity.
  • At SQL level, the upper-bound predicates < and <=, which had no end-to-end test at all, plus AND of two ranges and a literal-on-the-left comparison.

Test counts after this PR: SavepointFilterTranslatorTest 54, SavepointKeyFilterPlanTest 9 (new), SavepointDynamicTableSourceTest 19. Every test now states the predicate under test as a leading comment, so the SQL shape being exercised is readable without decoding the expression builders.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changed class annotated with @Public(Evolving): no. SavepointKeyFilter is @Experimental. Six methods are removed from it and moved to an @Internal type in the connector package; implementers only ever needed test and getExactKeys, both unchanged.
  • The serializers: no
  • The runtime per-record code paths (performance sensitive): no
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
  • The S3 file system connector: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? docs — the State Processor API page is updated in both content and content.zh to drop the removed factory method and the stale note about internal interface methods.

Was generative AI tooling used to co-author this PR?

  • Yes (please specify the tool below)

Generated-by: Claude Code (Claude Opus 5)

Ilya Soin added 2 commits August 16, 2026 23:46
…m push-down planning

SavepointKeyFilter carried two unrelated responsibilities: the contract the
savepoint scan needs, and the algebra used only while combining predicates
during filter push-down. The second group was public and @experimental despite
being unusable outside push-down translation, to the point that its javadoc had
to disclaim it - "Used only while combining filters during push-down
translation, not during the scan."

Split the two:

  * org.apache.flink.state.api.filter.SavepointKeyFilter keeps only what the
    scan needs - test() and getExactKeys() - plus the public factories.
  * org.apache.flink.state.table.filter.SavepointKeyFilterPlan carries the
    planning algebra (isEmpty, bounds, intersect, filterKeys, empty), next to
    the connector that is its only user.

SavepointKeyFilterPlan and BoundInfo are public only so the connector in
org.apache.flink.state.table can reach them, and are marked @internal.

The unit tests are split the same way: the plan algebra cases move to
SavepointKeyFilterPlanTest, leaving SavepointFilterTranslatorTest to cover only
expression translation.

No behaviour change.
Adds cases for savepoint key filter push-down behaviour that had no test, most
importantly that predicates which cannot be pushed are returned in remaining()
so the runtime still evaluates them. Nothing covered that before: every apply()
case asserted remaining() was empty, so a regression there would have silently
dropped rows.

Also covered: an untranslatable child aborting the whole AND/OR, "key = NULL"
whose literal has no readable value, non-Comparable literals on a BYTES key,
the two comparison flip directions that were missing, comparison arity, and at
SQL level the upper-bound predicates, which had no end-to-end test at all.

Finally, every test states the predicate under test as a leading comment, so
the SQL shape being exercised is readable without decoding the expression
builders.
@flinkbot

flinkbot commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@soin08 soin08 changed the title Flink 40399 [FLINK-40399][state] Separate SavepointKeyFilter runtime contract from push-down planning Aug 16, 2026
}

@Test
void testOrOfExactAndRangeOnKeyIsNotPushedDownButReturnsCorrectResult() throws Exception {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will be supported in the next MR

}

@Test
void testOrOfTwoRangesOnKeyIsNotPushedDownButReturnsCorrectResult() throws Exception {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also will be supported in the next MR

@soin08
soin08 marked this pull request as ready for review August 17, 2026 12:46
@github-actions github-actions Bot added the community-reviewed PR has been reviewed by the community. label Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-reviewed PR has been reviewed by the community.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants