GH: [Go][Parquet]Handle errors to prevent panic - #3
Merged
Conversation
Signed-off-by: Cai Zhang <cai.zhang@zilliz.com>
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
xiaocai2333
changed the title
|
❌ GitHub issue apache#613 could not be retrieved. |
1 similar comment
|
❌ GitHub issue apache#613 could not be retrieved. |
xiaocai2333
changed the title
Fix three cases in doImportChildren where importChild errors were silently discarded for STRUCT, DENSE_UNION, and SPARSE_UNION types, causing nil pointer dereference (SIGSEGV) in downstream code when a child import fails. Also fix memory cleanup on import error: - Add forceRelease to importAllocator to prevent double-free - Release already-imported children when doImportChildren fails - Call forceRelease in doImportArr on error to avoid leaks - Remove per-import defer cleanup in doImport (now handled by doImportArr) Regression test for milvus-io/milvus#48375, milvus-io/milvus#48383. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Cai Zhang <cai.zhang@zilliz.com>
1 task
sre-ci-robot
pushed a commit
to milvus-io/milvus
that referenced
this pull request
Aug 10, 2026
issue: #51981 ## What `storage.MergeSort` is the default mix-compaction path (`dataNode.compaction.useMergeSort` defaults to `true`), reached via `mix_compactor.go` -> `merge_sort.go:135`. Three changes: 1. **Heap holds k entries, not every in-flight row.** `enqueueAll` pushed every qualifying row of the current record into the queue, so the heap held the in-flight row count rather than the reader count. Each input record is already sorted by the merge key, so the standard form applies: one entry per reader, pop one and push one. At 30 readers x 4096 rows/record the heap goes from ~246k entries to 30. 2. **Merge keys resolved once per record.** The comparator did `recs[x.ri].Column(fid).(*array.Int64).Value(x.i)` per side per comparison -- a map lookup plus a type assert whose result is invariant for a record. They are now extracted in `advanceRecord` (`Int64Values()` for int64, the `*array.String` pointer for varchar; both O(1) and copy-free). 3. **No per-row allocation.** `pq.Enqueue(&index{...})` escaped once per row, and `container/heap`'s `Push(x any)` boxes regardless. Replaced by a small typed value heap over the existing `rowIndex{ri, i int32}` (added in #50817). `storage.PriorityQueue` had no other users repo-wide and is removed, along with the `endPositions` bookkeeping. The sibling `Sort` in the same file already received (2) and (3) in #50817. ## Numbers `BenchmarkMergeSort` (added here), 200k rows over `generateTestSchema()` -- 20 fields including 5 vector types. Median of 3, `-benchtime 10x -cpu 1`: | | before | after | |---|---|---| | time | 523.50 ms | **325.96 ms (1.61x)** | | allocs | 302,617 | **102,599 (-66%)** | The gain depends on how much `rb.Append` dominates. `BenchmarkMergeSortVarcharKey`, 65k rows over a single varchar field: | | before | after | |---|---|---| | time | 60.83 ms | **9.18 ms (6.63x)** | | allocs | 65,642 | **91** | | B/op | 6.11 MB | 4.84 MB (-21%) | The varchar allocation count is the cleanest evidence: 65,642 is approximately the 65,536 rows, i.e. one allocation per row; after is 91 in total, identical across all three runs. ## One deliberate behavior change A k-way merge relies on each input record being sorted by the merge key. That reliance is not new, but the old path did not fail uniformly on disorder, so this is a narrowing and worth stating precisely. `endPositions` already depended on sortedness for the general case: an unsorted record could dequeue its smallest key first and trigger `advanceRecord` while stale entries from the now-invalid record remained queued (`RecordReader.Next` is borrow-scoped, `record_reader.go:20-21`), which panics with `index out of range` -- the `[3,1,2]` shape. But it did tolerate a narrower class. Because the old loop enqueued every eligible row of a record, disorder whose maximum key still lands on the last row (a single reader with key order `[2,1,3]`, so `endPositions[ri]` is the final index) never triggered the early `advanceRecord`: it was emitted correctly as 1,2,3 and `MergeSort` returned success. Those inputs now return an error instead. The narrowing is intended -- a bare k-way merge would emit them silently out of order, and trading a loud failure for silent corruption is the one outcome worth avoiding -- and `mix_compactor.go:473-484` gates merge-sort to already-sorted segments, so the affected class is vanishingly rare on real data. To avoid trading a loud failure for silent corruption, the merge now checks that the emitted key never decreases and returns a merr error otherwise. The previous key is held in reusable buffers rather than cloned per row -- cloning would reintroduce exactly the per-row allocation this PR removes. **On #48322 / #48449.** An earlier revision of this description named the gate as their root cause and called for a reachability analysis. Both parts have since been resolved, and neither the way that paragraph assumed. #48322 was an arrow-go use-after-free on STRUCT array deallocation, fixed by milvus-io/arrow#3 and #48775 and verified on `master-20260429`. #48449 carries the same title; its closure trail is inconclusive and does not point at the gate either. Neither issue supports the attribution. The reachability analysis has been done, and the scenario it described -- an `IsSorted=true` segment passing the gate "after `namespaceEnabled` is turned on" -- is **not reachable**. `EnableNamespace` is written only at collection creation (`ddl_callbacks_create_collection.go:217`); every schema-altering path rebuilds the snapshot through `coll.ToCollectionSchemaPB()`, which copies the stored value (`collection.go:163`), so the assignment at `collection.go:216` is an identity. And `sort_compaction.go:373-374` derives both sorted flags from that same immutable setting (`isSorted := !isNamespaceSorted`), so a segment's flag and the plan's merge key cannot disagree today. The gate is nonetheless loose in *structure* -- it tests whether a segment was sorted, not by which key -- so it is safe by coincidence rather than by construction. This PR now tightens it. ## Gate tightened `canMergeSort` (extracted from `Compact()` so it is unit-testable) now requires the sorted flag that corresponds to *this plan's* merge key, rather than either flag. A mismatch falls through to the existing `mergeSplit` path before any segment rows are read; no new fallback code. Two reviewers asked for this. It is defense-in-depth, not a fix for an observed production case -- as established above, the metadata-level mismatch is not reachable today. The failure that *is* left loud -- a segment whose flag matches but whose bytes do not -- is now diagnosable. `MergeSort` reports `ErrDataIntegrity` (`ErrStorage`'s own doc directs callers here; both are non-retriable `SystemError`, so retry semantics and classification are unchanged) with a `reader N record M row K` coordinate, and `mergeSortMultipleSegments` logs the reader-index to segment-ID mapping alongside it, so the reader index resolves to a segment. **Not done:** degrading to `mergeSplit` when that error fires. Rationale in the review thread -- briefly, the prior behavior for general disorder was a panic, so a deterministic error is not a regression against it; and a metadata/data inconsistency is a should-not-happen class where silent degradation would hide the problem. ## Test coverage `MergeSort` had no varchar-key test and no multi-field-key test, and `TestMergeSort`'s ordering assertion only checked `Value(0)` of each output record while the 64MB `batchSize` yielded a single batch. The first commit adds those baselines and shows them green on the pre-rewrite implementation; the second keeps them green. - `TestMergeSort` -- every row checked, small batchSize to force multiple batches - `TestMergeSortVarcharKey`, `TestMergeSortByMoreThanOneField` -- previously uncovered paths - `TestMergeSortPredicateCalledOncePerRow` -- pins predicate arity, whose production implementation carries a side effect (`segmentTotalRows`) - `TestMergeSortUnsortedInputReturnsError` -- the ordering check - `TestMergeSortUnsortedInputReportsLaterRecord`, `TestMergeSortUnsortedInputReportsOffendingReader` -- pin the reported coordinate: the first that the record number is computed rather than always 0, the second that the reader index is too - `TestCanMergeSort`, `TestCanMergeSortMatchesMergeKey` -- the gate, both halves of the merge-key truth table plus the segment-count boundary - `TestRowHeap`, `TestRowHeapSingleElement` ## Verification - `./internal/storage/ -run 'TestMergeSort|TestRowHeap|TestSort|TestRadixSort'` -- pass - `./internal/datanode/compactor/...` -- pass - `pkg/util/merr` guard tests -- pass (an error code changed) - `go build -tags dynamic` on both packages -- pass - `make static-check` -- **not verified locally**; the local golangci-lint binary is incompatible with the current Go toolchain (`unsupported version: 2`). `go vet` on both packages reports only pre-existing findings in files this PR does not touch. Relying on CI for this one. Two pre-existing failures in the full `./internal/storage/` package (`TestAzureObjectStorage` timeout, `TestReadFile` nil deref) reproduce on unmodified master and are unrelated to this change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: bigsheeper <yihao.dai@zilliz.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
sunby
pushed a commit
to sunby/milvus
that referenced
this pull request
Aug 20, 2026
…#51998) issue: milvus-io#51981 ## What `storage.MergeSort` is the default mix-compaction path (`dataNode.compaction.useMergeSort` defaults to `true`), reached via `mix_compactor.go` -> `merge_sort.go:135`. Three changes: 1. **Heap holds k entries, not every in-flight row.** `enqueueAll` pushed every qualifying row of the current record into the queue, so the heap held the in-flight row count rather than the reader count. Each input record is already sorted by the merge key, so the standard form applies: one entry per reader, pop one and push one. At 30 readers x 4096 rows/record the heap goes from ~246k entries to 30. 2. **Merge keys resolved once per record.** The comparator did `recs[x.ri].Column(fid).(*array.Int64).Value(x.i)` per side per comparison -- a map lookup plus a type assert whose result is invariant for a record. They are now extracted in `advanceRecord` (`Int64Values()` for int64, the `*array.String` pointer for varchar; both O(1) and copy-free). 3. **No per-row allocation.** `pq.Enqueue(&index{...})` escaped once per row, and `container/heap`'s `Push(x any)` boxes regardless. Replaced by a small typed value heap over the existing `rowIndex{ri, i int32}` (added in milvus-io#50817). `storage.PriorityQueue` had no other users repo-wide and is removed, along with the `endPositions` bookkeeping. The sibling `Sort` in the same file already received (2) and (3) in milvus-io#50817. ## Numbers `BenchmarkMergeSort` (added here), 200k rows over `generateTestSchema()` -- 20 fields including 5 vector types. Median of 3, `-benchtime 10x -cpu 1`: | | before | after | |---|---|---| | time | 523.50 ms | **325.96 ms (1.61x)** | | allocs | 302,617 | **102,599 (-66%)** | The gain depends on how much `rb.Append` dominates. `BenchmarkMergeSortVarcharKey`, 65k rows over a single varchar field: | | before | after | |---|---|---| | time | 60.83 ms | **9.18 ms (6.63x)** | | allocs | 65,642 | **91** | | B/op | 6.11 MB | 4.84 MB (-21%) | The varchar allocation count is the cleanest evidence: 65,642 is approximately the 65,536 rows, i.e. one allocation per row; after is 91 in total, identical across all three runs. ## One deliberate behavior change A k-way merge relies on each input record being sorted by the merge key. That reliance is not new, but the old path did not fail uniformly on disorder, so this is a narrowing and worth stating precisely. `endPositions` already depended on sortedness for the general case: an unsorted record could dequeue its smallest key first and trigger `advanceRecord` while stale entries from the now-invalid record remained queued (`RecordReader.Next` is borrow-scoped, `record_reader.go:20-21`), which panics with `index out of range` -- the `[3,1,2]` shape. But it did tolerate a narrower class. Because the old loop enqueued every eligible row of a record, disorder whose maximum key still lands on the last row (a single reader with key order `[2,1,3]`, so `endPositions[ri]` is the final index) never triggered the early `advanceRecord`: it was emitted correctly as 1,2,3 and `MergeSort` returned success. Those inputs now return an error instead. The narrowing is intended -- a bare k-way merge would emit them silently out of order, and trading a loud failure for silent corruption is the one outcome worth avoiding -- and `mix_compactor.go:473-484` gates merge-sort to already-sorted segments, so the affected class is vanishingly rare on real data. To avoid trading a loud failure for silent corruption, the merge now checks that the emitted key never decreases and returns a merr error otherwise. The previous key is held in reusable buffers rather than cloned per row -- cloning would reintroduce exactly the per-row allocation this PR removes. **On milvus-io#48322 / milvus-io#48449.** An earlier revision of this description named the gate as their root cause and called for a reachability analysis. Both parts have since been resolved, and neither the way that paragraph assumed. milvus-io#48322 was an arrow-go use-after-free on STRUCT array deallocation, fixed by milvus-io/arrow#3 and milvus-io#48775 and verified on `master-20260429`. milvus-io#48449 carries the same title; its closure trail is inconclusive and does not point at the gate either. Neither issue supports the attribution. The reachability analysis has been done, and the scenario it described -- an `IsSorted=true` segment passing the gate "after `namespaceEnabled` is turned on" -- is **not reachable**. `EnableNamespace` is written only at collection creation (`ddl_callbacks_create_collection.go:217`); every schema-altering path rebuilds the snapshot through `coll.ToCollectionSchemaPB()`, which copies the stored value (`collection.go:163`), so the assignment at `collection.go:216` is an identity. And `sort_compaction.go:373-374` derives both sorted flags from that same immutable setting (`isSorted := !isNamespaceSorted`), so a segment's flag and the plan's merge key cannot disagree today. The gate is nonetheless loose in *structure* -- it tests whether a segment was sorted, not by which key -- so it is safe by coincidence rather than by construction. This PR now tightens it. ## Gate tightened `canMergeSort` (extracted from `Compact()` so it is unit-testable) now requires the sorted flag that corresponds to *this plan's* merge key, rather than either flag. A mismatch falls through to the existing `mergeSplit` path before any segment rows are read; no new fallback code. Two reviewers asked for this. It is defense-in-depth, not a fix for an observed production case -- as established above, the metadata-level mismatch is not reachable today. The failure that *is* left loud -- a segment whose flag matches but whose bytes do not -- is now diagnosable. `MergeSort` reports `ErrDataIntegrity` (`ErrStorage`'s own doc directs callers here; both are non-retriable `SystemError`, so retry semantics and classification are unchanged) with a `reader N record M row K` coordinate, and `mergeSortMultipleSegments` logs the reader-index to segment-ID mapping alongside it, so the reader index resolves to a segment. **Not done:** degrading to `mergeSplit` when that error fires. Rationale in the review thread -- briefly, the prior behavior for general disorder was a panic, so a deterministic error is not a regression against it; and a metadata/data inconsistency is a should-not-happen class where silent degradation would hide the problem. ## Test coverage `MergeSort` had no varchar-key test and no multi-field-key test, and `TestMergeSort`'s ordering assertion only checked `Value(0)` of each output record while the 64MB `batchSize` yielded a single batch. The first commit adds those baselines and shows them green on the pre-rewrite implementation; the second keeps them green. - `TestMergeSort` -- every row checked, small batchSize to force multiple batches - `TestMergeSortVarcharKey`, `TestMergeSortByMoreThanOneField` -- previously uncovered paths - `TestMergeSortPredicateCalledOncePerRow` -- pins predicate arity, whose production implementation carries a side effect (`segmentTotalRows`) - `TestMergeSortUnsortedInputReturnsError` -- the ordering check - `TestMergeSortUnsortedInputReportsLaterRecord`, `TestMergeSortUnsortedInputReportsOffendingReader` -- pin the reported coordinate: the first that the record number is computed rather than always 0, the second that the reader index is too - `TestCanMergeSort`, `TestCanMergeSortMatchesMergeKey` -- the gate, both halves of the merge-key truth table plus the segment-count boundary - `TestRowHeap`, `TestRowHeapSingleElement` ## Verification - `./internal/storage/ -run 'TestMergeSort|TestRowHeap|TestSort|TestRadixSort'` -- pass - `./internal/datanode/compactor/...` -- pass - `pkg/util/merr` guard tests -- pass (an error code changed) - `go build -tags dynamic` on both packages -- pass - `make static-check` -- **not verified locally**; the local golangci-lint binary is incompatible with the current Go toolchain (`unsupported version: 2`). `go vet` on both packages reports only pre-existing findings in files this PR does not touch. Relying on CI for this one. Two pre-existing failures in the full `./internal/storage/` package (`TestAzureObjectStorage` timeout, `TestReadFile` nil deref) reproduce on unmodified master and are unrelated to this change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: bigsheeper <yihao.dai@zilliz.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Fixes: apache/arrow-go#613
What changes are included in this PR?
Return error correctly.
Are these changes tested?
Yes
Are there any user-facing changes?
No