fix(container): bound external null bitmap merge - #27444
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? |
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
[P1] Do not clip allocation-accounted function-result NULLs when the bitmap's logical length is reset. At f7197fe, FunctionExpressionExecutor preallocates external NULL storage, then PreExtendAndReset calls ResetWithSameType, leaving its bitmap length zero while capacity still covers the result rows. Built-ins such as lower propagate source NULLs through nulls.Or; the new external-result branch sees source length greater than zero, sets its limit to zero, and returns without copying any NULLs.\n\nReproduced with an allocation-accounted expression executor over lower(varchar): input values AA, NULL, CC return a non-NULL value at row 1 on this head. The identical isolated test passes at merge base 344d852.\n\nPreserve the normal OR path whenever the external destination capacity can represent the source; only use a bounded copy when growth would exceed that capacity, or pass the output's visible row bound explicitly. Add an allocation-accounted function-expression regression alongside the bitmap tests.
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
Deep re-review of exact head 74489d3df71b63a42f9b39b92db1739cd0273b97. I read the full prior review/history, audited the complete four-file diff, and focused on the increment after the changes-requested head f7197fe8c539126e9258820420d5c75f182f0809. The allocation-accounted expression NULL-loss case is fixed, and the hash-join empty-bucket caller now establishes its row bound. However, the generic nulls.Or fallback now uses physical capacity as a logical row bound and reintroduces the PR's original out-of-range NULL leak whenever the longer source still fits spare external capacity; the deterministic counterexample is inline. Unmodified nulls/bitmap/colexec/hashjoin suites, focused race runs, vet, and diff checks pass; the added exact-head counterexample fails as described and was removed afterward.
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
Deep re-reviewed exact head a3f6dcf63913be198a60d4a940912720b1820d56 against merge base dc403fbac6a09c77ac93bb023fe83056308a038b and the full prior review history.
The result owner now publishes the current visible row domain after reset; external physical capacity is never inferred as logical length. All Nulls union entry points route through the same word-wise bounded merge, preserve in-range NULLs, exclude stale out-of-range bits, handle aliases/empty/zero-row generations, and retain destination length. External expansion failure is failure-atomic, and the hash-join empty-bucket path establishes its result bound before propagation.
This closes the original panic, the spare-capacity leak, and reset-result NULL loss without adding per-row work or allocations. No remaining correctness, ownership, or boundedness blocker found.
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
Re-reviewed exact head a3f6dcf after reading the full review/thread history and checking both the rewritten delta and complete diff. The prior blocker is closed: external NULL unions are bounded by the destination logical row domain rather than spare backing capacity; allocation-accounted function results republish that domain after reset; the empty-spill MARK join establishes it before propagation; and capacity failure is now state-atomic. Exact-head list/build/vet/full package tests passed, focused race tests passed 20 iterations, and an allocation-accounted MARK-join counterexample passed 20 iterations. No blocking issues found.
|
@Mergifyio queue |
Merge Queue Status
This pull request spent 1 hour 26 minutes 56 seconds in the queue, including 50 minutes 39 seconds running CI. Required conditions to merge
|
5 participants
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #27432
What this PR does / why we need it:
nulls.Orcould panic when a source bitmap retained a longer logical length than an externally allocated result bitmap. This occurred while evaluating a nullableVARCHARcomparison inside aCASEaggregate over a large Kafka-loaded table, producing:This change keeps externally backed result bitmap storage bounded to its visible logical length and copies only source NULL bits within that bound. It adds regression coverage for one and two long source bitmaps, including in-range and out-of-range bits.
Tests completed locally:
go test -mod=readonly -vet=off -count=1 ./pkg/container/nulls ./pkg/common/bitmapgo test -mod=readonly -vet=off -short -race -tags matrixone_test -p 1 ./pkg/container/nulls ./pkg/common/bitmapgo vet -mod=readonly ./pkg/common/bitmap ./pkg/container/nulls