fix: DELETE with JOIN deletes all rows instead of only matched rows by aunjgr · Pull Request #25125 · matrixorigin/matrixone

aunjgr · 2026-06-24T06:43:08Z

What type of PR is this?

BUG

Which issue(s) this PR fixes:

What this PR does / why we need it:

DELETE ... JOIN was broken: the planner rejected these queries from the modern bindDelete path, routing them to the legacy Node_DELETE path that ignored the join condition and deleted all rows from the target table.

Root cause

Two guards in bindDelete incorrectly rejected single-target DELETE...JOIN:

Truncation guard (stmt.Where == nil && stmt.Limit == nil): for JOIN deletes the condition lives in the ON clause, not WHERE. With stmt.Where == nil, the guard fired and tried a truncate rewrite, which failed and returned an error — falling back to legacy Node_DELETE.
PROJECT node check (selectNode.NodeType != plan.Node_PROJECT): after a JOIN, bindSelect can produce a non-PROJECT root. This guard rejected it as "malformed select node", also falling back to legacy.

Fix

Truncation guard: added len(stmt.TableRefs) == 0 — skip truncate rewrite when joins are present.
PROJECT check: removed. bindSelect output is always usable regardless of root node type.
Target-row dedup: injected Node_DISTINCT above the select when stmt.TableRefs > 0. Since DELETE projects only target-table columns (including the unique Row_ID), DISTINCT on the whole row eliminates exact duplicates produced by a JOIN where the RHS has multiple matches per target row. This happens at the plan level — no runtime flag plumbing, no cross-batch dedup hole.

Removed

All runtime dedup plumbing from the previous iteration of this PR: NeedDedupDelete flag, FilterRowIdForDel call in delete_table(), hasJoinDescendant tree walker, flag propagation through constructMultiUpdate/dupOperator/NewPartitionMultiUpdateFrom.

Tests

Added three regression groups:

tj_a/tj_b: DELETE with ON-clause only (no WHERE) — verifies the truncation guard fix
tj_c/tj_d: duplicate RHS matches (3× id=1) — verifies dedup correctness on non-partitioned path
tj_part_c/tj_part_d: HASH-partitioned table with duplicate RHS matches (3× id=1, 2× id=2) — verifies partitioned path

115/115 BVT tests pass at 100%.

File	Change
`pkg/sql/plan/bind_delete.go`	+6: truncation guard + DISTINCT injection
`pkg/sql/colexec/multi_update/delete.go`	-22: removed runtime dedup block
`pkg/sql/compile/compile.go`	-21: removed `needDedup` walk, `hasJoinDescendant`
`pkg/sql/compile/operator.go`	-3: removed flag parameter and dup copy
`pkg/sql/colexec/multi_update/multi_update_partition.go`	-1: removed flag copy
`pkg/sql/colexec/multi_update/types.go`	-1: removed flag field
`test/.../delete_multiple_table.sql`	+42: three regression groups
`test/.../delete_multiple_table.result`	regenerated

Net: -49 lines simpler than the runtime dedup approach.

🤖 Generated with Claude Code

qodo-code-review · 2026-06-24T06:43:13Z

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

…ates When DELETE...JOIN routes through MULTI_UPDATE, the join output may contain duplicate target rows when the right side has multiple matches. Use FilterRowIdForDel to deduplicate by rowid before counting affected rows, matching the plain deletion.go path. Skip allocation when no duplicates exist. BVT: tj_c JOIN tj_d with 3x duplicate matches — count(*)=1 verified.

XuPeng-SH

matrixone#25098 is an issue, so I reviewed the fixing PR #25125.

I found one blocker here: the new rowid-dedup path still does not reach partitioned multi-table DELETE.

compileMultiUpdate() only sets NeedDedupDelete when the scope root is *multi_update.MultiUpdate, but constructMultiUpdate() wraps partitioned targets in *multi_update.PartitionMultiUpdate instead. As a result, the raw operator behind PartitionMultiUpdate keeps NeedDedupDelete=false, and delete_table() skips FilterRowIdForDel() on the partitioned path.

There is a second propagation hole too: NewPartitionMultiUpdateFrom() does not copy raw.NeedDedupDelete, so even if the root operator is fixed later, the cloned partition operator path would still drop the flag.

That means the duplicate-join-match bug is still present for partitioned target tables.

Suggested fix:

propagate the dedup decision to PartitionMultiUpdate.raw in compileMultiUpdate();
preserve NeedDedupDelete in NewPartitionMultiUpdateFrom();
add a regression with a partitioned target table where the RHS has duplicate matches, so we prove each target row is deleted exactly once on the partitioned path.

The new non-partitioned BVTs are directionally right, but without the partitioned regression this fix is still incomplete.

Fixes the partition-path dedup hole flagged in review for matrixorigin#25125. - Add GetMultiUpdate() accessor on PartitionMultiUpdate (following PartitionDelete.GetDelete() convention) - compileMultiUpdate: set NeedDedupDelete on PartitionMultiUpdate.raw when the root op is a partition wrapper - NewPartitionMultiUpdateFrom: copy NeedDedupDelete from source - dupOperator MultiUpdate case: defensively copy NeedDedupDelete - Add regression test: hash-partitioned DELETE...JOIN with duplicate RHS matches (3x id=1, 2x id=2 → 2 distinct deletes) All 115 delete_multiple_table BVT tests pass at 100%. Co-Authored-By: Claude <noreply@anthropic.com>

