Skip to content

[4.1] fix data branch edge cases#25570

Open
gouhongshen wants to merge 9 commits into
matrixorigin:4.1-devfrom
gouhongshen:agent/fix-data-branch-edge-cases-4.1
Open

[4.1] fix data branch edge cases#25570
gouhongshen wants to merge 9 commits into
matrixorigin:4.1-devfrom
gouhongshen:agent/fix-data-branch-edge-cases-4.1

Conversation

@gouhongshen

@gouhongshen gouhongshen commented Jul 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 #24924

What this PR does / why we need it:

Backports #25471 to 4.1-dev.

This PR adds and fixes BVT coverage for data branch edge cases from #24924, then fixes the underlying failures found by those cases.

Root causes covered include unsafe clone SQL rewriting for string literals and quoted identifiers, unescaped internal SQL literals for apostrophe identifiers, missing data branch diff/merge/PICK support for decimal256/year/geometry/enum and other edge types, incomplete GEOMETRY32 comparison and SQL write-back support, partitioned table change collection panics, unstable PICK validation errors, and explicit transaction rejection for DATA BRANCH DIFF/MERGE.

GEOMETRY32 is now handled as varlena data in data branch comparisons. SQL write-back uses st_geomfromtext(WKT[, SRID]) so generic GEOMETRY32, POINT32, GEOGRAPHY32, and explicitly SRID-constrained columns retain subtype validation, float32 WKB conversion, and SRID metadata. The BVT exercises diff, concurrent-update conflict detection, and WHEN CONFLICT ACCEPT merge for all four forms.

The changes also update branch_matrix_coverage and branch_unhappy_path expected results after checking that the generated results match the intended semantics. The invalid DATA BRANCH PICK src INTO dst grammar extension is removed; incomplete PICK syntax remains rejected by the parser.

4.1-specific adaptation:

Validation:

  • make build
  • git diff --check upstream/4.1-dev...HEAD
  • go test -count=1 ./pkg/frontend
  • go vet ./pkg/frontend
  • Fresh go build, go vet, and go test -count=1 for container/types, frontend, databranchutils, objectio/mergeutil, parser tree/mysql, sql/plan, sql/compile, and disttae
  • Conservative CI coverage replay after rebasing: 230/288 changed blocks covered (79.86%, threshold 75%)
  • No @bvt:issue / @skip tags in the three data-branch acceptance BVT files
  • mo-tester branch_matrix_coverage: 108/108 passed, ignored: 0
  • mo-tester branch_unhappy_path: 63/63 passed, ignored: 0
  • mo-tester branch_edge_cases: 109/109 passed, ignored: 0
  • mo-tester pessimistic clone suite: 614/614 passed, ignored: 0

Review follow-up

Addresses XuPeng-SH's review of the clone-view AST rewrite.

  • Rewrites the database component of fully-qualified column names, while preserving two-part table or CTE aliases and string literals.
  • Traverses every relevant expression container: top-level SELECT ORDER BY and LIMIT, VALUES, aggregate ORDER BY, window partition, order and frame expressions, time windows, INSERT duplicate updates, UPDATE and DELETE ordering and limits, and nested subqueries.
  • Adds table-driven coverage for qualified columns; top-level, aggregate and window ordering subqueries; CTE, UNION, nested SELECT and JOIN; alias and literal negatives; and UPDATE and DELETE ordering paths. An end-to-end clone-view test verifies quoted output.
  • Also completes the 4.1 Decimal256 PICK segmentation path already present on main: exact 256-bit gap calculation restores adaptive ZoneMap splitting instead of falling back to count-only segmentation.

Follow-up validation:

  • go test with the MatrixOne CGo library paths for the remap, clone-view, Decimal256 gap, and numeric-type tests
  • go build and go vet for pkg/frontend
  • git diff --check

@mergify mergify Bot requested a review from XuPeng-SH July 10, 2026 05:37
@mergify mergify Bot added kind/bug Something isn't working kind/test-ci labels Jul 10, 2026
@matrix-meow matrix-meow added the size/XL Denotes a PR that changes [1000, 1999] lines label Jul 10, 2026
@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.

I found one blocking correctness issue in the new clone AST rewrite.

High: cloned views can retain references to the source database.

applyRemapDb only remaps tree.TableName nodes, and the manual walker does not cover all expression-bearing fields. I reproduced both cases directly on the current head (c5ed7d1b3a3b74d85c337792e72ae2a10c94ee05):

  1. CREATE VIEW src.v AS SELECT src.t.a FROM src.t is rewritten as CREATE VIEW dst.v AS SELECT src.t.a FROM dst.t.
  2. CREATE VIEW src.v AS SELECT a FROM src.t ORDER BY (SELECT max(b) FROM src.u) is rewritten with the inner FROM src.u unchanged.

The first case happens because database-qualified column references are tree.UnresolvedName values and are never remapped. The second happens because remapDbInSelect walks only With and Select, not top-level OrderBy (and the expression walker also omits aggregate/window OrderBy, window partition/frame expressions, and similar containers).

This breaks the clone isolation invariant: a cloned view can bind to mixed source/destination objects, fail to create, silently continue reading the source database, or fail after the source is dropped.

Please fix this systematically rather than adding only the two observed cases:

  • remap the database component of database-qualified column UnresolvedName nodes;
  • audit and cover every expression-bearing AST field, including top-level SELECT ordering/limit expressions, aggregate ordering, window partition/order/frame expressions, JOIN predicates, and UPDATE/DELETE ordering paths;
  • add table-driven rewrite tests for qualified columns, top-level ORDER BY subqueries, aggregate/window ORDER BY subqueries, JOIN/CTE/UNION/nested-select combinations, and negative cases proving that string literals and aliases matching the source database remain unchanged.

The data-type, partition, explicit-transaction, and geometry changes otherwise look consistent, and all CI checks are green. Non-blocking performance note: unlike the main-branch implementation, this 4.1 backport does not include Decimal256 in numericGap/isNumericType, so Decimal256 PICK falls back to count-based segmentation and can lose ZoneMap pruning efficiency.

@gouhongshen

Copy link
Copy Markdown
Contributor Author

@XuPeng-SH 已处理 review。

  • 限定列名 src.table.column 现在会改写数据库部分;两段式 alias.column 与字符串字面量保持不变。
  • 补齐顶层与函数或窗口 ORDER BY、LIMIT、window frame、VALUES、DML 排序以及嵌套子查询的遍历。
  • 新增限定列、排序子查询、CTE UNION 嵌套 JOIN、别名和字面量负例,以及 UPDATE DELETE 的回归覆盖;clone-view 端到端格式化也已验证。
  • Decimal256 的 PICK 自适应 ZoneMap 切段已与 main 对齐,使用精确 256-bit gap,不再退化为仅按数量切段。

验证已运行:pkg/frontend 定向 go test、go build、go vet、git diff --check。请重新 review。

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 kind/test-ci size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants