[C++][Parquet] `num_rows` method in `FileSerializer` returns wrong number.

[HuaHuaY]

Describe the bug, including details regarding any error messages, version, and platform.

nums_rows() method is inherited from ParquetFileWriter. It seems that the method is designed to count the number of rows of RowGroups which are closed.

/// Number of rows in the yet started RowGroups.
///
/// Changes on the addition of a new RowGroup.
int64_t num_rows() const;

But in the implementation of class FileSerializer : public ParquetFileWriter::Contents, the num_rows_ variable is only modified in Close method. It means that num_rows() method always returns 0 before Close is called.
And after Close of ParquetFileWriter is called we can no longer call nums_rows() method, because contents_ is reset in Close and we will get a null pointer exception if we call nums_rows().

So, in the current implementation of FileSerializer, num_rows() will only return 0 or throw exception.

int ParquetFileWriter::num_columns() const { return contents_->num_columns(); }

void ParquetFileWriter::Close() {
  if (contents_) {
    contents_->Close();
    file_metadata_ = contents_->metadata();
    contents_.reset();
  }
}


class FileSerializer : public ParquetFileWriter::Contents {
  int64_t num_rows() const override { return num_rows_; }

  void Close() override {
    if (is_open_) {
      if (row_group_writer_) {
        num_rows_ += row_group_writer_->num_rows();    // <- num_rows is only modified here and can never be read.
        ...
      }
    ...
    }
  }

  RowGroupWriter* AppendRowGroup(bool buffered_row_group) {
    if (row_group_writer_) {
      // <- A possible missing `num_rows_ += row_group_writer_->num_rows()` here.
      row_group_writer_->Close();
    }
    ...
  }
};

Should we mark the virtual method in ParquetFileWriter deprecated and remove the method in next release? I guess the method which is used to count the row number during writing row groups is useless and our default implementation always returns 0 now. Otherwise we can fix the issue by following the comments above.

Component(s)

C++

[HuaHuaY]

@pitrou @wgtmac @mapleFU What do you think of this issue?

[mapleFU]

I don't know. This might because CheckRowsWritten() would adjust this by current writer...

[HuaHuaY]

I don't know. This might because CheckRowsWritten() would adjust this by current writer...

CheckRowsWritten will modify the num_rows_ in RowGroupSerializer but this issue is about nums_rows_ in FileSerializer. They are two different variables.

[mapleFU]

You're right. Would this better to:

1. If has rowgroup writer, use the rowgroup writer
2. Otherwise, set the count when close a rowgroup, and uses this count

Previously it should be a bug

[HuaHuaY]

1. If has rowgroup writer, use the rowgroup writer
2. Otherwise, set the count when close a rowgroup, and uses this count

I think I get your point that we can add the num_rows() result of RowGroupSerializer to num_rows_ in FileSerializer before each call to RowGroupSerializer::Close and modify the num_rows() in FileSerializer:

class FileSerializer : public ParquetFileWriter::Contents {
  int64_t num_rows() const override { 
    return num_rows_ + row_group_writer_->num_rows();  // add `+ row_group_writer_->num_rows()`.
  }

  RowGroupWriter* AppendRowGroup(bool buffered_row_group) {
    if (row_group_writer_) {
      num_rows_ += row_group_writer_->num_rows(); // <- add this line.
      row_group_writer_->Close();
    }
    ...
  }
};

And we may need one more change:

class RowGroupSerializer : public RowGroupWriter::Contents {
  int64_t num_rows() const override {
    CheckRowsWritten();
    return num_rows_;
  }

  void CheckRowsWritten() const {
    if (!buffered_row_group_ && column_writers_.size() > 0 && column_writers_[0]) {
      int64_t current_col_rows = column_writers_[0]->rows_written();
      if (num_rows_ == 0) {
        num_rows_ = current_col_rows;
      } else if (num_rows_ != current_col_rows) {
        ThrowRowsMisMatchError(next_column_index_, current_col_rows, num_rows_); // <- should not throw an error here.
      }
    } ...
  }
};

If we allow ParquetFileWriter::num_rows() to be called multiple times, CheckRowsWritten will be called multiple times. Then we will update num_rows_ multiple times and cann't throw a mismatch error here.

[mapleFU]

This is ok for me, just record when close without updating is also ok for me

[mapleFU]

(Since it exists for so long, maybe we don't have really user for this, sigh)

[HuaHuaY]

I have submitted a PR and I just record when close. I think it fits this comment in ParquetFileWriter.

/// Number of rows in the yet started RowGroups.
///
/// Changes on the addition of a new RowGroup.
int64_t num_rows() const;