Compute needDedup before constructMultiUpdate calls and pass it as a parameter. Set arg.NeedDedupDelete before NewPartitionMultiUpdate wraps it — the flag rides into the wrapper automatically, eliminating the brittle type-assertion loop. - compileMultiUpdate: move needDedup walk before construction; only UpdateWriteTable call site passes needDedup (the only path that reaches delete_table); WriteS3/FlushS3Info pass false - constructMultiUpdate: add needDedup bool parameter, set on arg - Remove GetMultiUpdate() accessor (dead code) - Remove post-hoc type-assertion loop Co-Authored-By: Claude <noreply@anthropic.com>

…me plumbing Replace the runtime NeedDedupDelete flag + FilterRowIdForDel chain with a single Node_DISTINCT injected above the select in bindDelete when joins are present. Since DELETE projects only target-table columns (including the unique Row_ID), DISTINCT on the whole row correctly deduplicates. - bind_delete.go: +6 lines — appendDistinctNode when TableRefs > 0 - delete.go: -22 lines — remove dedup block and colexec import - compile.go: -21 lines — remove needDedup walk, hasJoinDescendant, call-site args - operator.go: -3 lines — remove parameter and dup copy - multi_update_partition.go: -1 line — remove NeedDedupDelete copy - types.go: -1 line — remove NeedDedupDelete field 115/115 delete_multiple_table BVT tests pass at 100%. Net: -49 lines simpler. Co-Authored-By: Claude <noreply@anthropic.com>

mergify · 2026-06-30T08:25:47Z

Queued — the merge queue status continues in this comment ↓.

mergify · 2026-06-30T08:26:05Z

Merge Queue Status

✅ Entered queue — 2026-06-30 08:25 UTC · Rule: main · triggered by rule Automatic queue on approval for main
✅ Checks passed · in-place
✅ Merged — 2026-06-30 10:45 UTC · at ef05b2efbafe5a507126883895a3e47dbe242986 · squash

This pull request spent 2 hours 19 minutes 58 seconds in the queue, including 1 hour 9 minutes 51 seconds running CI.

Required conditions to merge

aunjgr requested review from heni02 and ouyuanning as code owners June 24, 2026 06:43

mergify Bot added the kind/bug Something isn't working label Jun 24, 2026

aunjgr had a problem deploying to ci June 24, 2026 06:44 — with GitHub Actions Failure

aunjgr temporarily deployed to ci June 24, 2026 06:44 — with GitHub Actions Inactive

aunjgr had a problem deploying to ci June 24, 2026 06:44 — with GitHub Actions Error

aunjgr had a problem deploying to ci June 24, 2026 06:44 — with GitHub Actions Failure

aunjgr temporarily deployed to ci June 24, 2026 06:44 — with GitHub Actions Inactive

aunjgr had a problem deploying to ci June 24, 2026 06:44 — with GitHub Actions Error

aunjgr had a problem deploying to ci June 24, 2026 06:44 — with GitHub Actions Failure

aunjgr had a problem deploying to ci June 24, 2026 06:44 — with GitHub Actions Error

aunjgr temporarily deployed to ci June 24, 2026 06:44 — with GitHub Actions Inactive

matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Jun 24, 2026

aunjgr force-pushed the fix/25098-delete-join branch from 8d92328 to 9fd1b7d Compare June 24, 2026 07:22

aunjgr had a problem deploying to ci June 24, 2026 07:24 — with GitHub Actions Failure

aunjgr temporarily deployed to ci June 24, 2026 07:24 — with GitHub Actions Inactive

matrix-meow added size/M Denotes a PR that changes [100,499] lines and removed size/S Denotes a PR that changes [10,99] lines labels Jun 24, 2026

aunjgr temporarily deployed to ci June 25, 2026 05:05 — with GitHub Actions Inactive

aunjgr had a problem deploying to ci June 25, 2026 05:05 — with GitHub Actions Error

aunjgr temporarily deployed to ci June 25, 2026 05:05 — with GitHub Actions Inactive

aunjgr temporarily deployed to ci June 25, 2026 05:14 — with GitHub Actions Inactive

aunjgr added 4 commits June 27, 2026 00:11

Merge branch 'main' into fix/25098-delete-join

68df3b2

fix: use plan.Node_JOIN instead of literal 14 in hasJoinDescendant

db69548

Merge branch 'main' into fix/25098-delete-join

7992be2

XuPeng-SH requested changes Jun 29, 2026

View reviewed changes

aunjgr and others added 2 commits June 29, 2026 18:41

Merge branch 'main' into fix/25098-delete-join

43e2bc7

This comment was marked as outdated.

Sign in to view

aunjgr and others added 2 commits June 29, 2026 21:12

XuPeng-SH approved these changes Jun 30, 2026

View reviewed changes

ouyuanning approved these changes Jun 30, 2026

View reviewed changes

Merge branch 'main' into fix/25098-delete-join

ef05b2e

Navigation Menu

Uh oh!

fix: DELETE with JOIN deletes all rows instead of only matched rows - #25125

fix: DELETE with JOIN deletes all rows instead of only matched rows#25125
mergify[bot] merged 11 commits into
matrixorigin:mainfrom
aunjgr:fix/25098-delete-join

aunjgr commented Jun 24, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Jun 24, 2026

Uh oh!

XuPeng-SH left a comment

Uh oh!

This comment was marked as outdated.

mergify Bot commented Jun 30, 2026 •

edited

Loading

Uh oh!

mergify Bot commented Jun 30, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

aunjgr commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

Which issue(s) this PR fixes:

What this PR does / why we need it:

Root cause

Fix

Removed

Tests

Uh oh!

qodo-code-review Bot commented Jun 24, 2026

Qodo reviews are paused for this user.

Uh oh!

XuPeng-SH left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

mergify Bot commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify Bot commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Queue Status

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aunjgr commented Jun 24, 2026 •

edited

Loading

mergify Bot commented Jun 30, 2026 •

edited

Loading

mergify Bot commented Jun 30, 2026 •

edited

Loading