Skip to content

fix(hashmap): accept broadcast const iterator vectors - #26836

Open
LeftHandCold wants to merge 12 commits into
matrixorigin:mainfrom
LeftHandCold:fix/issue-25992-const-vector-hash
Open

fix(hashmap): accept broadcast const iterator vectors#26836
LeftHandCold wants to merge 12 commits into
matrixorigin:mainfrom
LeftHandCold:fix/issue-25992-const-vector-hash

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 fixes:

fixes #25992

What this PR does / why we need it:

Root cause

There were three constant-vector cardinality violations on prepared aggregate paths:

  1. fix(plan): preserve prepared aggregate parameters #26042 fixed prepared aggregate parameter identity. A later validation added by executor: replace HashBuild estimates with allocation-owned admission #26531 required every hashmap input vector to physically cover [start, start+count). Prepared parameters intentionally return a constant vector with one stored value, while hash encoders broadcast that value across the caller's logical range. The validator rejected the vector before encoding when count > 1, returning invalid allocation account for bare GROUP BY ?.
  2. A prepared parameter in the first projection column also produces a one-value constant vector. Projection correctly retained the input batch's logical row count, but the MySQL output path used bat.Vecs[0].Length() as the result cardinality. Grouped output with three logical rows therefore emitted only one row through both text and binary prepared-statement protocols. The same physical length was recorded as statement_info.result_count.
  3. With save_query_result=on, the same batch is persisted before it is sent to the client. saveBatch used the first vector length for saved/query row accounting, and objectio serialized the one-row logical length carried by the prepared constant. The client could receive three rows while saved-result metadata, the block header, and a column-pruned result_scan exposed only one row.

Fix

  • Preserve strict row-bound validation for ordinary flat hashmap vectors.
  • Accept non-empty constant hashmap vectors for any logical iterator range, matching the existing hash encoder broadcast contract.
  • Continue rejecting zero-length constants when rows are requested.
  • Use Batch.RowCount() as the authoritative client-output, sent-row, and saved-result cardinality.
  • At the query-result persistence boundary only, duplicate constants whose vector length does not cover the batch's logical row count and adjust the temporary copy's logical length before serialization. The executor-owned vector is never mutated, constant payloads remain physically compact, and the temporary copies are released on success and error paths.

This changes neither allocation accounting nor hashmap capacity admission. Ordinary queries do not enter the saved-result normalization path, and normal flat result batches are unchanged because their vector lengths already equal their logical row count.

Tests

  • Both IntHashMap and StrHashMap broadcast a one-value constant beyond its physical storage.
  • Constant NULL forms one valid NULL group; malformed vectors remain rejected.
  • A logical three-row batch whose first projection vector stores one constant value emits three text-protocol rows and three binary-protocol rows, and records three sent rows.
  • Saving that batch records three query/saved rows, persists a three-row block, and reading only the constant column returns all three values while leaving the executor-owned vector at length one.
  • The saved-result regression failed on the previous head in all four cardinality assertions with expected: 3, actual: 1; it passes for 10 consecutive runs after the fix.
  • Full pkg/frontend and pkg/common/hashmap package tests, plus owning-package build and vet, pass.

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

@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Aug 9, 2026
@mergify mergify Bot added the kind/bug Something isn't working label Aug 9, 2026
mergify Bot pushed a commit that referenced this pull request Aug 9, 2026
Backport #26836 to `4.2-dev`.

#26042 fixed the original prepared-parameter identity problem. Later, #26531
added strict HashMap iterator row-range validation and incorrectly treated a
physical length-1 const vector as a flat vector. Prepared `GROUP BY ?` therefore
fails whenever the input batch contains more than one logical row.

This change preserves strict bounds for flat vectors while accepting non-empty
const and const-null vectors as broadcast inputs, matching the existing IntHashMap
and StrHashMap encoders. Zero-length const vectors are still rejected whenever
rows are requested.

Validation on the `4.2-dev` base (`4066fffaf7`):

- malformed-flat and const-broadcast regression tests, `-count=10`
- full `pkg/common/hashmap` test and race test
- `go build` and `go vet` for `pkg/common/hashmap`
- `validateIteratorVectors` coverage: `100%`

The cherry-picked commit retains `-x` provenance for #26836.

Approved by: @XuPeng-SH

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

Blocking correctness issue: #25992 remains only partially fixed. On the exact PR head (f3cda59478f8120a7377cd36a35554cff3885e5d), a prepared projection constant still silently collapses grouped output:

CREATE TABLE metric_rows (bucket VARCHAR(20), value_col INT);
INSERT INTO metric_rows VALUES ('a', 10), ('b', 20), ('c', 30);

PREPARE p FROM
  'SELECT ? AS projection_value, SUM(value_col) AS total
     FROM metric_rows
    GROUP BY bucket
    ORDER BY total';
SET @v = 7;
EXECUTE p USING @v;

Actual result:

7  10

Expected result, confirmed by the nearest literal oracle (SELECT 7 AS ...) and by selecting bucket:

7  10
7  20
7  30

I reproduced the same one-row result through both text PREPARE/EXECUTE and the MySQL binary prepared-statement protocol. EXPLAIN for the prepared and literal forms both retains Aggregate Group Key: bucket, so this is execution/output cardinality loss rather than the grouping being optimized away.

The iterator validation change does fix the other reproduction (GROUP BY ?) and its hashmap unit/race/counterexample tests pass. However, this PR says Fixes #25992; merging it would close that issue while its silent wrong-result branch remains.

Please either fix the projection-constant broadcast/result-cardinality path and add text plus binary-protocol regression coverage, or track that branch in a separate issue and adjust the issue-closing semantics before merge.

@LeftHandCold

Copy link
Copy Markdown
Contributor Author

Addressed the blocking saved-result cardinality review in 332c8902c4, fcdaacb0da, and 617b17a587.

Root cause confirmed: the public INET/INET6 NULL shape can carry Batch.RowCount() == 1 with a flat, non-const varlena vector whose physical Length() == 0 and whose NULL bitmap already contains row 0. The client path now respects the logical row count, but objectWriterV1.addBlock derives persisted block rows from vector length, so the prior normalizer still wrote a zero-row block.

The saved-result boundary now enforces one invariant before writer.Write: every persisted vector must physically cover Batch.RowCount().

  • Const vectors are copied and broadcast to the logical row count.
  • A short flat vector is accepted only when every missing trailing row is explicitly marked NULL; those rows are physically materialized with UnionNull.
  • Nil vectors, oversized flat vectors, and missing non-NULL rows fail closed instead of fabricating data.
  • Executor-owned vectors are never mutated.
  • The ordinary already-consistent path performs only checks; copies and cleanup closures are created only for mismatched vectors.
  • All temporary copies have one cleanup owner on normalization errors, writer errors, WriteEnd errors, and success.

Deterministic evidence:

  • Before the fix, the exact flat-NULL regression saved metadata count 1 but persisted block/load row count 0.
  • After the fix, metadata, block header, loaded batch, and loaded NULL vector all report one row.
  • A mixed ["value", NULL] varlena case proves existing data is preserved while only the trailing NULL slot is materialized.
  • Counterexamples cover missing non-NULL rows, oversized vectors, nil vectors, and mpool cleanup.

Fresh validation on head 617b17a587:

  • focused four saved-result tests: PASS
  • full ./pkg/frontend: PASS
  • full ./pkg/common/hashmap: PASS
  • owning-package build and vet: PASS under the controlled CGo environment
  • git diff --check: PASS

This does not depend on #26877. That PR remains a useful upstream producer-invariant repair, while this change independently makes the persistence boundary correct and fail-closed for every supported cardinality mismatch.

@LeftHandCold

Copy link
Copy Markdown
Contributor Author

Fixed the SCA prealloc finding in prepareQueryResultBatchForWrite.

