elsteveogrande opened a new pull request, #1435:
URL: https://github.com/apache/arrow-go/pull/1435
### Rationale for this change
Two error paths while building a field reader leave readers unreleased:
- In `getReader`, when one child of a struct fails to build, the readers
already built for its siblings are dropped without `Release`. With
`errgroup.WithContext` and `SetLimit(1)`, the children after the failing one
are still built too, since `getReader` does not check the canceled context.
Their page readers' buffers leak: 32 KiB for a 3-child struct of `int32`s in
the new test.
- `newLeafReader` returns a live `*ColumnReader` together with its
`nextRowGroup` error. Callers check the error and drop the reader, so nobody
releases it.
`GetFieldReaders` already releases its partial readers on error; this
applies the same cleanup inside `getReader`.
### What changes are included in this PR?
- `getReader` releases the struct's already-built child readers before
returning a child's error.
- `newLeafReader` releases its reader and returns `nil` when `nextRowGroup`
fails.
- Tests:
- `TestGetFieldReaderReleasesStructChildrenOnError` fails a struct's
second child and checks that no allocations remain, with `Parallel` false and
true. On `main` it reports two 16 KiB leaks from `serializedPageReader.init`.
- `TestGetFieldReaderReturnsNilReaderOnError` checks that a failing leaf
returns a nil reader. On `main` it gets a `*leafReader`.
Both reuse `failingReaderAt` from
`TestGetFieldReadersReleasesPartialReadersOnError`.
This overlaps #1434, which adds a serial loop for struct children in the
same function. Whichever lands second should release the built children in that
loop too.
### Are these changes tested?
Yes:
- `PARQUET_TEST_DATA=<parquet-testing-data> go test ./parquet/...`
- `go test -race ./parquet/pqarrow -run
'^(TestGetFieldReader|TestGetFieldReaders|TestRecordReader|TestPartialStructColumnRead|TestStructReaderSerial)'
-count=1`
- `go vet ./parquet/pqarrow`, `git diff --check`
### Are there any user-facing changes?
`GetFieldReader` now returns a nil reader with its error when a leaf column
fails to open, where it used to return an unreleased reader. Struct reads that
fail no longer leak memory from a non-Go allocator.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
