zeroshade commented on code in PR #921:
URL: https://github.com/apache/arrow-go/pull/921#discussion_r3566723274
##########
parquet/pqarrow/file_writer.go:
##########
@@ -185,25 +185,39 @@ func NewFileWriter(arrschema *arrow.Schema, w io.Writer,
props *parquet.WriterPr
// 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 {
Review 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.
##########
parquet/pqarrow/file_writer.go:
##########
@@ -185,25 +185,39 @@ func NewFileWriter(arrschema *arrow.Schema, w io.Writer,
props *parquet.WriterPr
// 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 {
if fw.rgw != nil {
- fw.rgw.Close()
+ if err := fw.rgw.Close(); err != nil {
+ return err
+ }
+ }
+ rgw, err := fw.wr.AppendRowGroupChecked()
+ if err != nil {
+ return err
}
- fw.rgw = fw.wr.AppendRowGroup()
+ fw.rgw = rgw
fw.colIdx = 0
+ return nil
}
// NewBufferedRowGroup starts a new memory Buffered Row Group to allow writing
columns / records
// without immediately flushing them to disk. This allows using WriteBuffered
to write records
// and decide where to break your row group based on the TotalBytesWritten
rather than on the max
// 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 {
Review 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.
--
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]
