Skip to content

fix insert ignore - #26607

Open
daviszhen wants to merge 29 commits into
matrixorigin:mainfrom
daviszhen:0731-fix-insert-ignore
Open

fix insert ignore#26607
daviszhen wants to merge 29 commits into
matrixorigin:mainfrom
daviszhen:0731-fix-insert-ignore

Conversation

@daviszhen

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 #25367

What this PR does / why we need it:

  • ENUM 在数值比较、算术和纯数字 IN 中按成员索引计算。
  • SET 在数值、位运算和纯数字比较中按位图计算。
  • 字符串函数、COALESCE、字符串比较及混合字符串 IN 保持 ENUM/SET 的显示值语义。
  • 扩展 ENUM/SET 的 planner 回归测试,覆盖数值运算、数值比较、纯数字/混合 IN、字符串函数与 COALESCE。
  • 更新 dtype/enum、dtype/set BVT 场景及对应结果快照。

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

@matrix-meow matrix-meow added the size/XL Denotes a PR that changes [1000, 1999] lines label Aug 3, 2026
@daviszhen

Copy link
Copy Markdown
Contributor Author

Deep-reviewed exact head ca48d92de9a5242cbe3c8969377fd3364a93f95f. The covered literal arithmetic/comparison cases work, but two public-contract blockers remain.

P1 — Numeric ENUM/SET semantics are selected by a narrow AST whitelist, so ordinary numeric contexts still use the display string

mysqlSpecialTypeNumericComparison only recognizes a direct special-type column against NumVal/an all-literal tuple, while raw binding is separately special-cased only for unary/binary AST nodes (base_binder.go:2214-2294). Explicit numeric casts, numeric functions, BETWEEN, numeric columns, unary numeric literals on the comparison side, and non-literal IN lists therefore bind cast_enum_index_to_value(...) first. The new raw ENUM cast overload is not reachable from those SQL paths.

Using a real cluster built from this exact head with e ENUM(a,b,), i INT, and row (a,1):

SQL                            MySQL 8.4.10   this head
CAST(e AS SIGNED)              1              error: cast to int, bad value a
ABS(e)                         1              same error
e = i                          1              same error
e BETWEEN 1 AND 2              1              same error
e IN (i)                       1              0
e = +1                         1              same error

The typed planner probe independently confirms that all six plans retain the ENUM display-value wrapper. Numeric-vs-string context needs to be decided from the bound operand contract, not only these AST spellings; add public regressions with nearby string controls.

P1 — The advertised INSERT IGNORE half of #25367 is still unfixed

The PR is titled fix insert ignore and closes #25367, but no INSERT IGNORE adjustment path is changed or tested. The issue witness still fails on this exact head under STRICT_TRANS_TABLES:

insert ignore ... values (1,2156,b11111,bad,x,bad)
-- Error 20301: data too long, type width = 4, val = 11111

MySQL adjusts the YEAR/BIT/ENUM/SET values and writes the row. Either complete that contract here or narrow the PR title/issue linkage so the unresolved behavior is not represented as fixed.

Existing pkg/sql/plan and pkg/sql/plan/function suites, vet, diff checks, and remote CI pass; the missing public-path cases are not covered.

  • 扩展 ENUM/SET 数值上下文绑定:不再只依赖有限 AST 形式,支持显式数值 CAST、数值函数、数值列比较、BETWEEN、非字面量 IN、一元正负号及 quantified subquery,统一按 ENUM ordinal / SET bitmap 参与数值运算。
  • 保留字符串上下文行为:字符串比较、长度等操作仍使用 ENUM/SET 的展示标签。
  • 完善 INSERT IGNORE 特殊类型转换:YEAR、BIT、ENUM、SET 在严格模式下遇到溢出或非法值不再中断语句,而是按兼容规则调整后写入。
  • 修复 BIT 边界处理,包括 BIT(64) 浮点上界与溢出截断。
  • 新增 ENUM/SET 数值语义 BVT,以及严格模式 INSERT IGNORE 的 VALUES 和 INSERT…SELECT 回归用例。

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

Deep-reviewed exact head 7532437. The previous ENUM/SET numeric-context blocker and the local/VALUES INSERT IGNORE cases are addressed, and the focused planner/function/util regressions pass. One distributed correctness blocker remains.

P1 — INSERT IGNORE adjustment state is lost at the remote-CN process boundary

The new execution-time YEAR/BIT/ENUM/SET adjustment branches all depend on statementIgnore(proc), which reads proc.GetStmtProfile().GetStatementIgnore(). The frontend sets that flag on the coordinator, but BuildProcessInfo and pipeline.ProcessInfo do not serialize the statement runtime profile, and remoterunServer.go reconstructs a fresh StmtProfile from only txnId/stmtId. Its ignore bit is therefore false.

This is reachable, not just a codec concern: INSERT IGNORE ... SELECT adds the assignment casts in a PROJECT above the source; compilePlanScope keeps PROJECT on each table-scan scope, and table-scan scopes are Remote over the generated CN nodes. If an invalid YEAR/BIT/ENUM/SET value is evaluated on a remote source scope, the same cast that adjusts locally takes its strict error branch and aborts the statement. The added BVT covers INSERT ... SELECT but does not force that projection onto a different CN, so it does not close this execution shape.

A temporary boundary regression on this head set statementRuntimeIgnore=true, built ProcessInfo, reconstructed the remote StmtProfile exactly as remoterunServer does, and asserted the remote flag. It fails: expected true, got false.

Please carry the required statement runtime semantics through ProcessInfo and restore them before remote pipeline execution, with rolling-upgrade-safe behavior, and add a forced remote/multi-CN regression alongside the local control. A minimal public witness is strict mode plus INSERT IGNORE INTO dst(bit(4)) SELECT 31 FROM a source placed on another CN: expected stored 15 without statement failure; the remote projection currently sees ignore=false and raises out-of-range.

Unhappy-path audit: Q1 ownership/transfer fails at coordinator StmtProfile -> ProcessInfo -> remote StmtProfile; Q2 aborts before the write path can apply ignore semantics; Q3 adds no growth risk, but the state-transfer contract is incomplete. diff --check is clean; focused planner, function, and util tests pass; CI is green.

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.

4 participants