The fast path still returns an already row-aligned batch without allocating. When normalization is required, the helper now allocates the clone slice once at len(bat.Vecs) before duplicating vectors. This preserves the existing clone/release and error-cleanup ownership paths while removing repeated slice growth.

Validation: golangci-lint v2.6.2 (Go 1.26.4) reports 0 issues for pkg/frontend with the project CGo headers; focused normalization/save-result tests pass; full controlled-CGo pkg/frontend tests, build, and vet pass.

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

Deep review result

[P2] Enforce query_result_maxsize on the normalized batch

saveBatch computes s from the executor-owned batch before prepareQueryResultBatchForWrite establishes the physical cardinality that is actually serialized. Vector.Size() is based on Vector.Length(), so every shape added by this PR can be under-accounted: a broadcast constant is charged for its old length, and a trailing-NULL vector is charged before its missing rows are materialized.

The exact public shape already covered by this PR makes the violation deterministic:

vector: const NULL varchar, Length() = 0
batch:  RowCount() = 1
query_result_maxsize = 0
expected savedRowCount: 0
actual savedRowCount:   1

On this head, bat.Size() is zero, the limit check admits the batch, normalization changes the persisted vector to length 1, and the writer stores that row. curResultSize nevertheless remains zero while savedRowCount becomes one. This breaks the configured cap and leaves metadata size inconsistent with the batch that was written. The same counterexample on the exact base keeps savedRowCount at zero.

Please normalize first, perform admission and curResultSize accounting with writeBat.Size(), and release any temporary clones when the batch is rejected by the limit. Add a regression for the length-0/logical-1 NULL shape with query_result_maxsize=0 or another limit between the source and normalized sizes.

No other blocking findings remain in the reviewed hashmap broadcast paths, grouping/NULL/NaN handling, text and binary row output, normalized-vector ownership and cleanup, object write/read cardinality, or BVT expectation changes.

Validation on exact range 8be242b25bf9a44d73a3b1cc1db75fd0264a293c..b7e932d9f450f1449f13119429d88d271bb2dd0c:

  • changed hashmap and frontend tests: PASS at -count=10
  • changed tests under -race: PASS
  • full ./pkg/common/hashmap and ./pkg/frontend: PASS
  • owning-package build and vet: PASS
  • git diff --check: PASS
  • deterministic size-limit counterexample: FAILS with savedRowCount actual 1; exact-base control: PASS with 0

Comment thread pkg/frontend/query_result.go Outdated

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

Deep re-reviewed exact head 05cb1e22b9598e1fdc5708cd6da906e5118ae6dd. The previous query_result_maxsize blocker is closed: normalization now precedes admission, the limit and curResultSize use writeBat.Size(), and the cleanup defer is installed before the rejection branch.

The complete final paths are consistent:

  • IntHashMap and StrHashMap encoders already broadcast constant values from physical row zero while flat-vector bounds remain strict.
  • MySQL text and binary output, sent-row accounting, saved-result metadata, and persisted block rows all use the batch logical cardinality.
  • Saved-result normalization handles constants and explicitly-null trailing flat rows, rejects unsupported mismatches, and never mutates executor-owned vectors.
  • Temporary clones have one cleanup owner on validation failure, size rejection, writer/WriteEnd failure, and success; clone accumulation is bounded by batch columns and there are no new wait edges.

Local validation passed on this head: changed tests at high repeat counts, focused race tests, full pkg/common/hashmap and pkg/frontend, build, vet, and diff check. I also verified a counterexample where an earlier column is cloned before a later column fails validation; the mpool returns to its baseline. No blocking issue found.

@mergify

mergify Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

  • Entered queue2026-08-10 08:12 UTC · Rule: main · triggered by rule Automatic queue on approval for main
  • 🟠 Preparing checks
  • ⏳ Merge · ETA: 2026-08-10 09:28 UTC 🚀

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Prepared GROUP BY parameter markers produce invalid parameter metadata

4 participants