Add alias-based dynamic filter assertions for plan matching#28334
Add alias-based dynamic filter assertions for plan matching#28334piotrrzysko wants to merge 4 commits intotrinodb:masterfrom
Conversation
Bundling parameters into a context object allows adding new fields without modifying all Matcher implementations.
Previously, dynamic filter assertions either checked only the count
of filters (numberOfDynamicFilters) or matched filters by expression
without verifying the connection between producers and consumers.
This made it possible for tests to pass even when dynamic filters
were incorrectly wired.
The new API uses test-defined aliases to explicitly link producers
(at join nodes) with consumers (at filter nodes):
join(INNER, builder -> builder
.dynamicFilters(df -> df.addProducer("DF", "BUILD_COL"))
.left(filter(TRUE,
df -> df.addConsumer(c -> c.alias("DF").expression(BIGINT, "PROBE_COL")),
tableScan(...)))
.right(...))
During plan matching, the framework verifies that each alias maps
to exactly one dynamic filter ID, ensuring producers and consumers
are correctly paired.
2c3adda to
5c978fa
Compare
| join(INNER, builder -> builder | ||
| .equiCriteria("ORDERS_OK", "LINEITEM_OK") | ||
| .dynamicFilter(BIGINT, "ORDERS_OK", "LINEITEM_OK") | ||
| .dynamicFilters(dynamicFilters -> dynamicFilters.addProducer("DF", "LINEITEM_OK")) |
There was a problem hiding this comment.
Before the change, in the call we were providing 2 symbol names. They were oddly named ("key" and "value"), but apparently they were coming from left and right side.
After the change, we provide only the one from right value. Why is that?
| join(INNER, builder -> builder | ||
| .equiCriteria("ORDERS_OK", "LINEITEM_OK") | ||
| .dynamicFilter(BIGINT, "ORDERS_OK", "LINEITEM_OK") | ||
| .dynamicFilters(dynamicFilters -> dynamicFilters.addProducer("DF", "LINEITEM_OK")) |
There was a problem hiding this comment.
API choice
It appears that after .dynamicFilters(dynamicFilters -> dynamicFilters. the only think that can be called is addProducer(String alias, String buildAlias).
Do you envision many other options in the future?
Cause it seems we could cut of dynamicFilters -> dynamicFilters.addProducer from the callsite, and thus make it significantly simpler to read and write.
| PlanMatchPattern.filter( | ||
| TRUE, | ||
| createDynamicFilterExpression(metadata, new DynamicFilterId("DF"), BIGINT, new Reference(BIGINT, "ORDERS_OK")), | ||
| dynamicFilters -> dynamicFilters.addConsumer(consumer -> consumer.alias("DF").expression(BIGINT, "ORDERS_OK")), |
There was a problem hiding this comment.
API design
After dynamicFilters -> dynamicFilters. there are two options: a consumer, or no consumer.
It could be also done without a lambda over a builder, like this
filter(
«static filter condition matcher»,
«dynamic filter matcher (not builder)»,
«source»)
where «dynamic filter matcher (not builder)» is one of
noDynamicFilters()dynamicFilter("DF", BIGINT, "ORDERS_OK")
|
cc @sopel39 |
There was a problem hiding this comment.
Pull request overview
This PR updates Trino’s plan-matcher test framework to support alias-based dynamic filter assertions, allowing tests to explicitly link dynamic filter producers (Join/SemiJoin) to their consuming FilterNodes. It also refactors matcher APIs to pass a unified MatchContext, enabling dynamic-filter candidate propagation during bottom-up matching.
Changes:
- Introduce dynamic-filter alias tracking (
DynamicFilterAlias,DynamicFilterConsumerMatcher,DynamicFilterProducers,MatchingDynamicFilters) and enforce alias resolution inPlanAssert. - Refactor matcher plumbing to use
MatchContextand thread dynamic-filter candidate sets throughPlanMatchingVisitor/MatchResult. - Migrate existing plan tests and matchers to the new dynamic filter and matcher APIs.
Reviewed changes
Copilot reviewed 63 out of 63 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| core/trino-main/src/test/java/io/trino/sql/planner/optimizations/TestUnaliasSymbolReferences.java | Migrates dynamic filter assertions to alias-based producer/consumer patterns. |
| core/trino-main/src/test/java/io/trino/sql/planner/optimizations/TestRemoveUnsupportedDynamicFilters.java | Updates tests to assert no DF consumers via the new consumer matcher API. |
| core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestDetermineTableScanNodePartitioning.java | Updates custom matcher signature to use MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/WindowMatcher.java | Switches matcher to MatchContext and uses context.symbolAliases(). |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/ValuesMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/UnnestMatcher.java | Switches matcher to MatchContext and updates alias resolution usage. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/TopNRankingMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/TopNMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/TableWriterMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/TableScanMatcher.java | Switches matcher to MatchContext and routes metadata/session through context. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/TableFunctionProcessorMatcher.java | Switches matcher to MatchContext and updates symbol alias mapping. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/TableFunctionMatcher.java | Switches matcher to MatchContext and updates symbol alias mapping. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/TableExecuteMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/SymbolCardinalityMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/StatsOutputRowCountMatcher.java | Switches matcher to MatchContext and pulls stats via context.stats(). |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/SpatialJoinMatcher.java | Switches matcher to MatchContext and updates expression verification. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/SortMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/SemiJoinMatcher.java | Reworks semi-join DF matching to use alias-based candidate resolution via MatchingDynamicFilters. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/SemiJoinDynamicFilterProducer.java | New helper type to express semi-join DF expectations (ignore / none / alias). |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/RowNumberMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/RemoteSourceMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/PredicateMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/PlanNodeMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/PlanMatchingVisitor.java | Threads dynamic-filter matching state through matching/alias propagation. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/PlanMatchPattern.java | Updates DF APIs (filter, semiJoin, join DF producers) and removes old DF pattern type. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/PlanAssert.java | Adds final assertion that all DF aliases resolve to a single unique ID. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/PatternRecognitionMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/OutputMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/OffsetMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/NotPlanNodeMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/MatchingDynamicFilters.java | New alias→candidate-id intersection/merge structure for DF matching. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/Matcher.java | Changes matcher API to accept a single MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/MatchResult.java | Adds MatchingDynamicFilters to match results and new helpers. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/MatchContext.java | New unified context object passed to matchers. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/MarkDistinctMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/LimitMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/JoinMatcher.java | Replaces old DF expression matching with alias-based producer resolution. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/IndexSourceMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/IndexJoinMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/IdentityProjectionMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/GroupIdMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/FilterMatcher.java | Makes filter matching ignore dynamic conjuncts by extracting them first. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/ExchangeMatcher.java | Switches matcher to MatchContext and updates alias updates. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/DynamicFilterProducers.java | New join producer expectation container (alias→build symbol). |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/DynamicFilterConsumerMatcher.java | New filter-node consumer matcher that records DF candidates per alias. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/DynamicFilterAlias.java | New value type for test-defined DF aliases. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/DistinctMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/DistinctLimitMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/CorrelationMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/CorrelatedJoinMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/ConnectorAwareTableScanMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/BaseStrictSymbolsMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/AliasMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/AggregationStepMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/AggregationMatcher.java | Switches matcher to MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/assertions/AdaptivePlanMatcher.java | Switches matcher to MatchContext and propagates it when visiting initial plans. |
| core/trino-main/src/test/java/io/trino/sql/planner/TestPredicatePushdownWithoutDynamicFilter.java | Updates semi-join DF expectations to noDynamicFilter(). |
| core/trino-main/src/test/java/io/trino/sql/planner/TestPredicatePushdown.java | Migrates join/semi-join DF assertions to alias-based producer/consumer patterns. |
| core/trino-main/src/test/java/io/trino/sql/planner/TestLogicalPlanner.java | Migrates DF assertions away from removed DynamicFilterPattern. |
| core/trino-main/src/test/java/io/trino/sql/planner/TestInsert.java | Updates custom matcher signature to use MatchContext. |
| core/trino-main/src/test/java/io/trino/sql/planner/TestDynamicFilter.java | Migrates many DF plan assertions to alias-based producer/consumer patterns. |
| core/trino-main/src/test/java/io/trino/sql/planner/TestAddDynamicFilterSource.java | Migrates DF assertions to alias-based producer/consumer patterns. |
| core/trino-main/src/test/java/io/trino/sql/planner/AbstractPredicatePushdownTest.java | Migrates semi-join DF assertions to alias-based producer/consumer patterns with enable/disable switching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Set<DynamicFilterId> mergedCandidates = entry.getValue().stream() | ||
| .map(Candidates::candidates) | ||
| .reduce((candidates1, candidates2) -> Sets.intersection(candidates1, candidates2).immutableCopy()) | ||
| .orElseThrow(); | ||
| candidates.put(entry.getKey(), mergedCandidates); |
There was a problem hiding this comment.
MatchingDynamicFilters.Builder.build() can produce an empty mergedCandidates set when intersecting candidate sets for the same alias. That breaks Builder.add()'s non-empty invariant and can later make addAll()/merge() throw instead of returning NO_MATCH when aliases can't be reconciled across multiple FilterNodes. Consider treating an empty intersection as a match failure or ensuring build never emits empty candidate sets in a way that doesn't crash the matcher pipeline.
Description
Summary
This PR introduces a new approach for asserting dynamic filters in plan matcher tests. The new framework uses logical aliases to connect dynamic filter producers (at Join/SemiJoin nodes) with their consumers (at
FilterNode).Why?
The existing dynamic filter assertions lack explicit links between producers and consumers. Tests could only specify that a Join produces a dynamic filter and that somewhere in the left subtree there's a matching consumer—but couldn't explicitly assert which
FilterNodeconsumes a given dynamic filter. This made it impossible to verify correct DF placement in plans with multipleFilterNodesor multiple dynamic filters.Approach
Tests assign string aliases (e.g.,
"DF1") to dynamic filters. During bottom-up plan matching, consumer matchers atFilterNodescollect candidateDynamicFilterIds that match the expected pattern and associate them with the alias. As matching progresses up the tree, candidates are narrowed: multipleFilterNodeswith the same alias intersect their candidates, and the producer matcher at Join/SemiJoin resolves to exactly one ID by verifying the build symbol.API Change
Before
After
Additional context and related issues
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: