planner: Add distinct to semi join inner - #66788
Conversation
ti-chi-bot
Bot
added
the
release-note-none
|
Review Complete Findings: 5 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
sig/planner
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @terry1purcell. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds LogicalJoin.SemiJoinInnerDedup to optionally replace the inner side of semi/anti-semi joins with a GROUP BY + FirstRow aggregation based on NDV heuristics and integrates this transformation into the physical plan enumeration; updates many planner test fixtures and expected plan outputs. Changes
Sequence Diagram(s)sequenceDiagram
participant Optimizer as Optimizer
participant LogicalJoin as LogicalJoin
participant Stats as StatsEstimator
participant PlanExhaust as PlanExhaustor
participant Physical as PhysicalPlanGen
Optimizer->>LogicalJoin: Identify semi/anti-semi join
LogicalJoin->>Stats: Derive join keys, NDV, row counts
alt Dedup applicable
LogicalJoin->>LogicalJoin: Pushable rightconds -> Selection
LogicalJoin->>LogicalJoin: Build AGG(GROUP BY keys, FirstRow)
LogicalJoin->>Stats: Derive agg stats
LogicalJoin->>PlanExhaust: Return rewritten inner child
else Not applicable
LogicalJoin->>PlanExhaust: Return original children
end
PlanExhaust->>Physical: Enumerate IndexJoin, MergeJoin, HashJoin (use rewritten inner if present)
Physical-->>Optimizer: Candidate physical plans
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/planner/core/casetest/testdata/json_plan_suite_out.json (1)
108-152:⚠️ Potential issue | 🟡 MinorPlease drop the runtime-only
EXPLAIN ANALYZEchurn from this golden.These lines only update
executeInfo/memoryInfofor an inner-joinEXPLAIN ANALYZEcase. Those fields are runtime-dependent and unrelated to the semi-join planner change, so committing them makes the golden noisier and more brittle.As per coding guidelines, "Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/casetest/testdata/json_plan_suite_out.json` around lines 108 - 152, The golden includes runtime-only EXPLAIN ANALYZE fields (executeInfo, memoryInfo, diskInfo, and time_detail) in operator entries like "IndexReader_28(Build)", "IndexReader_26(Probe)", "IndexFullScan_27", and "IndexFullScan_25"; remove or revert those runtime-specific changes so the JSON only contains deterministic planner output (either remove the executeInfo/memoryInfo/diskInfo/time_detail keys or restore them to their previous stable values such as "N/A" or the prior committed strings) and commit the minimal deterministic diff.
🧹 Nitpick comments (1)
pkg/planner/core/casetest/tpch/testdata/tpch_suite_xut.json (1)
467-473: Add a focused assertion for the new semi-join rewrite.Lines 467-473 and Lines 482-492 mainly pin downstream operator/cost changes (
IndexHashJoinin one path, a different non-index semi join in the hinted path). They do not assert the inner dedup step itself, so the new rewrite could regress while these goldens still pass if the cost model lands on similar shapes. A smaller deterministic case that checks the semi-join inner dedup directly would make this coverage much less brittle.As per coding guidelines "Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required".
Also applies to: 482-492
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/casetest/tpch/testdata/tpch_suite_xut.json` around lines 467 - 473, Add a focused, deterministic assertion in the tpch_suite_xut.json testdata that the inner side of the semi-join rewrite contains the dedup step: locate the semi-join plan node IndexHashJoin_27 and assert that its inner branch (the TableReader_58 / Selection_57 / TableRangeScan_56 subtree tied to inner key test.lineitem.l_orderkey) contains an explicit dedup operator (e.g., an Aggregation or Distinct node that performs grouping on l_orderkey). Update the JSON golden to include this small assertion (not a broad cost/operator shape change) so the test fails if the inner dedup is omitted or regressed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/planner/core/casetest/planstats/testdata/plan_stats_suite_out.json`:
- Around line 125-135: The golden output was changed to show an ordinary inner
join for the query "explain format = brief select * from t join tp where tp.a =
10 and t.b = tp.c", which hides the new optimizer path and bypasses the
SemiJoinInnerDedup logic; revert this golden entry to its original inner-join
expectation (or extract this query into a stable unchanged golden) and instead
add a new, dedicated explain test case that triggers the semi/anti-semi join
optimizer path and exercises SemiJoinInnerDedup so the new rule is validated
without broadening existing golden churn.
In `@pkg/planner/core/exhaust_physical_plans.go`:
- Around line 2716-2723: The current call to p.SemiJoinInnerDedup() mutates the
shared join tree by rewriting p.Children()[1], which prevents later index-join
enumerators (INLJ/INLHJ/INLMJ) from seeing the original inner child; change the
flow so you do not mutate the shared child before index-join enumeration —
either (a) enumerate all index-join paths first using the original
p.Children()[1] and only then call p.SemiJoinInnerDedup(), or (b) make
SemiJoinInnerDedup operate on a cloned child (not p.Children()[1]) used only for
HashJoin/MergeJoin branches; reference p.SemiJoinInnerDedup(), p.Children()[1],
and the INLJ/INLHJ/INLMJ paths when applying the fix.
In `@pkg/planner/core/operator/logicalop/logical_join.go`:
- Around line 2071-2107: The injected LogicalSelection (sel) created when
p.RightConditions exists has no StatsInfo, so the subsequent subAgg.DeriveStats
sees nil and skips deriving stats; fix by deriving stats for that selection
immediately after constructing it: after you create sel and set its child
(innerChild before it was wrapped), call sel.DeriveStats(...) using the original
inner child's StatsInfo/Schema (or pass childStats/childSchema built from the
pre-wrapped inner child) so sel.StatsInfo() is populated, then assign innerChild
= sel and proceed to create subAgg and call subAgg.DeriveStats using
sel.StatsInfo()/sel.Schema() instead of assuming innerChild already has stats.
---
Outside diff comments:
In `@pkg/planner/core/casetest/testdata/json_plan_suite_out.json`:
- Around line 108-152: The golden includes runtime-only EXPLAIN ANALYZE fields
(executeInfo, memoryInfo, diskInfo, and time_detail) in operator entries like
"IndexReader_28(Build)", "IndexReader_26(Probe)", "IndexFullScan_27", and
"IndexFullScan_25"; remove or revert those runtime-specific changes so the JSON
only contains deterministic planner output (either remove the
executeInfo/memoryInfo/diskInfo/time_detail keys or restore them to their
previous stable values such as "N/A" or the prior committed strings) and commit
the minimal deterministic diff.
---
Nitpick comments:
In `@pkg/planner/core/casetest/tpch/testdata/tpch_suite_xut.json`:
- Around line 467-473: Add a focused, deterministic assertion in the
tpch_suite_xut.json testdata that the inner side of the semi-join rewrite
contains the dedup step: locate the semi-join plan node IndexHashJoin_27 and
assert that its inner branch (the TableReader_58 / Selection_57 /
TableRangeScan_56 subtree tied to inner key test.lineitem.l_orderkey) contains
an explicit dedup operator (e.g., an Aggregation or Distinct node that performs
grouping on l_orderkey). Update the JSON golden to include this small assertion
(not a broad cost/operator shape change) so the test fails if the inner dedup is
omitted or regressed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 43e70a99-b4a9-4c14-b726-b9f062bc3770
📒 Files selected for processing (8)
pkg/planner/core/casetest/mpp/testdata/integration_suite_out.jsonpkg/planner/core/casetest/planstats/testdata/plan_stats_suite_out.jsonpkg/planner/core/casetest/testdata/json_plan_suite_out.jsonpkg/planner/core/casetest/testdata/json_plan_suite_xut.jsonpkg/planner/core/casetest/tpch/testdata/tpch_suite_out.jsonpkg/planner/core/casetest/tpch/testdata/tpch_suite_xut.jsonpkg/planner/core/exhaust_physical_plans.gopkg/planner/core/operator/logicalop/logical_join.go
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (3)
pkg/planner/core/exhaust_physical_plans.go (1)
2715-2722:⚠️ Potential issue | 🟠 MajorDon't rewrite the shared join tree before enumerating the index-join family.
This call mutates
p.Children()[1]to aLogicalAggregationbeforetryToEnumerateIndexJoin/tryToGetIndexJoinrun. Those paths still inspect the logical inner child, and bothextractIndexJoinInnerChildPatternandadmitIndexJoinInnerChildPatternrejectLogicalAggregationunlessEnableINLJoinInnerMultiPatternis on. So valid INLJ/INLHJ/INLMJ candidates can disappear here; the “unaffected” comment is misleading. Please keep dedup off the shared join state until after index-join enumeration, or apply it on a clone used only for hash/merge paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/exhaust_physical_plans.go` around lines 2715 - 2722, The call to p.SemiJoinInnerDedup() mutates p.Children()[1] into a LogicalAggregation and must not run before tryToEnumerateIndexJoin/tryToGetIndexJoin because extractIndexJoinInnerChildPattern and admitIndexJoinInnerChildPattern will then reject valid INLJ/INL* candidates; fix by deferring SemiJoinInnerDedup until after index-join enumeration completes or instead apply SemiJoinInnerDedup to a deep clone of the join node used only for HashJoin/MergeJoin planning so the shared join state (p.Children()[1]) remains unchanged while tryToEnumerateIndexJoin/tryToGetIndexJoin inspect the original inner child.pkg/planner/core/operator/logicalop/logical_join.go (2)
2123-2159:⚠️ Potential issue | 🔴 CriticalDerive stats for the injected
LogicalSelectionbefore deriving the aggregate.Once Line 2124 wraps the inner child in a fresh
LogicalSelection,innerChild.StatsInfo()becomes nil. That makes the guard at Line 2153 skipsubAgg.DeriveStats, so both injected nodes can remain without stats. The downstream physical builders consumesel.StatsInfo()/logicalAgg.StatsInfo()directly, so this path can still miscost or panic.Suggested fix
if len(p.RightConditions) > 0 { sel := LogicalSelection{Conditions: make([]expression.Expression, len(p.RightConditions))}.Init(p.SCtx(), innerChild.QueryBlockOffset()) copy(sel.Conditions, p.RightConditions) sel.SetChildren(innerChild) + if childStats := innerChild.StatsInfo(); childStats != nil { + if _, _, err := sel.DeriveStats( + []*property.StatsInfo{childStats}, + sel.Schema(), + []*expression.Schema{innerChild.Schema()}, + nil, + ); err != nil { + return nil, err + } + } innerChild = sel p.RightConditions = nil } @@ - if innerChild.StatsInfo() != nil { - childStats := []*property.StatsInfo{innerChild.StatsInfo()} - childSchema := []*expression.Schema{innerChild.Schema()} + if childStats := innerChild.StatsInfo(); childStats != nil { if _, _, err := subAgg.DeriveStats(childStats, subAgg.Schema(), childSchema, nil); err != nil { return nil, err } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/operator/logicalop/logical_join.go` around lines 2123 - 2159, The injected LogicalSelection (created as sel from p.RightConditions) replaces innerChild and thus loses the original child's StatsInfo; before replacing innerChild (i.e., after sel.Init/SetChildren but before innerChild = sel and before creating subAgg), derive stats for sel by calling sel.DeriveStats with the original innerChild.StatsInfo()/Schema (similar to how subAgg.DeriveStats is invoked) so sel.StatsInfo() is populated and subsequent subAgg.DeriveStats sees non-nil child stats; refer to LogicalSelection (sel), innerChild (original), LogicalAggregation (subAgg), and DeriveStats to implement this.
2102-2118:⚠️ Potential issue | 🟠 MajorUse tuple NDV here, not
maxNDV.For multi-column join keys,
max(NDV(col_i))is only a lower bound onNDV(key_tuple), sorows / maxNDVcan greatly overestimate duplication and inject an expensive dedup agg on nearly-unique inputs. This is a real regression risk for semi-joins on composite keys. Please estimate the join-key tuple NDV instead of deriving it from per-column maxima.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/operator/logicalop/logical_join.go` around lines 2102 - 2118, The duplication check is using maxNDV (max per-column NDV) which underestimates tuple NDV for composite keys; change the logic in the block that reads innerChild.StatsInfo(), innerKeyCols, maxNDV and dupRatio to compute an estimated NDV for the entire join key tuple (e.g., start with 1.0, multiply per-column NDVs from innerStats.ColNDVs[col.UniqueID], cap the running product at innerStats.RowCount to avoid overflow and at a sensible upper bound, and handle missing/zero NDVs by falling back to a safe default), then compute dupRatio = innerStats.RowCount / tupleNDV and use that to decide whether to return p.Self() or insert the dedup; replace uses of maxNDV with the new tupleNDV variable and keep the same threshold logic (dupRatio < 2.0) otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/planner/core/exhaust_physical_plans.go`:
- Around line 2715-2722: The call to p.SemiJoinInnerDedup() mutates
p.Children()[1] into a LogicalAggregation and must not run before
tryToEnumerateIndexJoin/tryToGetIndexJoin because
extractIndexJoinInnerChildPattern and admitIndexJoinInnerChildPattern will then
reject valid INLJ/INL* candidates; fix by deferring SemiJoinInnerDedup until
after index-join enumeration completes or instead apply SemiJoinInnerDedup to a
deep clone of the join node used only for HashJoin/MergeJoin planning so the
shared join state (p.Children()[1]) remains unchanged while
tryToEnumerateIndexJoin/tryToGetIndexJoin inspect the original inner child.
In `@pkg/planner/core/operator/logicalop/logical_join.go`:
- Around line 2123-2159: The injected LogicalSelection (created as sel from
p.RightConditions) replaces innerChild and thus loses the original child's
StatsInfo; before replacing innerChild (i.e., after sel.Init/SetChildren but
before innerChild = sel and before creating subAgg), derive stats for sel by
calling sel.DeriveStats with the original innerChild.StatsInfo()/Schema (similar
to how subAgg.DeriveStats is invoked) so sel.StatsInfo() is populated and
subsequent subAgg.DeriveStats sees non-nil child stats; refer to
LogicalSelection (sel), innerChild (original), LogicalAggregation (subAgg), and
DeriveStats to implement this.
- Around line 2102-2118: The duplication check is using maxNDV (max per-column
NDV) which underestimates tuple NDV for composite keys; change the logic in the
block that reads innerChild.StatsInfo(), innerKeyCols, maxNDV and dupRatio to
compute an estimated NDV for the entire join key tuple (e.g., start with 1.0,
multiply per-column NDVs from innerStats.ColNDVs[col.UniqueID], cap the running
product at innerStats.RowCount to avoid overflow and at a sensible upper bound,
and handle missing/zero NDVs by falling back to a safe default), then compute
dupRatio = innerStats.RowCount / tupleNDV and use that to decide whether to
return p.Self() or insert the dedup; replace uses of maxNDV with the new
tupleNDV variable and keep the same threshold logic (dupRatio < 2.0) otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 50ddffa1-0e6f-4704-b8d1-b9d599901069
📒 Files selected for processing (2)
pkg/planner/core/exhaust_physical_plans.gopkg/planner/core/operator/logicalop/logical_join.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/planner/core/operator/logicalop/logical_join.go (1)
2107-2126:⚠️ Potential issue | 🟠 MajorEstimate duplication from the full join-key tuple, not per-column max NDV.
max(NDV(col_i))is only a lower bound onNDV(join_key_tuple), soRowCount / maxNDVis an upper bound on duplication. On composite semi-join keys this can easily cross the2.0threshold even when the tuple is nearly unique, which injects an unnecessary dedup aggregation and can regress plan quality. Please use a tuple-NDV estimate here instead, e.g.GroupNDV/cardinality.EstimateColsNDVWithMatchedLen.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/operator/logicalop/logical_join.go` around lines 2107 - 2126, The current logic in logical_join.go computes maxNDV across innerKeyCols and uses RowCount/maxNDV to decide dedup, which overestimates duplication for composite keys; replace the per-column max NDV calculation with a tuple NDV estimate (e.g. call GroupNDV or cardinality.EstimateColsNDVWithMatchedLen using innerKeyCols and innerStats.ColNDVs) to produce tupleNDV, then compute dupRatio := innerStats.RowCount / tupleNDV (guarding for tupleNDV <= 0) and keep the existing threshold check (dupRatio < 2.0) before returning p.Self(), false, nil; ensure you update references around innerStats, innerKeyCols, dupRatio to use the new tupleNDV-based logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/planner/core/operator/logicalop/logical_join.go`:
- Around line 2069-2074: The current dedup rewrite in SemiJoinInnerDedup is
incorrectly gated by the session var check EnableINLJoinInnerMultiPattern (the
if block that returns p.Self(), false, nil), which disables dedup for
HashJoin/MergeJoin; remove that conditional early return so the dedup rewrite
always proceeds (i.e., delete the if
!p.SCtx().GetSessionVars().EnableINLJoinInnerMultiPattern { return p.Self(),
false, nil } block) and let SemiJoinInnerDedup run unconditionally; keep the
rest of the function logic and its interactions with
exhaustPhysicalPlans4LogicalJoin intact.
---
Duplicate comments:
In `@pkg/planner/core/operator/logicalop/logical_join.go`:
- Around line 2107-2126: The current logic in logical_join.go computes maxNDV
across innerKeyCols and uses RowCount/maxNDV to decide dedup, which
overestimates duplication for composite keys; replace the per-column max NDV
calculation with a tuple NDV estimate (e.g. call GroupNDV or
cardinality.EstimateColsNDVWithMatchedLen using innerKeyCols and
innerStats.ColNDVs) to produce tupleNDV, then compute dupRatio :=
innerStats.RowCount / tupleNDV (guarding for tupleNDV <= 0) and keep the
existing threshold check (dupRatio < 2.0) before returning p.Self(), false, nil;
ensure you update references around innerStats, innerKeyCols, dupRatio to use
the new tupleNDV-based logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 129df6be-09b3-4c60-82f4-f612bea9f340
📒 Files selected for processing (3)
pkg/planner/core/casetest/rule/rule_outer_to_semi_join_test.gopkg/planner/core/exhaust_physical_plans.gopkg/planner/core/operator/logicalop/logical_join.go
Resolve conflict in exhaust_physical_plans.go: upstream pingcap#66871 removed index join build v1, leaving only tryToEnumerateIndexJoin. Adapted our SemiJoinInnerDedup ordering (IndexJoin first, then dedup, then MergeJoin) to the simplified single-path code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…dens Remove the EnableINLJoinInnerMultiPattern guard from SemiJoinInnerDedup since IndexJoin is enumerated before dedup fires, making the gate unnecessary. Re-record all affected golden test files to reflect the updated plan outputs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ti-chi-bot
Bot
added
size/XXL
Skip dedup when inner child has nil stats, zero row count, or unknown NDV (maxNDV <= 0). This prevents plan regressions on pseudo stats where injecting an aggregation can block IndexJoin or shift the optimizer toward more expensive plans. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/planner/core/casetest/planstats/testdata/plan_stats_suite_out.json (1)
125-134:⚠️ Potential issue | 🟡 MinorKeep these goldens focused on the semi/anti-semi optimizer path.
Line 125 and Line 151 are both ordinary inner joins, so updating their expected plans here adds unrelated golden churn without asserting
SemiJoinInnerDedup. Please keep these cases stable, or move the coverage into a dedicated semi/anti-semi explain case that actually triggers the new rule; otherwise this suite can mask unrelated planner regressions.As per coding guidelines, "Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required" and "For planner rule or logical/physical plan changes, perform targeted planner unit tests and update rule testdata when needed."
Also applies to: 151-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/casetest/planstats/testdata/plan_stats_suite_out.json` around lines 125 - 134, The updated goldens include ordinary inner-join cases (e.g., the query "explain format = brief select * from t join tp where tp.a = 10 and t.b = tp.c") that do not exercise the SemiJoinInnerDedup rule; revert the changes to these inner-join expected plans (restore the previous golden outputs for the ordinary inner joins) or move this coverage into a new dedicated semi/anti-semi explain test that explicitly triggers and asserts SemiJoinInnerDedup; ensure any new test uses a query/plan that actually produces a semi/anti-semi operator and add a focused assert for SemiJoinInnerDedup rather than changing unrelated plan goldens.
🧹 Nitpick comments (2)
pkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_xut.json (1)
1268-1277: Please add a CDC case where semi-inner dedup is observable.This updated
EXISTSgolden still only asserts the new selection placement; there is no distinct/agg node on the semi-join inner to lock in theSemiJoinInnerDedupbehavior itself. A paired case in this suite where the CDC path is expected to emitHashAggorStreamAggon the semi inner would make regressions in the new feature visible.Based on learnings: Applies to pkg/planner/** : For planner rule or logical/physical plan changes, perform targeted planner unit tests and update rule testdata when needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_xut.json` around lines 1268 - 1277, Add a CDC test case that makes the semi-join inner require dedup so the SemiJoinInnerDedup behavior is observable: create a query using EXISTS (or SEMI join) where the inner side can produce duplicates (e.g., joining to a table that repeats keys) and add it to the same suite; update the expected golden plan to assert a HashAgg or StreamAgg node under the SemiJoin inner (look for symbols "SemiJoin", "SemiJoinInnerDedup", "HashAgg" or "StreamAgg" and the EXISTS/EXISTS-style pattern) so the CDC path emits an aggregation on the semi inner and the test fails on regressions.pkg/planner/core/casetest/testdata/json_plan_suite_out.json (1)
112-159: These subtree snapshot updates are not exercised bytestJSONPlanInExplain.
pkg/planner/core/casetest/plan_test.go:365-396only compares the top-levelJSONPlanfields and never recurses intosubOperators, so the Lines 114-152 changes here will not fail if the inner-side subtree regresses. If this suite is meant to cover the new semi-join-inner dedup shape, please add a recursive comparator or a focused semi/anti-semi JSON-plan case.Based on learnings: Applies to pkg/planner/** : For planner rule or logical/physical plan changes, perform targeted planner unit tests and update rule testdata when needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/casetest/testdata/json_plan_suite_out.json` around lines 112 - 159, The test only compares top-level JSONPlan fields so regressions in nested "subOperators" (the semi-join-inner dedup shape) aren't caught; update pkg/planner/core/casetest/plan_test.go by enhancing testJSONPlanInExplain to recursively compare JSONPlan.subOperators (or add a new focused test that loads the full expected JSON and asserts deep equality of the nested structure) so changes to IndexReader_/IndexFullScan_ subtrees fail the test; reference JSONPlan, subOperators, and testJSONPlanInExplain when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/planner/core/casetest/planstats/testdata/plan_stats_suite_out.json`:
- Around line 125-134: The updated goldens include ordinary inner-join cases
(e.g., the query "explain format = brief select * from t join tp where tp.a = 10
and t.b = tp.c") that do not exercise the SemiJoinInnerDedup rule; revert the
changes to these inner-join expected plans (restore the previous golden outputs
for the ordinary inner joins) or move this coverage into a new dedicated
semi/anti-semi explain test that explicitly triggers and asserts
SemiJoinInnerDedup; ensure any new test uses a query/plan that actually produces
a semi/anti-semi operator and add a focused assert for SemiJoinInnerDedup rather
than changing unrelated plan goldens.
---
Nitpick comments:
In `@pkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_xut.json`:
- Around line 1268-1277: Add a CDC test case that makes the semi-join inner
require dedup so the SemiJoinInnerDedup behavior is observable: create a query
using EXISTS (or SEMI join) where the inner side can produce duplicates (e.g.,
joining to a table that repeats keys) and add it to the same suite; update the
expected golden plan to assert a HashAgg or StreamAgg node under the SemiJoin
inner (look for symbols "SemiJoin", "SemiJoinInnerDedup", "HashAgg" or
"StreamAgg" and the EXISTS/EXISTS-style pattern) so the CDC path emits an
aggregation on the semi inner and the test fails on regressions.
In `@pkg/planner/core/casetest/testdata/json_plan_suite_out.json`:
- Around line 112-159: The test only compares top-level JSONPlan fields so
regressions in nested "subOperators" (the semi-join-inner dedup shape) aren't
caught; update pkg/planner/core/casetest/plan_test.go by enhancing
testJSONPlanInExplain to recursively compare JSONPlan.subOperators (or add a new
focused test that loads the full expected JSON and asserts deep equality of the
nested structure) so changes to IndexReader_/IndexFullScan_ subtrees fail the
test; reference JSONPlan, subOperators, and testJSONPlanInExplain when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e849578a-0b04-4c78-86fe-ccdca0669dbf
📒 Files selected for processing (15)
pkg/planner/core/casetest/mpp/testdata/integration_suite_out.jsonpkg/planner/core/casetest/mpp/testdata/integration_suite_xut.jsonpkg/planner/core/casetest/planstats/testdata/plan_stats_suite_out.jsonpkg/planner/core/casetest/rule/rule_outer_to_semi_join_test.gopkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_out.jsonpkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_xut.jsonpkg/planner/core/casetest/rule/testdata/join_reorder_suite_out.jsonpkg/planner/core/casetest/rule/testdata/join_reorder_suite_xut.jsonpkg/planner/core/casetest/testdata/integration_suite_out.jsonpkg/planner/core/casetest/testdata/integration_suite_xut.jsonpkg/planner/core/casetest/testdata/json_plan_suite_out.jsonpkg/planner/core/casetest/testdata/json_plan_suite_xut.jsonpkg/planner/core/casetest/tpch/testdata/tpch_suite_out.jsonpkg/planner/core/casetest/tpch/testdata/tpch_suite_xut.jsonpkg/planner/core/operator/logicalop/logical_join.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/planner/core/casetest/rule/rule_outer_to_semi_join_test.go
|
/ok-to-test |
|
@terry1purcell: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@terry1purcell: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
What problem does this PR solve?
Issue Number: ref #66716
Problem Summary:
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Improvements
Tests