Do not merge an outer join whose preserved side is a reference table

[Endika]

Description

A reference table has the same rows on every shard, so merging it into the shard routes runs the whole join once per shard. That is correct for an inner join, but when the reference table is the preserved side of an outer join, each unmatched preserved row comes back once per shard and vtgate concatenates the duplicates. ref LEFT JOIN sharded — and the sharded RIGHT JOIN ref that is rewritten into it — planned as a single Scatter route.

The merge is now blocked for non-inner joins on that branch. The existing ApplyJoin fallback then plans the preserved side as a Reference route and the other side as a scatter probe, which yields each preserved row exactly once:

Join (LeftJoin)
├── [Reference] select ref_with_source.col from ref_with_source
└── [Scatter]   select 1 from `user` where `user`.col = :ref_with_source_col

Inner joins, and outer joins whose preserved side is the sharded table, are unaffected — the whole planbuilder golden suite passes with no regeneration.

Related Issue(s)

Fixes #19545

Checklist

• "Backport to:" labels have been added if this change should be back-ported to release branches
• If this change is to be back-ported to previous releases, a justification is included in the PR description
• Tests were added or are not required
• Did the new or modified tests pass consistently locally and on CI?
• Documentation was added or is not required

Deployment Notes

Affected queries previously returned duplicated rows and will now return the correct ones, and their plans change from a single scatter to an ApplyJoin. Worth a release-note callout.

AI Disclosure

AI-assisted: the comments, pr text and its tests were written with Claude Code, from a diagnosis and fix I had worked out beforehand and reviewed afterwards.

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[Copilot]

Pull request overview

Fixes #19545 by preventing incorrect shard-level merging when a reference table is the preserved side of an outer join.

Changes:

• Blocks unsafe outer-join route merging.
• Adds LEFT and rewritten RIGHT JOIN plan coverage.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
join_merging.go	Prevents duplicate preserved rows across shards.
reference_cases.json	Verifies ApplyJoin fallback plans.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[GrahamCampbell]

Should we account for EqualUnique resolving to zero shards here? For example, this can still be merged into a single route:

ref_with_source
LEFT JOIN (SELECT * FROM user WHERE id = :id) AS u ON ...

If :id is NULL—or a unique lookup has no mapping—the route resolves to DestinationNone. The merged SQL then executes nowhere, dropping the preserved reference rows. I think we should either exclude EqualUnique here or propagate NoRoutesSpecialHandling for this merge so it executes once on an arbitrary shard when routing finds no destination.

[Endika]

Reproduced. With :id bound to NULL the vindex maps to DestinationNone, resolveShards comes back with zero shards, and executeShards short-circuits to an empty result — so the preserved reference rows are gone. A lookup vindex with no mapping for the value gets there the same way.

I went with the second option, propagating NoRoutesSpecialHandling, rather than excluding EqualUnique. Excluding it would remove the merge entirely rather than narrow it: in a sharded keyspace EqualUnique is the only single-shard opcode that reaches this branch — Unsharded and Reference are anyShard and are handled in the case above — so the allowance would become dead code and the N+1 shape from the earlier round would come back. With the flag, the zero-destination case runs on an arbitrary shard, where the predicate that routed nowhere matches nothing, which leaves exactly the preserved rows with NULLs that the outer join owes.

Pushed as a NoRoutesSpecialHandling field on the Route operator, set on this merge and propagated in routeToEngineRoute. The flag is not part of the plan description, so the golden files don't move — the regression test asserts it on the built primitive instead (TestReferencePreservedByOuterJoinRunsEvenWithoutADestination, red before the change, green after). planbuilder, operators, engine and vtgate suites pass, scripts/fmt clean.

One sibling I ran into while checking this, pre-existing rather than from this PR: the a == dual branch makes the same single-shard-is-enough call and has the same gap.

select 1 from (select 1 from dual) as d
  left join (select * from `user` where id = :id) as u on u.col = 1

plans as a single EqualUnique route with the flag unset, so a NULL :id returns no row where the outer join owes one. Now that the field exists it's one line plus a test there. Happy to include it here or leave it for a separate PR — whichever you prefer.

[Copilot]

🔵 Needs a closer look

The planner-wide routing changes require final human review and approval.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

The DML exemptions can cause incomplete or repeated reference-table writes.

Review details

Suppressed comments (2)

go/vt/vtgate/planbuilder/operators/join_merging.go:136

• This DML exemption is unsafe when the preserved reference table is itself the target and the other side is single-shard. For example, delete r from ref r left join user u on u.id = 1 adopts u's EqualUnique routing here, so the merged delete runs on only one shard even though ref has a physical copy on every shard; the copies then diverge. Reference-target DML needs to retain/force all-shard routing (while sharded-side targets may keep the current exemption), with a routed-predicate regression test.

	targets := ctx.SemTable.DMLTargets
	isDirectDMLRowSource := targets.NotEmpty() &&
		TableID(preserved).Merge(TableID(other)).IsOverlapping(targets)

go/vt/vtgate/planbuilder/operators/join_merging.go:136

• Target overlap still does not establish that this join is the direct row source of a routed DML. With schema tracking, a multi-target update such as update ref_with_source r left join user u on r.col = u.col set r.tt = u.col, u.foo = 3 is converted to DMLWithInput before this merge (operators/update.go:129-130,159-199), while DMLTargets still contains both tables. This condition therefore permits a scatter SELECT that returns every unmatched r row once per shard; engine/dml_with_input.go:116-141 then executes the non-literal reference-table update once per duplicate input row, repeating writes and triggers. The exemption needs an explicit indication that the join is directly under a routed Update/Delete rather than merely checking target overlap.

	targets := ctx.SemTable.DMLTargets
	isDirectDMLRowSource := targets.NotEmpty() &&
		TableID(preserved).Merge(TableID(other)).IsOverlapping(targets)

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

The planner-wide routing changes require final human review and approval.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2700c94f24

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Exercise the no-destination path in the test

This test only builds a plan and inspects NoRoutesSpecialHandling; it never supplies a value whose lookup resolves to no destination or executes the route, despite its name and assertion message claiming that preserved rows are returned in that case. A regression in engine.Route that ignores the flag would therefore leave this test green, so add an execution-level case that produces no destination and asserts the preserved reference row is returned.

AGENTS.md reference: AGENTS.md:L79-L80

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Drop preservation when a later join consumes the rows

referenceRowsInvariant unconditionally inherits preservation from both inputs even when a preserved route is the non-preserved RHS of this later join. For example, in user u LEFT JOIN (ref r LEFT JOIN user x ON r.col = x.id AND x.id = 1) ON u.id = x.id, the inner unmatched reference rows have x.id = NULL and cannot be emitted by the outer join, while the only matching x.id = 1 row is safely colocated by the shared unique vindex. The inherited flag nevertheless rejects the scatter merge and leaves an ApplyJoin that can issue an RHS lookup for every user row; only preservation that can affect the merged join's output should constrain its fanout.

AGENTS.md reference: AGENTS.md:L159-L160

Useful? React with 👍 / 👎.

[Copilot]

🔵 Needs a closer look

The routing and optimizer changes span several interacting planner rewrite paths and require human validation.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

The planner-wide routing changes require final human review.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Endika]

@GrahamCampbell ready for another pass whenever you have a moment — everything from your
round is in, plus a rebase on main and two rounds of bot feedback. Head is 793b75f.

Only DCO has run: the other workflows are still in action_required, so there's no CI
signal at all. If someone can approve the runs, that's the blocker.

Also still labelled NeedsDescriptionUpdate, NeedsIssue, NeedsBackportReason and
NeedsWebsiteDocsUpdate — the description has Fixes #19545 and deployment notes, and no
backport is intended. Say the word and I'll write the justification.