Skip to content

test(plan): cover prefix-index range correctness - #26852

Merged
XuPeng-SH merged 4 commits into
matrixorigin:mainfrom
LeftHandCold:fix/prefix-index-range-correctness
Aug 10, 2026
Merged

test(plan): cover prefix-index range correctness#26852
XuPeng-SH merged 4 commits into
matrixorigin:mainfrom
LeftHandCold:fix/prefix-index-range-correctness

Conversation

@LeftHandCold

@LeftHandCold LeftHandCold commented Aug 9, 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 covers:

Regression coverage for #26841. The production correction merged independently in #26829 while this PR was under review.

What this PR does / why we need it:

This branch is rebased on current main and intentionally retains only coverage not already present upstream:

  • white-box planner cases for declared prefix metadata across BETWEEN, scalar and paired ranges, IN, in_range, OR, unique/non-unique, malformed metadata, and a complete-index control;
  • FORCE INDEX FOR ORDER BY and FORCE INDEX FOR GROUP BY planning checks;
  • an end-to-end BVT covering same-prefix neighbors, forced versus ignored access, runtime bounds, flush, a complete-index control, RENAME COLUMN, and INSERT/UPDATE/DELETE after rename.

During conflict resolution, the old production implementation was deliberately not replayed: current main contains the stronger #26829 implementation, including V2 prefix-metadata support. Reintroducing the old code would duplicate validation and reject valid delimiter-bearing column names.

Validation

  • focused and complete controlled-CGo ./pkg/sql/plan tests;
  • go vet ./pkg/sql/plan with the project CGo include paths;
  • fresh make build;
  • prefix_index_range BVT: 51/51;
  • existing prefix_index_non_equality BVT: 52/52;
  • git diff --check.

@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 two correctness blockers. Both can still let a lossy prefix key exclude valid rows before the base-table predicate is evaluated.

  1. P1 — scoped FORCE hints bypass the new guards. applyForceIndexHints runs before ordinary filter index selection. For FORCE INDEX FOR ORDER BY/GROUP BY, applyForceIndexHintToScan calls tryHintedIndexAccess, and tryHintedCoveringIndexScan treats a prefix part as a complete covered column and rewrites the original filter onto serial_extract(hiddenKey, ...). It then protects the base scan, so the guard added in tryIndexOnlyScan is never reached. I reproduced this at the exact PR head with:
CREATE TABLE t(id INT PRIMARY KEY, a VARCHAR(32), KEY idx_a(a(3)));
SELECT id FROM t FORCE INDEX FOR ORDER BY(idx_a)
WHERE a BETWEEN 'abcX' AND 'abdA' ORDER BY a;

The planner still produces an idx_a covering scan carrying the range on the truncated key. Besides under-fetching, a prefix key cannot provide full-value ORDER/GROUP ordering among values sharing the same prefix. Please centralize the lossy-prefix eligibility contract (or reject these hinted covering/order/group paths) and add scoped-hint regressions with same-prefix values.

  1. P1 — semantically stale prefix metadata fails open. indexHasLossyLeadingPrefix only fails closed on parse errors; it returns prefixLengths[ResolveAlias(Parts[0])] > 0. On the current base, ALTER TABLE ... RENAME COLUMN is INPLACE and updates IndexDef.Parts/mo_indexes.column_name, but does not rename the key inside algo_params.prefix_lengths. A valid state such as Parts=["new_name", ...] plus {"prefix_lengths":"old_name:3"} therefore returns false here, while the existing physical index rows are still truncated. The new range maps can then select that index and reproduce the same wrong-result bug after a normal DDL operation. This also contradicts the PR claim that malformed metadata fails closed. Please keep prefix metadata synchronized on rename and treat orphan/mismatched prefix keys conservatively; cover rename + range before/after flush and subsequent DML.

The focused tests added by this PR pass locally, and git diff --check is clean, but they do not exercise either bypass.

@LeftHandCold
LeftHandCold force-pushed the fix/prefix-index-range-correctness branch from 6e5a9fe to ab637b0 Compare August 9, 2026 11:10
@LeftHandCold
LeftHandCold requested a review from XuPeng-SH August 9, 2026 11:11
@matrix-meow matrix-meow added size/XL Denotes a PR that changes [1000, 1999] lines and removed size/L Denotes a PR that changes [500,999] lines labels Aug 9, 2026
@LeftHandCold

Copy link
Copy Markdown
Contributor Author

Addressed review 4890910135 in the rebased head ab637b0938.

Both P1 findings were reproduced on the previous head before changing production code:

  • scoped ORDER and GROUP FORCE hints both still selected idx_a
  • Parts=[new_name] plus prefix_lengths=old_name:3 was misclassified as non-lossy

The updated contract now rejects lossy prefix indexes from scoped hint, index-only, IN, and range paths; stale metadata makes optional reads fall back. Equality remains usable only as a candidate lookup with a truncated probe and the original base-table residual.

RENAME and CHANGE now rewrite persisted prefix metadata, with an all-index preflight before mutation. Stale metadata is rejected for all four regular-index DML binders. The BVT also verifies rename followed by INSERT, UPDATE, DELETE, and flush.

During self-review, the first recorded BVT exposed an empty renamed-prefix equality result. I did not accept that generated expectation; the lookup truncation was fixed, and the final expected INSERT/UPDATE row now returns as id 9.

Validation on current main ec06a68b80:

  • full controlled-CGo ./pkg/sql/plan
  • go list, go build, and go vet for ./pkg/sql/plan
  • fresh make build
  • final rebased-binary prefix range BVT: 51/51
  • secondary index range BVT: 171/171
  • prefix index DML BVT: 36/36
  • git diff --check

@matrix-meow matrix-meow added the size/L Denotes a PR that changes [500,999] lines label Aug 10, 2026
@LeftHandCold LeftHandCold changed the title fix(plan): avoid lossy prefix index range scans test(plan): cover prefix-index range correctness Aug 10, 2026
@LeftHandCold

Copy link
Copy Markdown
Contributor Author

Rebased onto current upstream main and resolved the conflicts.

The production prefix-index correction is already in main through #26829, so I intentionally did not replay the older conflicting implementation. This PR now contains only the additional regression coverage for #26841: scoped FORCE ORDER/GROUP hints, non-equality candidate selection, complete-index control, and end-to-end rename/DML/flush behavior.

Validated with complete controlled-CGo ./pkg/sql/plan, CGo-configured go vet, make build, prefix_index_range BVT (51/51), and existing prefix_index_non_equality BVT (52/52). Please re-review.

@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.

Reviewed the complete current diff at c7e56fcf00956e540d2522bf94b695f6faefad41 (base df49b097935ccd650d9f30016d5219228e053c5e).

No blocking correctness issue found. The added coverage exercises the previously unsafe declared-prefix range paths across FORCE/IGNORE, scalar and paired ranges, OR/IN, prepared statements, unique indexes, flush, rename/DML, scoped ORDER/GROUP hints, malformed metadata, and a complete-index control. Expected plans and result sets are consistent with the fail-closed prefix-index contract. The test-only change adds no production ownership, wait, or lifecycle edge.

I also integrated this head with current latest main f9a3184c3ae251810265aef90baa7aaa8047fe23 (the regular-index cost refactor): merge was conflict-free, focused tests passed 10x, and go test ./pkg/sql/plan -count=1 -timeout=300s passed.

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/L Denotes a PR that changes [500,999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants