wgtmac commented on code in PR #43428:
URL: https://github.com/apache/arrow/pull/43428#discussion_r1702806441
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -667,8 +669,9 @@ class BufferedPageWriter : public PageWriter {
has_dictionary, fallback, pager_->dict_encoding_stats_,
pager_->data_encoding_stats_, pager_->meta_encryptor_);
- // Write metadata at end of column chunk
- metadata_->WriteTo(in_memory_sink_.get());
+ // Not write metadata at end of column chunk since we will
Review Comment:
ditto
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -353,8 +353,10 @@ class SerializedPageWriter : public PageWriter {
total_compressed_size_, total_uncompressed_size_,
has_dictionary,
fallback, dict_encoding_stats_, data_encoding_stats_,
meta_encryptor_);
- // Write metadata at end of column chunk
- metadata_->WriteTo(sink_.get());
+
+ // Not write metadata at end of column chunk since we will
Review Comment:
I'd rather removing these lines to not confuse readers.
##########
cpp/src/parquet/metadata.cc:
##########
@@ -1536,10 +1536,11 @@ class
ColumnChunkMetaDataBuilder::ColumnChunkMetaDataBuilderImpl {
const std::shared_ptr<Encryptor>& encryptor) {
if (dictionary_page_offset > 0) {
column_chunk_->meta_data.__set_dictionary_page_offset(dictionary_page_offset);
- column_chunk_->__set_file_offset(dictionary_page_offset +
compressed_size);
- } else {
- column_chunk_->__set_file_offset(data_page_offset + compressed_size);
}
+ // https://github.com/apache/parquet-format/pull/440
+ // The `file_offset` field is deprecated and should be set to 0 for writer
+ // if the column chunk has not been written outsidethe footer.
Review Comment:
```suggestion
// The `file_offset` field is deprecated and should be set to 0.
// See https://github.com/apache/parquet-format/pull/440 for detail.
```
##########
cpp/src/parquet/metadata.h:
##########
@@ -146,7 +146,12 @@ class PARQUET_EXPORT ColumnChunkMetaData {
bool Equals(const ColumnChunkMetaData& other) const;
- // column chunk
+ // Byte offset of `ColumnMetaData` in `file_path()`.
+ //
+ // Note that the meaning of this field has been inconsistent between
implementations
Review Comment:
```suggestion
// Note that the meaning of this field has been inconsistent among
implementations
```
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
