Repository navigation
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? |
|
@iamlinjunhong Addressed your P1 finding on exact head
Validation: focused planner regression PASS, full |
|
@XuPeng-SH Addressed your P1 finding on exact head
Validation: focused planner regression PASS, full |
|
Follow-up for exact head
|
aunjgr
left a comment
There was a problem hiding this comment.
Approved at exact head 325686d29c17b3729f32b837b5bc2144d33f0a9b against 4.2-dev@d2393868a7aaa6343518d80849fe4696aa3577e9.
The bounded manifest identifies the fixed main-line behavior closure and the 4.2-specific parser/PERFORM/pipe adaptations. FULLTEXT2 remains separately registered from classic FULLTEXT, with catalog/compile/plan/idxcron/ISCP interface assertions, CPU registration, table-function wiring, CDC/ISCP imports, and public SQL coverage intact. The source/target ownership, cache generation, position-free, persistence/restore, and readiness paths are represented in the backport tests.
The latest JOIN-child fix now collects wrapped-only FULLTEXT2 MATCH drivers before the early return, routes them through the existing served-match rewrite, lifts the score comparison into a post-search FILTER, and keeps upper-bound-only/nonmembership predicates fail-closed. Both INNER JOIN child positions and qualifying/rejecting thresholds are covered at planner and BVT layers. The structural plugin guards found no removed assertions, plugin back-import cycle, or missing registration seam. I found no remaining release-branch correctness or lifecycle blocker.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Approved exact head 325686d against 4.2-dev@d2393868a7aaa6343518d80849fe4696aa3577e9. Re-reviewed the complete delta after my prior CHANGES_REQUESTED at a8476d6: wrapped-only FULLTEXT2 MATCH predicates are now collected before the join-child early return, both INNER JOIN child positions route through the existing wrapped rewrite, and the exact score comparison remains in an executable FILTER above the generated search join. The qualifying and rejecting thresholds are covered in planner tests and in the selected fulltext2_wrapped_score BVT. Exact-head UT, SCA, coverage, build, proxy BVT, and pessimistic BVT are green. I found no remaining correctness, liveness, lifecycle, compatibility, or material performance blocker.
iamlinjunhong
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 325686d29c17b3729f32b837b5bc2144d33f0a9b against 4.2-dev@d2393868a7aaa6343518d80849fe4696aa3577e9 after my previous CHANGES_REQUESTED.
No findings. The prior P1 is resolved: the JOIN-child path now collects wrapped-only FULLTEXT2 MATCH drivers before the no-filter return, routes both INNER JOIN child positions through applyJoinFullTextIndicesWithWrapped, and evaluates the exact score comparison in a real FILTER above the generated search join. Planner coverage verifies both child positions; the canonical BVT verifies qualifying and rejecting thresholds.
I also re-inspected the complete backport range and its parser/catalog/DDL, plugin registration and interface assertions, table-function execution, CDC/ISCP error propagation, cache-generation and cleanup lifecycle, persistence/restore paths, and focused test coverage. The index-plugin structural searches found no removed assertions, plugin-to-SQL import cycle, or missing CPU/ISCP registration seam. The exact-head pre-merge CI suite was green. The later merge-trigger TKE failure is infrastructure-only (moby/buildkit manifest request returned EOF while the workflow checked out main), not a PR-code failure.
## What type of PR is this? - [ ] API-change - [ ] BUG - [ ] Improvement - [ ] Documentation - [ ] Feature - [x] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: issue #25541 ## What this PR does / why we need it: Adds deterministic package-level acceptance coverage for the WAND retrieval paths delivered by merged PR #27663 on `4.2-dev`. - Exercises WAND routing and BlockMax candidate pruning with independent probe counters on both build and serialize/load paths. - Verifies BM25 and TF-IDF score-to-PK results with an independent oracle, including ties. - Covers loaded-index and CDC update/delete/insert liveness in the test fixture. - Makes the base-segment ownership transfer explicit so failed tail assembly releases the base exactly once. - No production code, SQL, protocol, or runtime hot-path instrumentation is changed. - This follow-up depends on the merged FULLTEXT2 backport in #27663; it does not modify that PR's owner worktree. Validation: - Unit/integration: PASS — focused normal/race and owning CGo tests pass, including build and serialize/load BlockMax probes, independent score↔PK checks, ties, CDC liveness, and cleanup. Evidence: `/tmp/wand-qa-postcommit-focused-20260911.log`, `/tmp/wand-qa-postcommit-focused-count3-20260911.log`, `/tmp/wand-qa-postcommit-owning-20260911.log`, `/tmp/wand-uncommitted-race.log`. - BVT: N/A — this test-only change does not alter SQL-visible behavior. Candidate-admission counters and controlled block metadata are package-internal observation points; focused package tests cover them. This does not replace the existing SQL BVT or deployed QA acceptance for issue #25541. - Base behavior: PASS — the merged #27663 implementation is unchanged; this PR adds the missing acceptance assertions. - PR-head behavior: PASS — local focused, race, build/vet, and owning package CGo tests pass at head `b3844d544f6b92ae2f2eb65c9a659d5fa866d9b7` on base `4199b11c5c7bb24eccdb379cd6f68abcbd89e450`. Exact evidence: - test file SHA-256: `882b3736e3b078b40181dd234aa1208e91cbc2b56b2a76239fb86cac3437e1b7` - final patch delta SHA-256: `801f73e0d5060f32939714471b36dc31e4763a657d2d7771bba8bcddc623ce34` QA required: yes — issue #25541 is marked `phase/testing` and acceptance includes deployed/lifecycle evidence beyond package tests. Remote deployment, CI, and formal QA remain pending. ## Remaining validation - Exact-head CI and maintainer review are pending. - Official 4.2 deployment, GitOps/TKE shakeout, continuous run, and QA acceptance are not run in this local patch validation. --------- Co-authored-by: XuPeng-SH <xupeng3112@163.com>
What type of PR is this?
Which issue(s) this PR fixes:
issue #25541
What this PR does / why we need it:
This is a bounded backport of the FULLTEXT2 behavior closure from the fixed
upstream/mainreference to4.2-dev. It keeps FULLTEXT2 distinct fromclassic FULLTEXT and preserves the target branch's parser/PERFORM/pipe
contracts.
Source/target closure:
upstream/main:8e616a693458652cc26b3c4be45c6602dff298d4upstream/4.2-dev:d2393868a7aaa6343518d80849fe4696aa3577e9325686d29c17b3729f32b837b5bc2144d33f0a9bfulltext2— complete rewrite of fulltext index and add a position-free BM25 full-text retrieval index #25904 core:966acb2a92bb740e57b0b7a181df2d21956e1d3,0eb9a921ece274c956447640b41ad2a255079e8d,403a99fb65304e75b82e75793448f0690a871d0e,bfa83f42737ca6304c0af3f6913e70ccc9614642b142144c3c24a2100d7d313e952717025d56aeb835e046faa9242dbe288bd72364a475e2132bbede4abf5e90d0b5e09590571ca78447e50806527ce3a7440e2ac32bac780669f33c656960e22cff22Current-head review repairs:
</<=before harvesting a lower-bound MATCHmembership driver; reject constant-left
>/>=upper bounds fail-closed.report
ctx.Err()on cancellation.fulltext2_wrapped_score.sql/.resultBVT pairand replace restore/clone fixed sleeps with readiness waits.
git diff --checktrailing whitespace findings.boundary, preserving the shared renderer and vector-index behavior.
through the existing wrapped rewrite and keep the score comparison in a
post-rewrite FILTER.
Validation
make -C pkg/sql/parsers/dialect/mysql— PASSmake generate-pb— PASS; protoc 29.3 descriptor-only churn was discardedand no unexpected generated diff remains
go test -mod=readonly ./pkg/fulltext2/... ./pkg/iscp ./pkg/sql/plan— PASSGOWORK=off go test -mod=readonly -count=1 -timeout 120s -run '^TestFulltext2WrappedMatchJoinChild$' ./pkg/sql/plan— PASSGOWORK=off go test -mod=readonly -count=1 ./pkg/sql/plan— PASSGOWORK=off go build -mod=readonly ./pkg/sql/planandGOWORK=off go vet -mod=readonly ./pkg/sql/plan— PASSapplyJoinFullTextIndiceswrapper; targeted plan tests, build, and vet pass; exact-head remote SCA is
pending
mo-cgo-testwrapper — BLOCKED by missingcgo/libmo.dylib; no CGowrapper pass is claimed
go test -race -mod=readonly ./pkg/fulltext2/... ./pkg/iscp— PASSgo build -mod=readonly ./cmd/mo-service— PASS (macOS linker rpath/pathwarnings only)
d2393868a7aaa6343518d80849fe4696aa3577e9rejects
CREATE FULLTEXT2 INDEX; this exact head parses it asINDEX_CATEGORY_FULLTEXT2child positions with qualifying and rejecting thresholds; exact-head remote
BVT remains pending
PASS semantic=PASS, diff hashefc51153938b14e051372026b296c06e05696385ac674fa7f05f3a7217741d09Scope and acceptance boundaries
QA required: yesRequired CI, formal reviews, and Mergify remain pending; no remote pass is
claimed until they bind to this exact head.
not acceptance by themselves. No classic FULLTEXT result, fixed-commit
product acceptance claim, or formal QPS claim is used here.
freetier-01workload remain post-mergegates. After merge, only the official immutable image/release workflow may
produce the tag/digest;
freetier-01must be upgraded through the reviewedGitOps
environment=new-dev + mode=mo + action=setPR. No local image,direct
4.2-devpush, direct GitOps edit, kubectl mutation, or pod restartis authorized.
FULLTEXT2 shakeout with readiness polling, token-bucket DML, C1/C10 readers,
and an independent correctness ledger. Only
MEASURED_PASSwith zerocorrectness errors, no backlog, and complete observations may unlock the
low-load two-hour continuous run.
Please keep issue #25541 open until QA explicitly reports PASS/FAIL.