fix: preserve varlena result cardinality for null rows - #26877
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
There was a problem hiding this comment.
Deep review result
[P2] Complete the cardinality repair for specialized varlena producers
The producer invariant established by this PR is correct: after a successful evaluation, a flat varlena result must publish exactly length rows, including NULL rows. However, the repair only covers the shared templates and INET6 specializations; the same malformed-result paths remain in public/specialized functions:
func_builtin_jq.go: all-null branches still callrs.AddNullRange(...)(for example lines 81, 91, 114, and 144).func_mo_tuple_expr.go: the all-null branch callsAddNullRange(line 33), while per-row NULL/invalid values call genericAppendMustNull(lines 40 and 47), which advancesFunctionResult.lengthbut not the underlying varlena vector length.func_builtin_w.go:129andfunc_builtin_onnx.go:183:IgnoreAllRowstill updates only the NULL bitmap.
This is observable at head 509c21dfccee66b634d08522bc8eb8eb712da3da. A controlled two-row probe for both try_jq(NULL, ".") and mo_tuple_expr(NULL) returned success with result.Length() == 0 and batch row count 2; calling Batch.Shuffle([]int64{1, 0}) then deterministically panicked, reproducing the same consumer failure as #26871.
Please route every full-NULL varlena return through SetNullResult(uint64(length)), use AppendMustNullForBytesResult for per-row NULLs, and extend the regression matrix to these specialized producers (including an ORDER BY/public-path witness). Otherwise the stated invariant remains function-dependent and the same panic is still reachable.
Verification completed
- Exact review range: base/merge-base
db2ef4dd03f719459c46fffec0ccdec29b2810db, head509c21dfccee66b634d08522bc8eb8eb712da3da. - New focused tests fail on the unfixed base with the intended
expected N rows but get 0/N-1 rowssignatures and pass on the PR head. - Full controlled CGo test:
./pkg/sql/plan/functionpasses. git diff --checkpasses; trial merge with currentorigin/main(1eee01fe427d90fb2bedfac1e14e9ae7fbced76f) is conflict-free.- No new goroutine, wait-for, cleanup-ownership, or unbounded-growth edge was introduced by the changed paths.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review result
No blocking findings at head 8af9257f61fbe7a59f78f9724847d5a2fdfb27fd.
The previous P2 is resolved at the producer boundary, not hidden in Batch.Shuffle:
- every full-NULL specialized varlena path now publishes
lengthphysical descriptors viaSetNullResult(jq/try_jq,mo_tuple_expr,wasm/try_wasm, andonnx_run); - mixed NULL and selected rows advance the underlying varlena vector, so later rows retain their logical positions;
- constant
jqresults are materialized for every logical row and honor selection masks; append failures also reset the reusable encoder and propagate upward; LOAD_FILEnow implements both constant and row-vector cardinality correctly, skips I/O for ignored rows, closes each reader on every exit, and bounds reads to the 64 MiB blob limit plus one byte before rejecting oversized data;- result-allocation errors in the touched specialized paths are no longer silently ignored.
I re-audited all 191 FunctionResult[Varlena] construction sites in pkg/sql/plan/function; no remaining specialized AddNullRange or generic AppendMustNull cardinality path was found. Q1-Q3 review covered success/error cleanup, partial-result failure propagation, selection masks, operator/result reuse, and restart/reset state. These changes add no goroutines, waits, locks, or shared concurrent state.
Validation performed on the pushed head:
- focused const-NULL, mixed-NULL, ignore-all, selection-mask, reuse, shuffle, JQ, tuple, WASM, ONNX, and
LOAD_FILEregressions: pass; - full controlled CGo test for
./pkg/sql/plan/function: pass; go build -mod=readonly ./pkg/sql/plan/function: pass;go vet -mod=readonly ./pkg/sql/plan/function: pass;git diff --check: pass;- exact review range: merge-base
db2ef4dd03f719459c46fffec0ccdec29b2810dbto head8af9257f61fbe7a59f78f9724847d5a2fdfb27fd; - trial merge with current
origin/main1eee01fe427d90fb2bedfac1e14e9ae7fbced76f: conflict-free.
CI was triggered by the push and was intentionally not awaited.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review result
[P2] Broadcast constant non-inline payloads instead of copying them once per row
The constant branches now preserve cardinality, but LoadFile at func_unary.go:4349-4354 and LoadFileDatalink at func_unary.go:4456-4461 materialize the same contents by calling rs.AppendBytes(contents, false) once for every row. For values larger than VarlenaInlineSize, each call reaches appendOneBytes / BuildVarlenaFromByteSlice and appends another full copy to vector.area. Consequently a legal constant file is retained length times: at the 64 MiB limit, an 8,192-row batch attempts roughly 512 GiB of result-area allocation and fails even though the value was read only once.
The vector layer already has the intended representation: vector.AppendMultiBytes materializes a non-inline value once, marks areaDisjoint = false, and broadcasts the descriptor. Please expose/use an equivalent repeated-value operation at the FunctionResult boundary, then apply the selection NULL bitmap without duplicating the payload. The same amplification exists in the new appendJqConstResult loop at func_builtin_jq.go:233-238 when a constant JQ result is non-inline.
The new constant-value regression uses only "value" (5 bytes), so it stays inline and cannot detect this behavior. Add a non-inline constant test that verifies result cardinality/selection and that retained area grows once per distinct payload, not once per output row.
Apart from this issue, the producer-boundary repair is coherent: full-NULL and per-row NULL paths publish physical varlena descriptors, selected rows retain their positions, reader/plugin/encoder ownership is closed on error and reuse paths, and no new wait-for or concurrent-state edge was introduced.
Verification
- Exact range: base/merge-base
8be242b25bf9a44d73a3b1cc1db75fd0264a293c, head5981a7d81bd472f1fa2d50f77bc881268949a5c7. - Focused cardinality, selection, reuse, JQ, tuple, WASM, ONNX,
LOAD_FILE, and Datalink tests: pass. - Full controlled CGo test
./pkg/sql/plan/function: pass. go build,go vet, andgit diff --check: pass.- Race stress was not triggered because the changed code adds no shared state, goroutine, lock, channel, or synchronization edge.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review result
No blocking findings at head 6e2be5148458e9c3c4d3b3e61f8ab57ce228a57f.
The previous P2 is resolved at the shared result-construction boundary:
FunctionResult.AppendMultiBytesexposes the vector layer's existing broadcast representation, materializing a non-inline payload once and publishinglengthdescriptors withareaDisjoint = false;- every same-result constant branch in the affected shared varlena templates now uses that boundary instead of copying the payload once per row;
- constant JQ,
LOAD_FILE, Datalink, andmo_tuple_expruse the selection-aware wrapper, preserving NULL positions without duplicating selected-row payloads; mo_tuple_expralso decodes/formats a constant tuple once, while vector inputs retain per-row evaluation;- shared-template paths do not pay a redundant selection scan because they already establish their result NULL bitmap before the constant branch.
The physical retention cost is now O(payload + rows * varlena descriptor) instead of O(payload * rows). The pre-fix regression produced exact amplification witnesses: a 40-byte payload retained 120 bytes in the shared-template cases and 80 bytes in the two-valid-row JQ/LOAD_FILE/Datalink cases. The same tests now retain exactly one payload while preserving cardinality, selection masks, and returned values.
I re-ran the full PR review across correctness, compatibility, failure handling, performance/scale, lifecycle, and Q1-Q3 unhappy paths. Broadcast descriptors have one vector-area owner; downstream vector code observes areaDisjoint = false; encoder buffers are reset only after the payload copy; file readers and WASM plugins retain their existing exactly-once cleanup paths. No new goroutine, wait-for dependency, shared state, or generation transition was introduced.
Verification
- Exact range: base/merge-base
8be242b25bf9a44d73a3b1cc1db75fd0264a293c, head6e2be5148458e9c3c4d3b3e61f8ab57ce228a57f. - Focused non-inline broadcast, selection, template, JQ, tuple,
LOAD_FILE, and Datalink regressions: pass. - Full controlled CGo tests:
./pkg/container/vectorand./pkg/sql/plan/functionpass. go build,go vet, andgit diff --checkpass for both owning packages / the full PR diff.- Race stress was not triggered because this remains sequential vector construction with no concurrency or synchronization edge.
- CI was triggered by the push and was intentionally not awaited.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review result
No blocking findings at head 42aaf143a1e6ef928b9a74c5b2053be75d095a31.
Reviewed the complete PR against origin/main (8be242b25bf9a44d73a3b1cc1db75fd0264a293c):
- varlena producers preserve physical cardinality across NULL, selection, zero-row, reuse, and shuffle paths;
- repeated non-inline payloads are materialized once, reducing retention from
O(payload * rows)toO(payload + descriptors * rows); - JQ encoder, file readers, and WASM plugin ownership close on success/error/reset/free paths;
- TRY-WASM only converts per-row invocation failures to NULL; setup/load/instantiation failures propagate;
- no new goroutines, locks, waits, retries, or shared concurrent state were introduced;
- Q1-Q3 audit found no leak, hang, or unbounded-growth blocker.
Validation is complete: focused tests, full owner packages, build, vet, git diff --check, and related SQL BVTs passed. Current CI checks are green, including UT, coverage, Proxy/Standalone BVT, SCA, and CodeQL.
Code-level conclusion: approve. The current account is the PR author, so GitHub cannot record an APPROVE review from this account; this review is submitted as a comment and requires an external reviewer for approval.
What changed
Root cause
Varlena function result construction had paths that marked rows NULL without extending the result vector. The batch still reported N rows while the produced vector had fewer (or zero) descriptors. ORDER BY shuffle was the first consumer to index that malformed vector and panic. The fix restores the producer invariant: every successful evaluation publishes exactly the requested number of result rows, including NULL rows.
Validation
Fixes #26871