PARQUET-2139: Deprecate ColumnChunk::file_offset field #440
Conversation
src/main/thrift/parquet.thrift
Outdated
| * Writers should set this field to 0 if no ColumnMetaData has been written outside | ||
| * the footer. | ||
| */ | ||
| 2: required i64 file_offset |
There was a problem hiding this comment.
Can we set a default value of 0 here so that we can stop writers from having to set it (according to the spec it will still be written on the wire).
There was a problem hiding this comment.
That's a good idea, thanks!
julienledem
left a comment
julienledem
left a comment
There was a problem hiding this comment.
This looks great to me. Thank you so much
|
Thanks all for the good discussion here and on the mailing list. @wgtmac when you have time could you also take a look? If this is adopted it will allow apache/parquet-java#1369 to move forward. 🙏 |
wgtmac
left a comment
wgtmac
left a comment
There was a problem hiding this comment.
+1 Thanks @etseidl for working on this!
cc @pitrou @emkornfield @mapleFU as I think we can change following lines on the parquet-cpp (together with rust and go) side by not writing ColumnMetaData and set the file_offset to 0:
|
With the default in thrift IDL do we need to set this at all? |
| * Writers should set this field to 0 if no ColumnMetaData has been written outside | ||
| * the footer. |
There was a problem hiding this comment.
Hmmm I don't understand would set a required field to 0 is ok and would it cracks the legacy reader
There was a problem hiding this comment.
Yes, required fields are allowed to have default values (see this for some messy details). I looked at code generated by thrift 0.21.0. Java and c++ set the field to the default value in the default constructor, and write the field regardless of its value. The generated rust code doesn't appear to set the default value, however.
It is conceivable that there is a thrift implementation that would not write the '0', which would then break an old reader that expects the field to be set. Maybe I should revert setting a default. @alkis what do you think?
There was a problem hiding this comment.
This is required by the thrift spec.
From https://thrift.apache.org/docs/idl#required:
Write: Required fields are always written and are expected to be set.
Read: Required fields are always read and are expected to be contained in the input stream.
Defaults values: are always written
In any case leaving this with a default of 0 doesn't hurt. Java and C++ can stop populating this field as soon as they pick up the new thrift IDL. Rust cannot stop populating because its thrift implementation is broken. When its fixed, it can also simplify.
| /** Column metadata for this chunk. Some writers may also replicate this at the | ||
| * location pointed to by file_path/file_offset. | ||
| * Note: while marked as optional, this field is in fact required by most major | ||
| * Parquet implementations. As such, writers MUST populate this field. |
README.md
Outdated
| 4-byte magic number "PAR1" | ||
| <Column 1 Chunk 1 + Column Metadata> | ||
| <Column 2 Chunk 1 + Column Metadata> | ||
| <Column 1 Chunk 1 + Page Metadata> |
There was a problem hiding this comment.
Hmm why this is Page Metadata? It's a bit confusing for me, since from ColumnChunk, we may not get the details of the page internal (like how many pages we have?)
There was a problem hiding this comment.
I switched it for two reasons. First was a comment @julienledem made. The second is that since the following section on metadata now states there are two types of metadata (file metadata and page header metadata), both should appear in the diagram. Perhaps the diagram should say "Page Header Metadata", or just omit all mention of metadata and simply have <Column 1 Chunk 1>. I don't have a strong preference beyond it should no longer show "Column Metadata".
There was a problem hiding this comment.
There is no bijection between column chunks and page headers, so I find this highly misleading.
You could make it "Optional Column Metadata" if you want stress that it's not always there, or you could indeed omit it if it's not an important feature anymore.
There was a problem hiding this comment.
Since the following diagram makes clear that a column chunk is a collection of pages, and pages include the page header, I'm fine with omitting all mention of metadata here.
emkornfield
left a comment
emkornfield
left a comment
There was a problem hiding this comment.
running CI now but this looks good to me.
|
Going to merge now. Thanks everyone for feedback and @etseidl for writing this up. |
|
FYI the ColumnChunkMetadata after ColumnChunk, which is used in arrow-rs and arrow-cpp ( I didn't check implemetation in other languages), is deprecated, since parquet-java doesn't write that and other implementation doesn't read that |
9 participants
An alternative to #439
Related to parquet-java/#2678 and apache/parquet-java#1369
See mailing list discussion at https://lists.apache.org/thread/r6r2cvzrdoorq6h6gqwh0b1hbfbhxv29
Jira