fix(parquet): propagate row-group and page-index write errors#921
Conversation
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
The core change — persisting writeErr on file.Writer and propagating row-group/page-index close/write errors — is valuable, and the file.Writer side is done well (existing signatures preserved, ...Checked variants added). Two blockers inline. Also worth a changelog note: the deprecated AppendRowGroup()/AppendBufferedRowGroup() now panic on errors that were previously swallowed. The failure-injection tests look good.
| // NewRowGroup does what it says on the tin, creates a new row group in the underlying file. | ||
| // Equivalent to `AppendRowGroup` on a file.Writer | ||
| func (fw *FileWriter) NewRowGroup() { | ||
| func (fw *FileWriter) NewRowGroup() error { |
There was a problem hiding this comment.
Blocking (exported API break): changing NewRowGroup() to NewRowGroup() error alters a public method signature and breaks downstream method-values / interface satisfaction. Please mirror the approach you used in file.Writer: keep NewRowGroup() (panic on error) and add NewRowGroupChecked() error, with the internal write paths calling the checked variant.
| // row group len. If using Records, this should be paired with WriteBuffered, while | ||
| // Write will always write a new record as a row group in and of itself. | ||
| func (fw *FileWriter) NewBufferedRowGroup() { | ||
| func (fw *FileWriter) NewBufferedRowGroup() error { |
There was a problem hiding this comment.
Same exported-API concern — keep NewBufferedRowGroup() and add NewBufferedRowGroupChecked() error. Note the existing callers at encode_arrow_test.go:231,330,385,1374,1399 invoke these as bare statements and now silently drop the returned error; with the compatibility shape above they can remain unchanged.
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Good direction on propagating the writer errors. Two gaps remain before this is complete — see inline.
| @@ -251,7 +256,7 @@ func (rg *rowGroupWriter) Close() error { | |||
| rg.metadata.SetNumRows(rg.nrows) | |||
| rg.metadata.Finish(rg.bytesWritten, rg.ordinal) | |||
There was a problem hiding this comment.
rg.metadata.Finish(...) still discards its returned error. If it fails (for example, closing a serial row group before all schema columns have been initialized), Close() will report success and the file writer may continue with incomplete row-group metadata. Please capture it into rg.closeErr and return it, consistent with the other close failures handled just above.
| if fw.pageIndexBuilder != nil { | ||
| // serialize page index after all row groups have been written and report | ||
| // location to the file metadata | ||
| fw.pageIndexBuilder.Finish() |
There was a problem hiding this comment.
writePageIndex permanently Finish()es the PageIndexBuilder, but this path is reachable from FlushWithFooter, which is documented to leave the writer open for additional row groups. With page indexes enabled, a subsequent AppendRowGroupChecked/AppendBufferedRowGroupChecked will then call AppendRowGroup on a finished builder and record a sticky error. Please keep page-index state appendable after an intermediate footer, or explicitly document/enforce that cumulative footers are unsupported when page indexes are enabled.
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
Row-group transitions and footer flushes used to discard close and page-index errors, which could leave a truncated file that still appeared to write successfully.
This change adds checked row-group APIs, preserves legacy panic behavior for the no-error APIs, persists writer failures so a later footer cannot be written, returns Arrow Parquet row-group errors, and checks buffered metadata serialization and short writes.
Tests: