zeroshade commented on code in PR #921:
URL: https://github.com/apache/arrow-go/pull/921#discussion_r3572941297
##########
parquet/file/row_group_writer.go:
##########
@@ -251,7 +256,7 @@ func (rg *rowGroupWriter) Close() error {
rg.metadata.SetNumRows(rg.nrows)
rg.metadata.Finish(rg.bytesWritten, rg.ordinal)
Review 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.
##########
parquet/file/file_writer.go:
##########
@@ -244,22 +298,35 @@ func (fw *Writer) startFile() error {
return nil
}
-func (fw *Writer) writePageIndex() {
+func (fw *Writer) writePageIndex() error {
if fw.pageIndexBuilder != nil {
// serialize page index after all row groups have been written
and report
// location to the file metadata
fw.pageIndexBuilder.Finish()
Review 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.
--
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]
