fix: preserve string source provenance across execution - #27467
Conversation
# Conflicts: # pkg/defines/const.go
# Conflicts: # pkg/pb/pipeline/pipeline.pb.go
aunjgr
left a comment
There was a problem hiding this comment.
Deep review of exact head 3ab9dce4f816849fc9fd09993468ca1cfe52d3cc against merge base 22b91fe986b269d24a6b8816eafdd1fa6327c8e1.
[P1] Preflight source-preserving aggregate sidecars before publishing the group hash
The group-key source path correctly reserves its sidecars before CommitPreview, but the aggregate half does not. Group.buildOneBatch calls each aggregate's PreflightBatchFill at pkg/sql/colexec/group/exec2.go:499-506, commits the hash/key state at lines 513-520, and only afterward calls GroupGrow and BatchFill at lines 522-532. Errors from those post-commit calls are returned directly; they cannot enter the pre-publication spill/retry path.
The updated preserving aggregates still leave source allocations until that post-commit phase:
anyExec.PreflightBatchFillmodels the winner's varlen area andPrepareParamKind, but notStringSource(aggexec/capacity_preflight.go:768-825);BatchFilllater callsSetRawBytesAtFromAndUnsetNull, which can allocate the first per-row source sidecar (aggexec/any2.go:37-52).- fixed MIN/MAX preflight's candidate/event model carries only value and prepare kind (
capacity_preflight.go:1024-1155), whileBatchFillfirst callsPreflightSetStringSourceAtataggexec/minmax2.go:92-125. - byte MIN/MAX and MAX_BY have the same omission; equal-candidate source merging is especially missed because MAX_BY's preflight skips a candidate that does not replace the value winner, while runtime still merges its source.
A minimal reachable failure is an allocation-accounted GROUP with an existing aggregate state whose uniform source is Literal, followed by a unit that introduces a new group or an equal/winning COMStmt candidate. The preview preflights successfully and publishes the new hash group. GroupGrow/aggregate fill then needs the first mixed-source sidecar; if the bounded allocation controller rejects it, the statement returns an allocation error instead of spilling the resident prefix and retrying. The committed group state prevents safe retry. This contradicts the PR's claimed “source-sidecar allocation rejection before hash commit” closure.
Extend PreflightBatchFill/PreflightBatchMerge for every source-preserving aggregate to compute the exact winning/equal source events and reserve destination sidecars—including future group rows—before CommitPreview. After commit, GroupGrow, BatchFill, and BatchMerge must be non-allocating for this metadata. Add deterministic Group and MergeGroup regressions with the allocation controller rejecting the first source allocation after the preview; assert the rejection occurs pre-publication, spill/retry succeeds, and final winner/tie sources are correct. Include ANY_VALUE, fixed/byte MIN/MAX, and MAX_BY equal-candidate controls.
I also audited vector lifecycle/reset, planner literal/deep-copy/hash identity, v29 batch/process/aggregate transport downgrade, flow-control merge policies, and invalid-enum decode cleanup. I found no additional confirmed blocker.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep-reviewed exact head 3ab9dce4f816849fc9fd09993468ca1cfe52d3cc, including the provenance ownership/merge rules, vector lifecycle, aggregate/group publication order, transport boundaries, and prior review findings.
The overall independent source axis is reasonable, but its allocation and propagation contracts are not closed yet:
-
[P1 lifecycle] Include the mixed-source sidecar in generic vector pre-extension.
Vector.PreExtenddelegates toextendWithBitmaps, which reserves data, requested bitmaps,prepareParamKinds, and runtime-domain metadata, but neverstringSources. For a flat vector already carrying mixed row sources,PreExtend(64, mp)can therefore succeed while the source sidecar still has capacity only for the existing rows; the laterSetLengthcallspreExtendStringSourcesand allocates/panics after the caller treated pre-extension as admission.This new row-parallel metadata must participate in the same generic capacity contract as every existing sidecar.
-
[P1 correctness] Preserve NULL source ownership in the
T_anytime-window partition materializer.getPartitionSetFunction's specialT_anyclosure callsSetConstNulland clears prepare-param metadata, but does not copyw.GetStringSourceAt(sel). A SQLPrepare/COMStmt NULL therefore becomesStringSourceExpressionat this otherwise transparent boundary, contrary to the PR's explicit invariant that NULL retains its independent source. Copy/reset the selected source exactly and cover constant and selected NULL cases. -
[P1 atomicity/design] Preflight aggregate provenance before publishing GROUP state.
Group.buildOneBatchpreflights aggregate fills, commits the hash/key preview, grows groups, and then performsBatchFill. The new source-preserving aggregate preflights still model values and prepare kinds but omit the winning/equal source events. Runtime ANY_VALUE, fixed/bytes MIN/MAX, and MAX_BY can consequently allocate the first mixedstringSourcessidecar only after the hash group is committed; rejection at that point cannot safely enter the existing spill/retry path.Extend both fill and merge preflights to model exact winner/tie source merges and reserve current/future destination rows before
CommitPreview. After commit,GroupGrow,BatchFill, andBatchMergemust not allocate this metadata. Add allocation-rejection regressions for ANY_VALUE, fixed/bytes MIN/MAX, and MAX_BY equal-candidate behavior through Group and MergeGroup/spill retry.
These are manifestations of one design invariant: once provenance is independently row-owned state, every generic capacity/publication boundary must admit it atomically with the value, not as an operator-specific afterthought.
aptend
left a comment
There was a problem hiding this comment.
Deep review completed at exact head 2d52c8d, including the full diff, all prior reviews/inline threads, and the delta since 3ab9dce.
The two previously reported gaps were addressed, but two publication-boundary allocation gaps remain. Both are reproducible by allowing the outer preflight, rejecting the next allocation, and then running the supposedly preflighted mutation. These can make execution fail after logical/vector or group-hash publication, defeating the preflight/retry contract.
Validation on this head:
- related Go package suite passed
- focused provenance/preflight race tests passed for 10 repetitions
- go vet passed
- git diff --check passed
Please add allocation-rejection regressions for both cases described inline.
aunjgr
left a comment
There was a problem hiding this comment.
Re-review of exact head 2d52c8d8e28111be52755107c9e51f8100e6dbca against merge base ae12701a888fac102e9641cc7cbb8706527366d8.
[P1] Keep aggregate source reservations alive through the complete batch publication
applyStringSourceEvents now allocates the required destination sidecar before the group hash commit, but the reservation does not survive the runtime mutation sequence. Every SetStringSourceAtWithMP call immediately invokes normalizeStringSources (pkg/container/vector/vector.go:783-809), which releases the sidecar whenever the intermediate rows are uniform. The aggregate paths never call FinalizeStringSourcePreflight; that deferred-normalization lifetime currently exists only for group-key publication.
A concrete reverse-order case still allocates after commit: start with two NULL ANY_VALUE state rows carrying uniform scalar source Literal, then fill group 1 from Literal and group 2 from COMStmt. Preflight sees the second mixed event and allocates [Literal, Literal]. After CommitPreview, publishing group 1 writes Literal; normalization observes a uniform sidecar and frees it. Publishing group 2 then calls preflightStringSourceAt and must allocate the sidecar again. If the allocation controller rejects at that point, BatchFill returns after the hash groups have already been published, so spill/retry is no longer safe. The same ordering is reachable in fill/merge for MIN/MAX and MAX_BY.
The new direct test uses COMStmt before Literal, which keeps the sidecar mixed after the first write and therefore misses this interleaving. Please give aggregate preflight a batch-scoped deferred-normalization lifetime (including error/cancel finalization) so setters cannot discard admitted capacity until all correlated rows are published. Add fill and merge controls with the same-source row first and mixed-source row last, plus a Group/MergeGroup allocation controller that rejects any source allocation after commit.
The winner/equal-event calculations themselves match the runtime aggregate policies; I found no additional confirmed blocker in this delta.
aptend
left a comment
There was a problem hiding this comment.
Blocking aggregate publication still reallocates after the outer preflight for a valid source ordering; details are inline.
aptend
left a comment
There was a problem hiding this comment.
Re-reviewed the complete change and all prior review history/threads at exact head 2ccb07b, with focused comparison from the previously reviewed head 2cd1569. The retained-preflight lifetime now covers reverse-order aggregate publication and current/standby group-key publication, closing the prior post-commit reallocation counterexamples. Verified affected package tests, 100x/50x focused stress runs, 10x race runs, go vet, and diff check. The Ubuntu UT failure is TestCloneCommitFailureRollbackKeepsSourceFiles failing amid service connection resets/HAKeeper and logservice timeouts, outside this diff. No blocking findings.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of exact head d3d6a6d96651e54f0367a82dc538e70cafdd2ac7 against merge base c3974615c9419d2577aa37b2bfcd201b1515fff3.
[P1 performance] Restore the provenance-free bulk-copy fast path
summarizeStringSourceBatch and summarizeStringSourceSelection now scan every selected row unconditionally (pkg/container/vector/vector.go:2128-2158), even when the source has no StringSource metadata. Publication then scans the same rows again in propagatePrepareParamKindsBatch / propagatePrepareParamKindsAll (pkg/container/vector/vector.go:2530-2586). Consequently, an ordinary fixed-width UnionBatch that used to be one bulk copy now pays two Go-level row loops for every vector in normal source-free execution.
This is a measured material regression using the repository existing BenchmarkUnionBatchNoMetadata, with the same Apple M4 host, CGo wrapper, native artifacts, -count=7, and -benchtime=500ms:
- base
c3974615c9: median about 110 ns/op (107.0-112.4), 0 allocs - head
d3d6a6d966: median about 3206 ns/op (3203-3212), 0 allocs
That is roughly a 29x slowdown in a foundational vector-copy hot path, and it affects the overwhelmingly common case where both vectors are ordinary Expression-owned values. It also contradicts the PR goal that the source-free path remain fast.
Please short-circuit provenance work when neither side has StringSource metadata, handle uniform scalar sources in O(1), and reserve row scans for actual mixed sidecars or explicit per-row overrides. Batch-level preflight should also avoid causing a second identical scan inside the vector union. Keep this existing benchmark as the regression gate and add controls for uniform metadata and mixed sidecars.
I also rechecked the full provenance ownership/merge model, prior publication-order findings, latest-main integration, decoder rollback, const-NULL cache reuse, and LAG/LEAD NULL defaults. The new focused correctness tests pass; this measured hot-path regression is the remaining confirmed blocker.
aunjgr
left a comment
There was a problem hiding this comment.
Reviewed exact head d3d6a6d96651e54f0367a82dc538e70cafdd2ac7 against exact base c3974615c9419d2577aa37b2bfcd201b1515fff3. One measured merge-blocking hot-path regression remains.
The metadata-free UnionBatch path now scans all source rows in summarizeStringSourceBatch, then scans them again in propagatePrepareParamKindsBatch to append the default Expression source. This applies to ordinary vectors where neither side has any StringSource or prepare-parameter metadata—the overwhelmingly common path that was previously a bulk copy.
I reproduced BenchmarkUnionBatchNoMetadata for 1,024 INT64 rows on the same Ryzen 7950X3D host, five 300 ms samples per exact revision:
- base: 80.49–83.54 ns/op
- head: 4.551–4.841 µs/op
- both: 0 B/op, 0 allocs/op
That is about a 58x median slowdown in a foundational vector operation. Preserve the source-free bulk-copy fast path with an O(1) metadata-presence guard, handle uniform scalar metadata without per-row scans, and scan only mixed sidecars or explicit row overrides. Keep this benchmark as the regression gate and add uniform/mixed controls so the optimization does not weaken provenance correctness.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of exact head df0a2ce.
The prior P1 hot-path blocker is materially addressed in bb66117: metadata-free Union/UnionBatch now has an O(1) guard, uniform sources are summarized and propagated without row scans, mixed sidecars remain row-exact, and the zero-selection case is covered. I also rechecked that the previously approved preflight-retention and publication-order invariants were not weakened by this delta. Latest CI is green. Local benchmark execution was unavailable because this worktree does not contain cgo/libmo.dylib, so I treated the same-host benchmark ranges in the PR as supporting evidence rather than reproducing them.
[P1 design gate] Add and obtain approval for the mandatory architecture/design record before implementation approval.
Change scope: the PR adds an independent row-level provenance axis across vectors, expressions, planner rewrites, group/spill/aggregates, process state, remote dispatch, protobufs, and MORPC v30. It changes more than eight production owners, a wire compatibility contract, mixed-version behavior, allocation/publication lifetimes, and foundational vector-copy hot paths. This independently triggers the repository design-first gate by package count, cross-subsystem ownership, wire protocol, lifecycle, compatibility, and performance criteria.
CLAUDE_STRING_SOURCE_BOUNDARY_INVENTORY.md is useful as an implementation boundary checklist, but it is not an approved design artifact: it does not identify the owning issue and implementation PR/revision, has no traceable design approval phase, and does not close problem evidence/success criteria, status quo plus credible alternatives, rollout/fallback/observability, or an explicit capacity/performance budget. The PR body and implementation review rounds cannot substitute for the required reviewed design.
Please place the design in docs/rfcs/, docs/design/, or another stable versioned location, link issue #27215 and this implementation PR, close the required decisions above (especially MORPC v29/v30 mixed-version downgrade and rollback, source-sidecar ownership/bounds, and source-free hot-path acceptance thresholds), and obtain a distinct design approval. Then the implementation can be approved against that exact revision.
Change scope: whole provenance feature/refactor
Trigger: package count plus cross-subsystem, wire, lifecycle, compatibility, and hot-path complexity triggers
Design: missing approved artifact; current boundary inventory is unapproved/incomplete
Decision: REQUEST_CHANGES
XuPeng-SH
left a comment
There was a problem hiding this comment.
Design review PASS for exact revision 148b1ad.
Change scope: the complete row-level StringSource provenance feature across vector, expression/planner, group/aggregate/spill, process/remote transport, protobuf, and MORPC.
Trigger: package-count plus cross-subsystem ownership, wire compatibility, publication lifecycle, capacity, and hot-path complexity triggers.
Design: docs/design/CLAUDE_STRING_SOURCE_PROVENANCE_DESIGN.md.
Blocking findings: none.
Decision log: accepted the independent provenance axis; scalar fast path with on-demand one-byte-per-row mixed sidecar; selected-value and conservative merge rules; pre-publication reservation with retained normalization lifetime; v29 downgrade/v30 capability gate; no catalog migration; source-free 0-allocation and <=2x benchmark gate.
Decision: PASS.
Implementation review remains a separate phase and is evaluated against this exact design revision.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Implementation review PASS against the separately approved design revision 148b1ad.
The previously reviewed implementation closes the row-source propagation, aggregate/group pre-publication reservation, retained-normalization lifetime, decoder cleanup, mixed-version downgrade, and source-free fast-path blockers. The new commit is design-only and introduces no implementation deviation.
I independently reran BenchmarkUnionBatchNoMetadata on Apple M4 with five 500 ms samples: 118.4-123.5 ns/op, 0 B/op, 0 allocs/op. This is close to the previously measured pre-feature baseline and comfortably satisfies the approved <=2x threshold. No remaining blocking finding.
# Conflicts: # pkg/defines/const.go # pkg/frontend/computation_wrapper.go # pkg/frontend/computation_wrapper_test.go
What type of PR is this?
Which issue(s) this PR fixes:
issue #27215
What this PR does / why we need it:
mo/mainwhile retaining main's prepared-runtime-specialization revert; frontend differences add only SQLPrepare/COMStmt StringSource ownershipTesting
[Literal, UserVariable, Literal]group allocation rejection before hash publication and no-allocation-after-commit controlsgo list,go build,go vet, owning-package unit tests, BVT verification,gofmt, andgit diff --checkReview round 8
mo/mainatba8954d543; resolvedplan.proto/pipeline.protoadditively and regenerated both.pb.gofiles withmake generate-pbhashmap.UnitLimit-bounded operator-owned preview scratch and selected-row source overrides[Literal, Expression], including a shared GROUP BY / ANY_VALUE columnReview round 9
hashmap.UnitLimit-bounded touched destination vectors; success and every error path finalize through a pre-commit-installed defera/Literalthena/Expression + b/Literalin internal commit and full operator paths; an allocation controller rejects only afterHash.GroupCount()==2and observes no allocationmo/mainat53f6aa6f0bReview round 10
errors.IsthroughUnwrap; post-publication failures remain non-retryablecancelGroupByPreflightsand the existing spill/retry loop in Group, MergeGroup, and spill reloada/Literalthena/Expression, one rejection at resident group count 1, successful spill/retry, final source Expression and count 2mo/mainat6a8e7ecc53Conflict resolution round 11
mo/mainat22b91fe986pipeline.pb.go/plan.pb.gowithmake generate-pb; repeated generation is cleanReview round 12
PreExtendnow admits mixedstringSourcesalongside every other row-parallel sidecar; successful pre-extension makes laterSetLengthallocation-freeT_anyNULL partition materializer preserves selected SQLPrepare/COMStmt ownership for constant and selected NULLsmo/mainatae12701a88; BVT remains 33/33Review round 13
Review round 14
mo/mainate866c535f5; build, vet, UT, SCA, make, and string-source BVT (33/33) passReview round 15
mo/main(e866c535f5)CI/conflict update
mo/mainat8dd1efc201TestCloneCommitFailureRollbackKeepsSourceFilesduring logservice/Hakeeper connection reset with concurrent Dragonboat heartbeat/propose timeouts; the exact test passes locally after the merge (21.77s)Review follow-up: publication and NULL ownership
UnionOne,UnionMulti,Union,UnionBatch, andGetUnionAllFunction) and finalized them on every exitColumnExpressionExecutorcache creation and reusemo/mainatc3974615c9; owning/dependent build, vet, UT, SCA, make, and StringSource BVT (33/33) passHot-path review follow-up
BenchmarkUnionBatchNoMetadataimproved from the pre-fix 5.700–5.821 µs/op to a stable 159.0–187.5 ns/op range (0 B/op, 0 allocs/op); uniform scalar measured 164.6–221.3 ns/opmo/mainat5bc051abc4; owning/dependent UT, build, vet, SCA, make, and StringSource BVT (33/33) passDesign-gate phase
A stable architecture record is now available at
docs/design/CLAUDE_STRING_SOURCE_PROVENANCE_DESIGN.md, revision148b1ad60e.It links issue #27215 and this implementation PR and defines:
The document is intentionally marked Proposed. Implementation approval remains blocked until a maintainer provides a distinct, traceable design approval for exact revision
148b1ad60e; no approval is being inferred or self-recorded here.Latest-main conflict resolution
mo/mainat9a289d96d5The current proposed design revision is now
72886d1618; distinct maintainer design approval remains pending for this exact revision.