Skip to content

[4.2-dev] fix(frontend): preserve logical result cardinality - #26868

Merged
XuPeng-SH merged 2 commits into
4.2-devfrom
agent/fix-4.2-prepared-projection-cardinality
Aug 9, 2026
Merged

[4.2-dev] fix(frontend): preserve logical result cardinality#26868
XuPeng-SH merged 2 commits into
4.2-devfrom
agent/fix-4.2-prepared-projection-cardinality

Conversation

@XuPeng-SH

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:

issue #25992

Follow-up to #26843 (the 4.2-dev backport of #26836).

What this PR does / why we need it:

After #26843 accepts a physical length-1 const vector as a broadcast input, a
prepared grouped query can correctly produce a projection batch whose logical
row count is greater than its first vector's physical length:

SELECT ? AS projection_value, SUM(value_col) AS total
FROM metric_rows GROUP BY bucket ORDER BY total;

The frontend still derived result cardinality from bat.Vecs[0].Length().
Consequently, it emitted only one MySQL result row for a three-row batch and
also undercounted sentRows. Saved query results had a second inconsistency:
their metadata reported three rows, but object storage and result_scan
preserved only the first physical row.

This change uses batch.RowCount(), the logical cardinality propagated by the
projection operator, for protocol output and row accounting. At the query-result
persistence boundary, it validates vector shapes and creates an owned writer
view only when compact const vectors need their logical lengths normalized.
Aligned vectors are borrowed, mismatched const/const-null vectors remain
physically scalar, and the executor-owned input batch is never mutated.

Regression coverage includes flat, const, broadcast const, const-null, malformed
and zero-row batches; allocation/write failure cleanup; real object writer plus
result_scan; SQL-level prepared statement reuse; and the original GROUP BY ?
path.

Validation on the exact 4.2-dev base 9301a23cc8:

  • The direct-output BVT on the unfixed base: 19/21 statements pass; both
    parameter-first executions incorrectly return one row.
  • On the direct-output-only candidate, EXECUTE returns 3 rows but the immediate
    result_scan(last_query_id()) returns 1 row.
  • Focused frontend regressions, -count=10: pass.
  • Full pkg/frontend test suite: pass.
  • go build -mod=readonly ./pkg/frontend: pass.
  • go vet -mod=readonly ./pkg/frontend: pass.
  • make build: pass.
  • Isolated single-node fixed candidate BVT, including saved scans for first,
    NULL-reuse, and non-first parameters: 26/26 statements pass.
  • MySQL binary COM_STMT_PREPARE/EXECUTE probe: parameter-first returns 3 rows,
    NULL statement reuse returns 3 rows, and the [4.2-dev] fix(hashmap): accept broadcast const iterator vectors #26843 grouped-parameter control
    returns 1 row.

Direct output keeps an O(1) cardinality lookup. Saved aligned batches allocate
nothing beyond the existing path. A mismatched const adds an O(column count)
validation and copies only scalar physical storage; no per-row materialization
or global object-writer contract change is introduced.

@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

Copy link
Copy Markdown
Contributor Author

CI follow-up pushed in 0bfa799584.

The previous run had two independent failure classes:

  • Ubuntu UT never reached tests: the runner failed while downloading JDK 8 with Client network socket disconnected before secure TLS connection was established.
  • BVT exposed deterministic cardinality regressions after the frontend switched to Batch.RowCount():
    • OUTER APPLY appended null-extended vectors without incrementing the batch row count, hiding the final unmatched rows.
    • Query-result persistence rejected the valid payload-free zero-length const-null representation.
    • INET/INET6 expected results omitted eight NULL rows that are now correctly emitted.

The follow-up fixes both production paths, adds boundary/resume and const-null persistence regressions, and updates only the semantically missing NULL rows.

Validation on the final patch:

  • Full CGo-aware ./pkg/sql/colexec/apply and ./pkg/frontend: pass.
  • go build and go vet for both owning packages: pass.
  • make -j12 build: pass.
  • Fresh single-node 4.2-dev candidate, exact CI tester mode (-n): apply + inet + inet6 + prepared-cardinality BVT, 225/225 pass.
  • Independent owner-level review: no merge-blocking findings.

A separate pre-existing const-null/order shuffle panic found during the extended probe reproduces on the main baseline and is tracked in #26871; it is not part of this PR.

@XuPeng-SH
XuPeng-SH requested a review from LeftHandCold August 9, 2026 13:57

@LeftHandCold LeftHandCold left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep review completed on exact head 0bfa799584b300ea31b4402389ebc148149c706d against exact 4.2-dev base/merge-base 9301a23cc8f7c36aa0791b420aad8ed52e2ad3e6.

No actionable findings.

The cross-layer cardinality closure is complete:

  • protocol output, sentRows, saved-query accounting, persisted block cardinality, and result_scan now consistently use the batch logical row count;
  • compact const/const-null vectors are normalized only in an owned persistence view, aligned vectors remain borrowed, the executor-owned batch is never mutated, and partial-duplication plus writer-error paths release every temporary vector;
  • malformed flat shapes fail before accounting, while zero-row, first/non-first const, const-null, prepared reuse, and column-pruned saved-result reads retain correct semantics;
  • OUTER APPLY now increments logical cardinality for null-extended rows and resumes at the exact next input row when DefaultBatchSize is reached, without changing CROSS behavior or table-function lifecycle.

Compatibility and cost are bounded: the existing vector/object format is reused, direct output remains an O(1) row-count lookup, aligned saved batches allocate nothing, and normalization is O(column count) with scalar physical copies only. Q1-Q3 found one cleanup owner per duplicate, no new wait dependency, and no unbounded accumulation; the apply change restores the existing batch-size bound.

Independent validation on this exact head: all focused frontend cardinality/persistence tests and apply OUTER/resume tests were enumerated and passed, git diff --check passed, and current GitHub UT/SCA/coverage/BVT/CodeQL checks are green.

@XuPeng-SH
XuPeng-SH merged commit 60283ac into 4.2-dev Aug 9, 2026
32 checks passed
@XuPeng-SH
XuPeng-SH deleted the agent/fix-4.2-prepared-projection-cardinality branch August 9, 2026 14:19
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 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