address comments

Author: wecharyu

cpp/src/parquet/arrow/writer.cc

@@ -404,8 +404,8 @@ class FileWriterImpl : public FileWriter {
     }
     // max_row_group_bytes is applied only after the row group has accumulated data.
     if (row_group_writer_ != nullptr && row_group_writer_->num_rows() > 0) {
-      double avg_row_size = row_group_writer_->current_buffered_bytes() * 1.0 /
-                            row_group_writer_->num_rows();
+      double avg_row_size =
+          row_group_writer_->total_buffered_bytes() * 1.0 / row_group_writer_->num_rows();
       chunk_size = std::min(
           chunk_size,
           static_cast<int64_t>(this->properties().max_row_group_bytes() / avg_row_size));
@@ -496,11 +496,11 @@ class FileWriterImpl : public FileWriter {
       int64_t batch_size =
           std::min(max_row_group_length - group_rows, batch.num_rows() - offset);
       if (group_rows > 0) {
-        int64_t buffered_bytes = row_group_writer_->current_buffered_bytes();
-        double avg_row_bytes = buffered_bytes * 1.0 / group_rows;
+        int64_t buffered_bytes = row_group_writer_->total_buffered_bytes();
+        double avg_row_size = buffered_bytes * 1.0 / group_rows;
         batch_size = std::min(
             batch_size,
-            static_cast<int64_t>((max_row_group_bytes - buffered_bytes) / avg_row_bytes));
+            static_cast<int64_t>((max_row_group_bytes - buffered_bytes) / avg_row_size));
       }
       if (batch_size > 0) {
         RETURN_NOT_OK(WriteBatch(offset, batch_size));

cpp/src/parquet/file_writer.cc

@@ -68,10 +68,10 @@ int64_t RowGroupWriter::total_compressed_bytes_written() const {
   return contents_->total_compressed_bytes_written();
 }
 
-int64_t RowGroupWriter::current_buffered_bytes() const {
+int64_t RowGroupWriter::total_buffered_bytes() const {
   return contents_->total_compressed_bytes() +
          contents_->total_compressed_bytes_written() +
-         contents_->estimated_buffered_value_bytes();
+         contents_->EstimatedBufferedValueBytes();
 }
 
 bool RowGroupWriter::buffered() const { return contents_->buffered(); }
@@ -201,7 +201,7 @@ class RowGroupSerializer : public RowGroupWriter::Contents {
     return total_compressed_bytes_written;
   }
 
-  int64_t estimated_buffered_value_bytes() const override {
+  int64_t EstimatedBufferedValueBytes() const override {
     if (closed_) {
       return 0;
     }

cpp/src/parquet/file_writer.h

@@ -59,7 +59,7 @@ class PARQUET_EXPORT RowGroupWriter {
     /// \brief total compressed bytes written by the page writer
     virtual int64_t total_compressed_bytes_written() const = 0;
     /// \brief estimated size of the values that are not written to a page yet
-    virtual int64_t estimated_buffered_value_bytes() const = 0;
+    virtual int64_t EstimatedBufferedValueBytes() const = 0;
 
     virtual bool buffered() const = 0;
   };
@@ -103,7 +103,7 @@ class PARQUET_EXPORT RowGroupWriter {
   int64_t total_compressed_bytes_written() const;
   /// \brief including compressed bytes in page writer and uncompressed data
   /// value buffer.
-  int64_t current_buffered_bytes() const;
+  int64_t total_buffered_bytes() const;
 
   /// Returns whether the current RowGroupWriter is in the buffered mode and is created
   /// by calling ParquetFileWriter::AppendBufferedRowGroup.

cpp/src/parquet/properties.h

@@ -26,6 +26,7 @@
 #include "arrow/io/caching.h"
 #include "arrow/type_fwd.h"
 #include "arrow/util/compression.h"
+#include "arrow/util/logging.h"
 #include "arrow/util/type_fwd.h"
 #include "parquet/encryption/encryption.h"
 #include "parquet/exception.h"
@@ -417,13 +418,16 @@ class PARQUET_EXPORT WriterProperties {
     /// Specify the max number of rows to put in a single row group.
     /// Default 1Mi rows.
     Builder* max_row_group_length(int64_t max_row_group_length) {
+      ARROW_CHECK_GT(max_row_group_length, 0) << "max_row_group_length must be positive";
       max_row_group_length_ = max_row_group_length;
       return this;
     }
 
     /// Specify the max number of bytes to put in a single row group.
+    /// The size is estimated based on encoded and compressed data.
     /// Default 128MB.
     Builder* max_row_group_bytes(int64_t max_row_group_bytes) {
+      ARROW_CHECK_GT(max_row_group_bytes, 0) << "max_row_group_bytes must be positive";
       max_row_group_bytes_ = max_row_group_bytes;
       return this;
     }