[4.0-dev] fix(parser): support PostgreSQL-style UPDATE ... SET ... FROM ... WHERE#24562
Open
ck89119 wants to merge 8 commits into
Open
[4.0-dev] fix(parser): support PostgreSQL-style UPDATE ... SET ... FROM ... WHERE#24562ck89119 wants to merge 8 commits into
ck89119 wants to merge 8 commits into
Conversation
Allow UPDATE statements to introduce additional join sources via a FROM clause between SET and WHERE, matching the standard / PostgreSQL syntax. The grammar cross-joins the target with the FROM-clause tables so the existing multi-table UPDATE binder/planner path handles it unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address review feedback on the initial PostgreSQL-style UPDATE support:
- Introduce Update.From so the target table stays in Tables and the FROM
clause is preserved as its own join tree. The grammar no longer wraps
target and sources in a cross-join.
- bind_update / buildTableUpdate still build a single inner SELECT (a
cross-join of target and FROM sources) for column resolution, but
dmlTableInfo only sees the target.
- This fixes four issues:
1. Unqualified SET LHS is no longer reported as ambiguous when the
target and a FROM source share a column name (only the target is
considered for the LHS).
2. The target stays a plain TableName, so bind_update no longer falls
back to buildTableUpdate, restoring generated-column write checks
and base-column recomputation.
3. FROM-clause tables are not passed through checkTableType, so views
and other read-only sources are accepted.
4. Update.Format emits the original PG-style "from <join-tree>", so
round-tripping preserves associativity of FROM's join tree.
Tests updated:
- parser UT switches outputs to PG-style and adds JOIN/LEFT JOIN cases.
- plan UT adds same-column-name and FROM-with-JOIN cases.
- BVT covers view as source, target/source same-named columns, generated
column write rejection, and generated column recomputation.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address the second round of review feedback: - Update.Format now wraps any JoinTableExpr that occupies a right-operand position with ParenTableExpr (a shallow rewrite, the original AST is untouched). Previously "FROM b, c JOIN d ON ..." (AST: b CROSS (c INNER d)) re-parsed left-associatively as (b CROSS c) INNER d, changing the ON's binding. - getUpdateTableInfo (fallback path) sets needAggFilter when stmt.From is present. Without this, a target row matched by multiple FROM-source rows would be double-written on the fallback path (e.g. FK targets); with it, the existing any_value() aggregation in buildUpdatePlans runs. - getUpdateTableInfo rejects SET on a stored generated column (mirroring bind_update.go), and silently drops SET gen_col = DEFAULT so the recomputation below fills it. - selectUpdateTables now recomputes generated columns after binding, matching bind_update.go's behaviour but using the fallback ProjectList layout. insertColPos construction moved after the recomputation so it picks up the new positions. Tests: - parser UT adds round-trip cases for "FROM b, c JOIN d ON ..." and "FROM b, c LEFT JOIN d ON ..." that confirm the paren wrap. - plan UT adds a self-join case. - BVT covers FK target with generated column (fallback path: direct write rejection + base column recomputation), self-join, ORDER BY / LIMIT rejection at parse time, and duplicate-match on the fallback path (verifies any_value dedup converges to a single row). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ck89119
requested review from
aunjgr,
heni02,
iamlinjunhong and
ouyuanning
as code owners
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
aunjgr
approved these changes
May 25, 2026
heni02
approved these changes
May 25, 2026
iamlinjunhong
approved these changes
May 25, 2026
XuPeng-SH
requested changes
May 25, 2026
Contributor
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Requesting changes due to a planner correctness regression in the fallback UPDATE path.
In pkg/sql/plan/build_update.go, the generated-column rewrite appends recomputed generated expressions to the global selectNode.ProjectList, but then increments upPlanCtx.updateColLength / tableBase as if those expressions were inserted into that target table's own contiguous slice.
That breaks later target offsets in fallback multi-table UPDATE planning:
- multi-table UPDATE already routes through
buildTableUpdate(pkg/sql/plan/build.go,pkg/sql/plan/dml_context.go) - generated expressions are appended globally at
selectNode.ProjectList = append(selectNode.ProjectList, genExpr) - later targets still derive
beginIdxfrombeginIdx = beginIdx + upPlanCtx.updateColLength + len(tableDef.Cols) - downstream consumers use
delCtx.beginIdx + .../delCtx.beginIdx + len(cols) + ito read the projection slice (pkg/sql/plan/build_dml_util.go)
So if an earlier target has a generated column, later targets read/write the wrong projection positions. I also could not find coverage for this multi-target + generated-column fallback combination; the added tests cover parser shape and single-target fallback/generated-column cases, but not this layout hazard.
This looks like a real correctness blocker, not a follow-up enhancement.
7 tasks
Contributor
Merge Queue Status
This pull request spent 1 hour 34 minutes 32 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonThe merge conditions cannot be satisfied due to failing checks HintYou may have to fix your CI before adding the pull request to the queue again. |
Contributor
Merge Queue Status
This pull request spent 7 hours 31 minutes 35 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonThe merge conditions cannot be satisfied due to failing checks HintYou may have to fix your CI before adding the pull request to the queue again. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Which issue(s) this PR fixes:
issue #23137
What this PR does / why we need it:
Cherry-picks #24536 to
4.0-dev.This backports support for PostgreSQL-style
UPDATE ... SET ... FROM ... WHERE, including parser, formatter, planner fallback-path handling, parser/planner unit coverage, and BVT coverage.Cherry-picked commits:
fix(parser): support PostgreSQL-style UPDATE ... SET ... FROM ... WHEREfix(parser): keep UPDATE ... FROM target and sources separatedfix(plan): align UPDATE ... FROM fallback path and FROM formatterTests were not run locally, per the cherry-pick workflow.