Repository navigation
test: enforce prepared DECIMAL peer pruning and binding reuse - #29570
Conversation
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? |
LeftHandCold
left a comment
There was a problem hiding this comment.
Reviewed the complete two-file diff at bf1cd887bd41c4ed8f20a089928480117f89f4c7 against d59cc02ef6ca68f340c9570e56f7eb71d90e42e7. No blocking finding in this test-only change.
The stronger checks address the old coverage gaps: target-scan counters are checked independently of COUNT/MIN; unsafe fractional, NULL, negative, different-safe and recovery bindings have distinct expected results. The existing CAST/text-warning, nonconstant-derived and 2^53 collision controls remain, and prepared statements/database are cleaned up. No production code or runtime hot path changes.
Also traced EXPLAIN ANALYZE FORCE EXECUTE: FORCE is the grammar form permitting EXECUTE, not an unconditional prepared-plan rebuild flag. Explain and ordinary EXECUTE share parameter initialization, and the DECIMAL uniqueness proof is value-dependent rather than a type-only runtime-cache proof. No evidence that the added Explain statements mask ordinary binding reuse by forcing fresh generations.
Independently checked the actual mo-tester ScriptParser/ResultParser/SqlCommand comparator, not a Python approximation. A lightweight Java harness parsed all 213 commands and 21 regex assertions, accepted the retained healthy plan, and rejected changing only the target scan to 13 blocks / 100000 rows. It also rejected misleading aggregate/different-table metrics and non-adjacent Analyze rows; header-only expected results do not bypass the actual-output regex.
Exact-head CI run 37010990325 completed successfully. Its compose artifact reports this case at 213/213, with zero failures, ignored statements or abnormal statements (3.358 seconds). git diff --check passed and the complete-diff SHA-256 matches the PR description. Reused this real execution evidence; no additional MO build, service or broad UT run was started locally.
Scope: approving the regression coverage, not concluding that the conflicting environment-specific report in #29515 has been explained or that the issue can be closed. The PR already makes that distinction explicitly.
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
## What type of PR is this? - [ ] API-change - [x] BUG - [x] Improvement - [x] Documentation - [ ] Feature - [x] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: Fixes #29399 on `main`; forward-ports the remaining fixes from #29431 (`4.2-dev`). ## What this PR does / why we need it: Repeated restore can mistake CLONE provenance or a view definition for legacy CHECK metadata, lose ownership/current grants, or preserve revoked authorization in an existing connection. Partial restore also treated a role name as identity: renaming an owner blocked restore, while reusing its old name could transfer DROP authority to a different role. - Preserve structured CHECK recovery; recognize CLONE provenance and exclude view definitions from legacy table CHECK parsing. - Recreate objects under historical creator/owner IDs. Partial restore validates and pins required live principals within their physical catalog generation. Role renames preserve identity; name reuse cannot acquire ownership. Table restore locks and retains an existing database without requiring its historical creator/owner. - Resolve partial restore's historical logical source once, including existing view dependency planning, before ownership checks and destructive DDL. Ownership lookup intersects that source after existing external-table and unservable-view omissions. Execution reuses the same objects and classifications; FK/master/publication guards can still reject or omit candidates. This prevents an irrelevant deleted view principal from blocking ordinary data restoration, without exempting servable views. Explicit external-table restore still rejects; skipped single-view restore preserves the live view. - Full account restore rebuilds principal catalogs and can rewind their auto-increment IDs. **Partial restore across rebuilt catalogs rejects ordinary custom principals before DROP, even if IDs/names match or full restore legitimately reconstructed them.** Only reserved bootstrap identities with verified IDs/names and required direct admin grants can cross that boundary. Use a point in the current catalog generation or full restore; documented in `docs/ai-skills/backup-restore.md`. - Keep principal pins and current scoped grant capture/rebinding in the existing restore transaction. DROP USER/ROLE use private pessimistic RC transactions to honor those locks even under ordinary optimistic SI. Partial restore preserves current grants; full/cross-account catalog rebuilding retains its mapping. - Apply existing DATA BRANCH authorization to ordinary-user CLONE, preserving administrator historical/cross-account behavior, subscription errors and recreated publisher identity mapping. - Recheck authorization in existing and text/binary prepared connections across CNs. Within one privilege evaluation, deduplicate only identical SQL misses while retaining ordered errors, early hits and role/view scope. An enum-bounded stack buffer satisfies SCA preallocation without suppression or extra persistent authorization state. The upstream merge at `54af0d38290c66b61ac78a46aa07a363eb576aae` is preserved, including #29563 ESQL and #29568 FULLTEXT2/HNSW coordinator placement. No force push, workflow change or merge-policy bypass. ### Validation Go 1.26.4 / macOS ARM64, verified native provenance. Latest source-selection correction (`93405cdc0ea`): - Authenticated public SQL regression covers snapshot/PITR, independently deleted view creator/owner, ordinary-table data recovery, skipped live-view ID/definition/result preservation, and servable-view missing-principal rejection before DROP. Existing external rejection, retained/recreated DB and unchanged principal-generation controls remain. - Ownership tests cover excluded raw physical metadata, required servable views/sequences and fail-closed missing ownership. FK lookup tests verify shared pointers and scope without fresh enumeration. - After preserving the upstream merge: full frontend **26.886 s**, all six `TestIssue29399*` public tests **31.475 s**, and configured incremental golangci-lint **0 issues**. The byte-identical correction also passed all six tests under race detection before the merge (**69.920 s**). Go vet and repository molint completed with exit 0; existing molint recover diagnostics outside changed code are retained. No linter suppression/configuration weakening. Unchanged evidence from the whole-PR review is retained: planner/compiler and V406 maintenance tests; prepared/cache/active-role controls; normal BVT comparisons including metadata (ownership/CLONE 125/125, builtin full-then-partial 213/213, cache 37/37, revoked-role 39/39). The inherited target merge separately passed compiler/index owning packages and two-CN FULLTEXT2/HNSW rebuild integration. Its initial `TestConnMeasuresDelayedBufferedFlush` timing failure remains recorded; isolated repetitions and subsequent full frontend passed without weakening assertions. ### Performance and limitations Partial source selection adds bounded name maps, uses existing view planning once and avoids repeated partial source enumeration. It adds no persistent state, catalog format, principal query per object or new lock. Current-dependent runtime guards can conservatively omit a prevalidated candidate; this change does not claim exact prediction of every dynamic FK skip. Existing matched public microbenchmark: reader p50 median **1.617 → 1.333 ms (~17.6% lower)**; admin **243.5 → 244.3 µs**, with comparable controls. These local measurements do not recover the old stale-cache roughly 80 µs latency or establish production throughput. Authorization code is unchanged by the latest source-selection correction. The latest correction's design and final overall review use actual **gpt-6.1-sol / xhigh**, reconciling the whole-PR evidence with the changed paths. The source-selection correction's remote CI remains distinct from the target synchronization below; skipped jobs are not passes. ### Prior target synchronization and CI Prior synchronization head: `b190162ef097987a71e9252a49266caaee810451`; target `main`: `401d153150d9076f84aaaf2a32cdcf880b478395`. Normal merge parents are the preserved live `93405cdc0eafd1d2ce15a65d75d5afe2d531b36a` and the new target. This closes the observed strict-baseline BEHIND condition without rebasing, force-pushing or changing Ready/base state. Only #29570's two prepared-DECIMAL SQL/result files change relative to the first parent, byte-identically to the target; every other tracked file is unchanged. The existing source selection, ownership, current-grant transaction, authorization implementation and disclosed performance limits are preserved. Fresh monitor-local exact-merge binary build passes. The new upstream prepared-DECIMAL case passes strict JDBC metadata validation **213/213**, with named database cleanup zero (macOS ARM64 / Go 1.26.4 / Java 20). Independent read-only mechanical merge review approves and whitespace/worktree checks are clean. The prior `93405cdc` frontend/public/race proof above is external local unchanged-Go evidence, not a fresh whole-suite execution or current-head remote CI pass. No source-branch cache-certificate or internal-executor fix is blindly copied to main. XuPeng-SH approved `93405cdc` at 14:21 UTC after whole-PR follow-up; this is an actual submitted review, not the local mechanical approval. Prior-head Connector/J run [37015828472](https://github.com/matrixorigin/matrixone/actions/runs/37015828472) attempt 1 fails during ONNX Runtime download (HTTP 504), before the server/test. Exactly one failed-job rerun request was accepted. Attempt 2 / [110870725085](https://github.com/matrixorigin/matrixone/actions/runs/37015828472/job/110870725085) downloads the dependency and actually passes pool-reset tests for all four Connector/J versions at 14:07:46 UTC. That success belongs to `93405cdc`, not this new merge head. No healthy jobs or whole ALL CI run were rerun/cancelled and no build/workflow/tester/business workaround was added. That head's Connector/J [37023553819](https://github.com/matrixorigin/matrixone/actions/runs/37023553819) actually succeeds at 15:01:50 UTC; ALL CI remains unfinished when it is superseded by the next target merge. This is historical evidence, not a result for the current head. ### Prior target synchronization and validation Prior head: `380bbce20601196b9c9b09ea48089bb666985857`; target `main`: `5a09e1ffbf40a59f50c55fe89e6f0c23eba143a3`. After main merges #29567, strict protection marks the previous head BEHIND again. A normal merge preserves parents `b190162ef097987a71e9252a49266caaee810451` and the new target, without conflicts, manual overwrites, rebasing or force push. Its 15-file delta matches the upstream commit exactly, including modes and blob IDs. All existing PR changes from the approved implementation remain intact. Inherited prepared runtime-plan statistics admission does not bypass schema invalidation or text/binary EXECUTE reauthorization; shared restore source selection, ownership preflight and current-grant transactions are unchanged. Fresh monitor-local exact-head proof (macOS ARM64 / Go 1.26.4): - Full frontend/plan/hashmap **-race passes: 34.982 s / 36.546 s / 3.344 s**. - All six authenticated `TestIssue29399*` tests pass (**29.908 s**), including the eight optimistic-SI privilege-removal arms, snapshot/PITR ownership and source-selection boundaries, principal pins and invalid-grant rollback. Upstream reordered-join semantics and prepared workspace visibility tests pass (**13.031 s**), including integer/bigint/string/composite unique controls. The test-only atomic output `[no statements]` is not production coverage. - Exact binary build and version `380bbce206` pass. Configured incremental lint across all five upstream changed package roots reports **0 issues**. Worktree and whitespace checks are clean. Independent read-only merge/integration review approves; it checks inheritance and interaction, not the general upstream statistics policy. A fresh OPEN/Ready/head guard was awaited and read before the accepted normal push. Latest current-head CI is newly triggered and not complete; no BVT, full coverage, required-context or merge-gate pass is claimed. Prior-head successes are not substituted for current-head results. No workflow, tester or source cache-certificate/internal-executor framework is copied into main. Existing performance limitations and the separate constant-view issue #29572 remain unchanged. ### Current target synchronization and validation Current head: `ae4460f455bafa98a5beb301906adbd0abc522b6`; target `main`: `9078a8bd87af49ea06f52e09d64e7740122aea61` (#29573 vector UnionOne/marshal). Strict-baseline BEHIND is addressed by a normal merge with first parent `380bbce20601196b9c9b09ea48089bb666985857`, without conflict, manual overwrite, rebasing or force push. The four-file delta is byte/mode-identical to the upstream delta; the original 43-file PR patch remains identical against the new target. Restore source selection, ownership/current-grant transactions, principal-generation restriction, protocol/DAG and authorization implementation are preserved. Independent exact-merge local proof (macOS ARM64 / Go 1.26.4, verified inherited native inputs and absolute CGo paths): - Complete vector/batch/mergeutil **-race passes (6.392 s / 3.393 s / 3.712 s)**; complete frontend/plan **-race passes (34.427 s / 34.374 s)**. - All six authenticated `TestIssue29399*` plus authenticated fixture configuration pass **under race (65.232 s)**, including all eight optimistic-SI privilege-removal arms. Another 28 objectio/hashbuild marshal, recovery/area and runtime-filter consumer tests pass **under race (2.722 s / 2.297 s)**. - Configured incremental lint in the two upstream changed package roots: **0 issues**. Exact-head binary build/version, gofmt, whitespace and worktree checks pass. Native/packaged binary artifacts are preserved; the exact-head validation binary is separate. - Independent read-only integration/mechanical review **APPROVE**, covering ownership/aliasing, NULL reuse, errors, marshal and restore/clone consumers. This is local correctness evidence for this merge, not a GitHub approval, full BVT/BigData, Linux/GPU proof or performance comparison. Test-only atomic `[no statements]` is not production coverage. A fresh OPEN/Ready/head/body guard is awaited and read before the accepted normal push. The source-only SQL-task principal follow-up is not blindly copied into main's unchanged adapter/authorization path. Existing performance limitations and separate constant-view issue #29572 remain. Current exact-head [ALL CI run 37034311275](https://github.com/matrixorigin/matrixone/actions/runs/37034311275), attempt 1, completes **SUCCESS** at **2026-10-02 17:51:19 UTC**. Actual Ubuntu UT, arm64 SCA, shared build, UT Coverage, PROXY/PESSIMISTIC multi-CN BVT, Coverage and CI Required all succeed. The [CI Required log](https://github.com/matrixorigin/matrixone/actions/runs/37034311275/job/110959138144) confirms `full` scope, generation `37034311275-1`, all five dependency groups successful and `coverage_ready=true` / `coverage_status=passed`. The [Coverage log](https://github.com/matrixorigin/matrixone/actions/runs/37034311275/job/110952754024) selects UT and both complementary BVT profiles from that same generation and publishes the merged result. Current-head Connector/J succeeds; Iceberg is **SKIPPED**, not a functional test pass. No failed-job or whole-run rerun/cancellation is performed this round. The PR remains **OPEN / Ready / MERGEABLE / BLOCKED / APPROVED**. The retained approval was submitted on the earlier `93405cdc` implementation; it is not a new review of this target-merge head. All four discussion threads remain unresolved, with no new human findings. Completed CI Required and test jobs do not establish that every review/protection requirement has been satisfied; no Ready/base/approval/merge or protection policy is changed. --------- Co-authored-by: XuPeng-SH <xupeng3112@163.com>
What type of PR is this?
Which issue(s) this PR fixes:
Related to #29515. The production optimization already landed in #29508. This PR repairs its regression coverage; it does not automatically close the issue or claim to explain the conflicting latest report.
What this PR does / why we need it:
The existing #29515 BVT ignores the entire EXPLAIN ANALYZE column, including the scan counters that distinguish the reported performance defect from a correct result. It also checks only COUNT, which cannot distinguish a stale safe binding from a new safe binding returning a different row.
inputBlocks=1 inputRows=8192for direct, scalar, ABS, scalar ABS, singleton derived and derived ABS forms, plus the existing second-parameter case. Match the scan's immediately following Analyze row, excluding timings and memory statistics.0.10, preserving the existing 100,000-row sorted fixture. Incorrectly narrowing DOUBLE0.104to the stored DECIMAL value now causes a result failure.Implementation: 0 changed production lines. Tests: SQL +78 net lines, result +58 net lines; documentation: 0. Every added execution observes a distinct expression/binding transition. The existing fixture and topology are reused.
Validation
Linux/amd64, Go 1.26.4, matching native dependencies; independently built main
d59cc02ef6release binary with SIMD, isolated one-CN/TN/LOG instance and DISK-V2 storage. Tested with the published mo-tester runtime jar from main309a0ca(the compose CI Dockerfile clones mo-tester main).TestPreparedNumericPredicateFilteringpassed at the unchanged production revision.git diff --checkpassed. No Go files changed, so new Go SCA, full UT or race reruns are not required for this SQL/result-only closure.Review and remaining limitation
Design and final overall review used actual gpt-6.1-sol / xhigh, session
01a0fca6-9b52-7d22-ae0e-518dd340da19. Final decision: APPROVE, no unresolved material blocker. Commitbf1cd887bd41c4ed8f20a089928480117f89f4c7preserves the reviewed complete-diff SHA-256dce4fe300e9653534e469f209c7329b97ee4b210f6dab6a7ad493835c4c1b0b5.The latest issue comment reports 13-block scans on
d50a2d4; that behavior was not reproduced on this independently built current main. The relevant planner/binding/frontend sources are identical between those revisions. A rendered DOUBLE cast alone does not establish full scanning: current plans can display it while their actual scan reads one block. The comment's binary/configuration/execution discrepancy remains unexplained.This test-only change introduces no production runtime overhead. Local validation does not claim remote CI passed; CI is not awaited or rerun.