[4.1] fix data branch edge cases#25570
Conversation
41fe955 to
0fc4799
Compare
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? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
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):
CREATE VIEW src.v AS SELECT src.t.a FROM src.tis rewritten asCREATE VIEW dst.v AS SELECT src.t.a FROM dst.t.CREATE VIEW src.v AS SELECT a FROM src.t ORDER BY (SELECT max(b) FROM src.u)is rewritten with the innerFROM src.uunchanged.
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
UnresolvedNamenodes; - 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.
|
@XuPeng-SH 已处理 review。
验证已运行:pkg/frontend 定向 go test、go build、go vet、git diff --check。请重新 review。 |
What type of PR is this?
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, andWHEN CONFLICT ACCEPTmerge for all four forms.The changes also update
branch_matrix_coverageandbranch_unhappy_pathexpected results after checking that the generated results match the intended semantics. The invalidDATA BRANCH PICK src INTO dstgrammar extension is removed; incomplete PICK syntax remains rejected by the parser.4.1-specific adaptation:
4.1-devdoes not contain the session-level remap wiring from feat(remap): session-level table rewrites + remapdb database remapping #25185, and this PR does not add that feature surface.4.1-devdoes not contain the corresponding main-branch refactors.Validation:
make buildgit diff --check upstream/4.1-dev...HEADgo test -count=1 ./pkg/frontendgo vet ./pkg/frontendgo build,go vet, andgo test -count=1for container/types, frontend, databranchutils, objectio/mergeutil, parser tree/mysql, sql/plan, sql/compile, and disttae@bvt:issue/@skiptags in the three data-branch acceptance BVT filesbranch_matrix_coverage: 108/108 passed, ignored: 0branch_unhappy_path: 63/63 passed, ignored: 0branch_edge_cases: 109/109 passed, ignored: 0Review follow-up
Addresses XuPeng-SH's review of the clone-view AST rewrite.
Follow-up validation: