Skip to content

fix(plan): enforce CHECK constraints in REPLACE#25070

Open
ck89119 wants to merge 9 commits into
matrixorigin:mainfrom
ck89119:issue-24950-main
Open

fix(plan): enforce CHECK constraints in REPLACE#25070
ck89119 wants to merge 9 commits into
matrixorigin:mainfrom
ck89119:issue-24950-main

Conversation

@ck89119

@ck89119 ck89119 commented Jun 22, 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 #24950

What this PR does / why we need it:

  • Persists parsed CHECK constraints into table definitions so they survive catalog round-trips.
  • Adds row-level CHECK validation before INSERT/REPLACE writes, preserving SQL CHECK semantics where TRUE or NULL passes and FALSE fails.
  • Ensures a failing REPLACE does not delete or overwrite the existing row, with planner tests and BVT coverage.

Test Plan

  • go test ./pkg/vm/engine ./pkg/sql/plan -count=1
  • ./run.sh -p /Users/LoveYY/Develop/matrixorigin/matrixone/.worktrees/issue-24950-main/test/distributed/cases/dml/replace/replace.test -g -n

@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 →

@ck89119 ck89119 requested review from XuPeng-SH and gouhongshen June 24, 2026 03:37

@aunjgr aunjgr 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.

The REPLACE CHECK injection looks correct given how #24950 handles the plan side. The CHECK DDL concerns raised by others are valid but pre-existing — this PR only wires REPLACE into the same check path INSERT already uses.

@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.

I reviewed the latest head from the CHECK-constraint / REPLACE unhappy-path angle and did not find a blocker.

The important ordering looks right: REPLACE builds the final replacement-row projection, injects the CHECK filter/assert before LOCK_OP and before MULTI_UPDATE, so a CHECK failure aborts before the old conflicting row can be deleted. INSERT and UPDATE are wired through the same row-level validation path, and the assert expression preserves SQL CHECK semantics by allowing TRUE or NULL and failing FALSE.

I also checked the catalog round-trip path. Persisting CHECK defs through the hidden stream config key is separated back out in GetTableDef, so normal table properties stay visible while CHECK metadata is restored into TableDef.Checks. The PlanDefToCstrDef signature changes have only the expected call sites.

One detail I specifically chased: this PR changes colName2Idx to mean final projection position so CHECK expressions address the written row, not table ordinals. I did not find a current blocker from that change; the INSERT index-table path that consumes the map uses final projection positions, and the REPLACE CHECK path runs on the final projection before writes.

Local checks:

git diff --check origin/main...HEAD
CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test ./pkg/vm/engine ./pkg/sql/plan -count=1

CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test ./pkg/sql/plan/function -run 'TestCheckConstraintAssertReturnsConstraintViolation|Test_funids' -count=1

CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test ./pkg/sql/plan -run 'TestReplacePlanChecksTableCheckConstraints|TestInsertPlanChecksTableCheckConstraints|TestUpdatePlanChecksTableCheckConstraints|TestBuildCreateTableStoresCheckConstraints|TestCheckConstraintRejectsSyntheticColumns|TestReplace' -count=1

CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test ./pkg/vm/engine/memoryengine ./pkg/vm/engine/disttae/cache ./pkg/vm/engine/disttae -run '^$' -count=0

All passed. The PR is still behind main, so it will need an update/rebase before merge.

@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.

I need to correct my previous approval after re-checking the active blocking reviews and the latest head. The REPLACE injection point itself is good, but the CHECK metadata that reaches that injection point can still be semantically wrong, so I do not think this is ready yet.

Blocker 1: ENFORCED / NOT ENFORCED is still dropped.

The parser AST already carries the enforcement mode: AttributeCheckConstraint.Enforced and CheckIndex.Enforced exist (pkg/sql/parsers/tree/create.go). But buildTableDefs() calls appendCheckDef() with only name + expression for both column and table checks, and appendCheckDef() persists only {Name, Check} into plan.CheckDef. Later appendCheckConstraintPlan() asserts every recovered check unconditionally.

That means an explicit CHECK (...) NOT ENFORCED is silently turned into an enforced runtime constraint. Please either persist the enforcement flag and skip runtime assertions for non-enforced checks, or reject unsupported NOT ENFORCED at DDL time instead of changing its meaning.

Relevant code:

  • pkg/sql/parsers/tree/create.go: AttributeCheckConstraint.Enforced, CheckIndex.Enforced
  • pkg/sql/plan/build_ddl.go: column/table CHECK calls to appendCheckDef() do not pass enforcement mode
  • pkg/sql/plan/build_constraint_util.go: appendCheckConstraintPlan() wraps every stored check in check_constraint_assert(...)

Blocker 2: CHECK binding still uses the wrong scope/timing.

appendCheckDef() binds the expression immediately against the columns currently present in tableDef.Cols. That changes the expected scope rules:

  • table-level CHECKs that appear before later column declarations cannot reference those later columns because they are not in tableDef.Cols yet;
  • column-level CHECKs can see previously declared columns because the binder receives all columns accumulated so far, instead of only the column being defined.

Please collect the CHECK ASTs first with their kind/name/enforcement metadata, bind table-level checks after the full column list is known, and validate/bind column-level checks against only the defining column unless MatrixOne intentionally documents a different rule and adds tests for it.

The runtime ordering for REPLACE still looks right: once the stored CHECK metadata is correct, the filter/assert is placed before LOCK_OP and MULTI_UPDATE, so a failing REPLACE should not delete the old conflicting row.

Local checks I ran before re-checking the DDL blockers:

git diff --check origin/main...HEAD
CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test ./pkg/vm/engine ./pkg/sql/plan -count=1

CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test ./pkg/sql/plan/function -run 'TestCheckConstraintAssertReturnsConstraintViolation|Test_funids' -count=1

CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test ./pkg/sql/plan -run 'TestReplacePlanChecksTableCheckConstraints|TestInsertPlanChecksTableCheckConstraints|TestUpdatePlanChecksTableCheckConstraints|TestBuildCreateTableStoresCheckConstraints|TestCheckConstraintRejectsSyntheticColumns|TestReplace' -count=1

CGO_CFLAGS="-I/home/xupeng/matrixone/cgo -I/home/xupeng/matrixone/thirdparties/install/include" \
CGO_LDFLAGS="-L/home/xupeng/matrixone/cgo -lmo -L/home/xupeng/matrixone/thirdparties/install/lib -Wl,-rpath,/home/xupeng/matrixone/cgo -Wl,-rpath,/home/xupeng/matrixone/thirdparties/install/lib -fopenmp" \
LD_LIBRARY_PATH="/home/xupeng/matrixone/cgo:/home/xupeng/matrixone/thirdparties/install/lib:${LD_LIBRARY_PATH}" \
go test ./pkg/vm/engine/memoryengine ./pkg/vm/engine/disttae/cache ./pkg/vm/engine/disttae -run '^$' -count=0

Those pass, but they do not cover the two DDL semantics cases above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/XXL Denotes a PR that changes 2000+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants