fix(parquet): make column reader cleanup idempotent#924
Conversation
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
LGTM in substance — detaching curPage/rdr before cleanup makes Close idempotent (no double-release/close), and making setPageReader return an error so replacement failures propagate through SeekToRow/nextRowGroup is the right call. Good targeted tests. Two nits inline.
| return nil | ||
| } | ||
|
|
||
| // Close closes the page raeder and the page if set. |
| func (c *columnChunkReader) setPageReader(rdr PageReader) { | ||
| c.Close() | ||
|
|
||
| func (c *columnChunkReader) setPageReader(rdr PageReader) error { |
There was a problem hiding this 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.
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
The idempotent-cleanup change looks right. One resource leak on the error path to address — inline.
| if !ok { | ||
| return errors.New("record reader does not support error-returning page reader replacement") | ||
| } | ||
| return rr.SetPageReaderWithError(pr) |
There was a problem hiding this 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).
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Thanks — all prior review feedback is addressed. LGTM.
2 participants
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: