You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/07/11 07:42:10 UTC
[PR] fix(parquet): make column reader cleanup idempotent [arrow-go]
fallintoplace opened a new pull request, #924:
URL: https://github.com/apache/arrow-go/pull/924
Column chunk cleanup retained both the current page and page reader after Close, so repeated cleanup could release or close the same resources twice. Page-reader replacement also discarded close errors and installed the replacement anyway.
This change detaches resources before cleanup, makes Close idempotent, returns and retains replacement cleanup errors, and uses the checked replacement path from the Arrow reader before continuing to seek or read.
Tests:
- go test ./parquet/file -run 'TestPrimitiveReaderSuite|TestColumnChunkReader'
- go test ./parquet/... -run '^$'
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): make column reader cleanup idempotent [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #924:
URL: https://github.com/apache/arrow-go/pull/924
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): make column reader cleanup idempotent [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #924:
URL: https://github.com/apache/arrow-go/pull/924#discussion_r3572941433
##########
parquet/pqarrow/column_readers.go:
##########
@@ -162,13 +162,23 @@ func (lr *leafReader) clearOut() (out *arrow.Chunked) {
func (lr *leafReader) Field() *arrow.Field { return lr.field }
+func (lr *leafReader) setPageReader(pr file.PageReader) error {
+ rr, ok := lr.recordRdr.(file.RecordReaderWithError)
+ if !ok {
+ return errors.New("record reader does not support error-returning page reader replacement")
+ }
+ return rr.SetPageReaderWithError(pr)
Review Comment:
If `SetPageReaderWithError(pr)` returns an error, the freshly acquired `pr` is neither installed nor closed. Both call sites (`SeekToRow` and `nextRowGroup`) obtain a new `PageReader` immediately before calling this helper, so a replacement failure leaks the reader and its buffers. Please close `pr` before returning the error here (or transfer ownership back and close it at each call site).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet): make column reader cleanup idempotent [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #924:
URL: https://github.com/apache/arrow-go/pull/924#discussion_r3566723419
##########
parquet/file/column_reader.go:
##########
@@ -251,21 +253,31 @@ func (c *columnChunkReader) Descriptor() *schema.Column { return c.descr }
func (c *columnChunkReader) consumeBufferedValues(n int64) { c.numDecoded += n }
func (c *columnChunkReader) numAvailValues() int64 { return c.numBuffered - c.numDecoded }
func (c *columnChunkReader) pager() PageReader { return c.rdr }
-func (c *columnChunkReader) setPageReader(rdr PageReader) {
- c.Close()
+
+func (c *columnChunkReader) setPageReader(rdr PageReader) error {
+ if err := c.Close(); err != nil {
+ c.err = err
+ return err
+ }
c.rdr, c.err = rdr, nil
c.decoders = make(map[format.Encoding]encoding.TypedDecoder)
c.dictState = dictNotRead
c.numBuffered, c.numDecoded = 0, 0
+ return nil
}
// Close closes the page raeder and the page if set.
Review Comment:
Typo: `raeder` -> `reader`.
##########
parquet/file/column_reader.go:
##########
@@ -251,21 +253,31 @@ func (c *columnChunkReader) Descriptor() *schema.Column { return c.descr }
func (c *columnChunkReader) consumeBufferedValues(n int64) { c.numDecoded += n }
func (c *columnChunkReader) numAvailValues() int64 { return c.numBuffered - c.numDecoded }
func (c *columnChunkReader) pager() PageReader { return c.rdr }
-func (c *columnChunkReader) setPageReader(rdr PageReader) {
- c.Close()
+
+func (c *columnChunkReader) setPageReader(rdr PageReader) error {
Review Comment:
Optional: add a pqarrow-level test that a replacement close-error surfaces via `LoadBatch`/`SeekToRow`; the propagation is currently only covered at the primitive level.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org