perf(test): share compatible SQL fixtures and reuse pinned UT tooling - #28671
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? |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review at 06c35af, base main fffcacc: no concrete blocking finding. COMMENT only for this self-authored PR.
Reviewed every changed hunk, the versioned fixture design, complete moved scenarios, shared-cluster lifecycle, dependency-based UT routing, report consumer and builder cache export. All seven original tests remain named and discoverable; their data/topology/assertions and existing CDC CI skip are retained. No new parallel test execution, retry of a failed scenario, or weakened assertion is introduced; checking ExecTxn's final error and closing the catalog result strengthen the existing path.
Lifecycle closure: deferred database cleanup uses the CN frontend with a fresh 30-second context while Run still holds the fixture mutex, including FailNow unwinding. The registered testing cleanup closes a failed fixture only after Run releases that mutex. CloseIfActive resets a successfully closed generation and poisons a failed-close fixture instead of handing partially closed state to the next scenario. TestMain owns final successful-fixture cleanup and makes close failure fail the package. Existing CDC hook restoration and PITR/role cleanup remain in their original sequential scenarios. There is no new goroutine, channel, unbounded cache, or concurrent cluster owner here.
Tooling closure: only an executable with matching module-version metadata and no replacement is reused; unreadable/stale/replaced metadata retains bounded installation retries. The report command's failure still contributes to the final UT status. Builder warming exports module/build caches, not a guaranteed ready binary, consistent with the documented conditional benefit. No production SQL/wire/catalog contract changes.
Evidence/limits: source-derived closure plus the PR's reported local race/shuffle/count=3 results (18 passes and three unchanged CDC skips), optools race/count=3 checks, and explicitly single-sample before/after package timings. I did not independently rerun native UT/BVT, build the image, or wait for CI. Full Linux job critical-path/resource gains and numerical coverage delta remain unmeasured here; those limits do not establish a correctness blocker. The shared-fixture and unhappy-path review contracts guided the cleanup/ownership checks above.
## What type of PR is this? - [ ] API-change - [x] BUG - [ ] Improvement - [ ] Documentation - [ ] Feature - [ ] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: #28392, #28460, #28461 ## What this PR does / why we need it CONV/BIN accepted several typed domains at bind time but dispatched through string-only or incompatible accessors at execution time. Prepared markers made the gap observable because the runtime wire domain could change between executions. Numeric prefixes and MySQL's FLOAT/DOUBLE textual representation also had boundary mismatches. The change keeps the existing integer/string fast paths and closes the common typed dispatch boundary: - dispatch BOOL, BIT, DECIMAL, temporal, binary-string, and deferred prepared values through type-correct overloads; - preserve signed source-domain semantics for from_base, including integer, decimal, float, and temporal prefixes; - use bounded MySQL-compatible FLOAT/DOUBLE formatting; - treat raw HEX/BIT literals as numeric bit patterns when they fit in 64 bits, while keeping empty/over-wide literals bounded and deterministic; - reject unsupported fixed-width operands with a normal error instead of a varlena-accessor panic; - discover conversion marker positions once per prepared-plan generation and reuse the existing bounded runtime-plan cache without stale parameter references. ### Review follow-up: binary protocol BIT domain MYSQL_TYPE_BIT is an opaque bit-domain value. Its binary-protocol unsigned flag describes wire encoding, not a semantic conversion to UINT64. The prepared binary path now preserves T_bit for both flag variants. The packet-path regression test exercises protocol parsing, execute-time binding, expression execution, signed/unsigned BIT values, ordinary signed/unsigned LONGLONG, and BIT rebinding after an integer execution. ### Compatibility boundary The final branch is rebased onto main at c4053fd (main protocol version 63) and allocates the next cumulative version, MORPCVersion64, for the changed typed BIN/CONV execution contract. MORPC versions are cumulative and must be allocated in actual merge order: concurrent feature branches that provisionally use v64 must rebase and take the next value before merging. This PR does not claim interoperability between independently built sibling branches; sender and receiver continue to fail closed below v64. ## Validation / test plan All commands use the repository CGo wrapper where required: - frontend full UT: mo-cgo-test -count=1 -timeout=900s ./pkg/frontend — PASS; - function and plan support race suite: mo-cgo-test -race -count=1 -timeout=600s ./pkg/sql/plan/function ./pkg/pb/plan — PASS; - compile/plan protocol and prepared-runtime race tests — PASS; - COM_STMT_EXECUTE BIT-domain regression race×5 — PASS; - incremental go vet for changed packages — PASS; - incremental golangci-lint --new-from-rev origin/main for the changed package closure — 0 issues; - gofmt and git diff --check — PASS. The packet regression proves the protocol-parser -> execute binder -> expression-executor contract; the retained prepared-cache tests separately cover same-domain reuse, type replacement, parameter-reference restoration, and cleanup. No new unbounded cache, wait, goroutine, or per-row scan was introduced. The final rebase also included main's #28671 test-infrastructure change; the owning package suites were rerun on the final head. Fresh CI/BVT on the pushed head remains the authoritative clean Linux and distributed validation and is intentionally not awaited here.
2 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #28419
What this PR does / why we need it:
DDL and transaction-executor tests used identical single-CN fixtures in separate Go test processes. Move all seven tests into
pkg/tests/sqlintegrationso one fixture serves both groups. This removes one complete cluster startup without adding runners, increasing parallelism, reducing data, or removing assertions. Distinct Arrow, shard, partition and upgrade configurations remain isolated.Drop test-created databases through the CN frontend before releasing the shared fixture lock; discard the fixture after a failed scenario and close it explicitly at package exit. Preserve existing PITR/role cleanup and the CDC external-MySQL CI skip. Also check the prepared-parameter transaction result and close the catalog-read result. Emit scenario-body timing separately from initialization.
Reuse an installed report tool only when build metadata matches its pinned version with no replacements; otherwise retain bounded install retries. Warm that tool in the CI builder module/build caches. This benefit depends on a reusable binary or restored caches and a refreshed builder image.
Validation and measured effect
-race -short -tags matrixone_test -p 1 -count=1: original packages 14.322s + 14.034s = 28.356s; consolidated package 15.782s. Service-start events decreased from two to one. These are single local samples and package execution durations, not a CI job benchmark.-race -count=3 -shuffle=on: passed in 17.872s; 18 passing test executions plus three unchanged CDC skips. All seven original test functions retained.go test -race -count=3 ./optools: passed (36.380s), including pinned/stale/unreadable/replaced tool metadata and existing retry tests.matrixone_test: 0 issues.The image build and full Linux CI remain to be verified by CI. No numerical coverage delta has been measured locally; test inventory, inputs, assertions, instrumentation and automatic coverage discovery are preserved. CI critical-path improvement must be measured on this PR; cumulative admission waiting is not additive wall-time savings.