Skip to content

fix(ddl): qualify LIKE foreign keys and reject duplicate columns - #28630

Merged
XuPeng-SH merged 5 commits into
matrixorigin:mainfrom
aptend:fix/28318-28321-create-table-validation
Sep 11, 2026
Merged

fix(ddl): qualify LIKE foreign keys and reject duplicate columns#28630
XuPeng-SH merged 5 commits into
matrixorigin:mainfrom
aptend:fix/28318-28321-create-table-validation

Conversation

@aptend

@aptend aptend commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #28318, #28321

What this PR does / why we need it:

  • Qualify same-database foreign-key references while rebuilding CREATE TABLE ... LIKE, so resolution does not depend on the session default database.
  • Reject case-insensitive duplicate column names before catalog publication while preserving historical case-variant schemas during TAE replay.
  • Add focused planner/catalog unit tests and one public SQL regression test.

@qodo-code-review

Copy link
Copy Markdown

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 →

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head 352d947 against main 9a5dd0c.

One correctness blocker remains: historical catalog compatibility. The earlier version can commit a table with case-variant column names (the exact reproducer in #28321). Rejecting such names on new DDL is correct, but applying the new rule unconditionally during schema deserialization/replay turns an existing unusable table into a TN recovery failure. Please preserve historical-schema recovery (without silently dropping/renaming columns), or supply a safe migration/repair path before enforcing this invariant on reads.

The LIKE qualification change and early duplicate-column rejection otherwise look scoped and consistent: cached source metadata is copied, reference identifiers are quoted, source subscription remapping remains distinct, and new DDL is rejected before publication. I found no additional concrete blocker in those paths.

Validation: inspected all seven changed files, live PR/issue discussions, and exact-head schema reconstruction/replay callers. Current CI results were inspected, not awaited; no new native UT/BVT or live upgrade run was performed. The new Finalize(true) rejection unit test demonstrates the predicate, but does not cover recovery of a catalog produced by the previous version. This is a reachable compatibility failure, not a request for optional supplementary tests.

Comment thread pkg/vm/engine/tae/catalog/schema.go Outdated

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact head e72066d against base 2e4de29, including the prior requested-changes finding. The historical Id/id catalog-recovery blocker is closed: the stricter case-insensitive check is now confined to new CREATE planning before publication, while TAE Schema.Finalize/read/replay retain their historical exact-name behavior. The added marshal/read regression explicitly preserves both historical names rather than deleting or renaming either column.

The LIKE FK fix still qualifies resolved ordinary source references independently of the session default DB, keeps identifier quoting and source-subscription/clone remapping separate, and does not mutate the persistent schema format. New planner and public SQL assertions cover duplicate-column error 1060, transaction/autocommit non-publication and the copied FK's actual catalog target. The new fixture uses the shared cluster, dedicated names/connection and bounded cleanup. No additional concrete correctness, recovery, privilege, lifecycle or performance blocker found; the new duplicate scan is DDL-only, not a table-read/replay hot-path check.

Validation: full current diff and prior review, exact-head schema finalization and FK reconstruction consumers, and focused planner/catalog/public SQL test oracles. No fresh native tests, SQL execution or live restart/upgrade run is claimed; CI was not awaited. Head/base rechecked before submission.

@mergify mergify Bot added the queued label Sep 11, 2026
@mergify

mergify Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

  • Entered queue2026-09-11 07:17 UTC · Rule: main · triggered by rule Automatic queue on approval for main
  • 🟠 Checks running · in-place
  • 🚫 Left the queue2026-09-11 07:26 UTC · at 0bd748287a1ed0bcbe3e043a5348f33be8d07fb0

This pull request spent 8 minutes 21 seconds in the queue, with no time running CI.

Reason

The pull request can't be updated

This pull request seems to come from a fork, and Mergify needs the author's permission to update its branch.
The author needs to enable "Allow edits from maintainers" on this pull request, or update the branch manually.

Hint

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.

Tick the box to put this pull request back in the merge queue (same as @mergifyio queue).

  • Requeue this pull request

@XuPeng-SH
XuPeng-SH merged commit d1275eb into matrixorigin:main Sep 11, 2026
21 of 23 checks passed
@mergify mergify Bot added dequeued and removed queued labels Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dequeued kind/bug Something isn't working size/M Denotes a PR that changes [100,499] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants