Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PARQUET-2139: Deprecate ColumnChunk::file_offset field #440

Merged
merged 3 commits into from
Jul 3, 2024

Conversation

etseidl
Copy link
Contributor

@etseidl etseidl commented Jun 25, 2024

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

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @etseidl -- this makes sense to me

Copy link
Contributor

@alkis alkis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

* location is wrong. This field is now deprecated and should not be used.
* Writers should set this field to 0 if no ColumnMetaData has been written outside
* the footer.
*/
2: required i64 file_offset
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea, thanks!

Copy link
Member

@julienledem julienledem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me. Thank you so much

Copy link
Contributor

@alkis alkis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thank you @etseidl.

@etseidl
Copy link
Contributor Author

etseidl commented Jun 26, 2024

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. 🙏

Copy link
Member

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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:

@alkis
Copy link
Contributor

alkis commented Jun 27, 2024

With the default in thrift IDL do we need to set this at all?

Comment on lines +876 to +877
* Writers should set this field to 0 if no ColumnMetaData has been written outside
* the footer.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm I don't understand would set a required field to 0 is ok and would it cracks the legacy reader

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@alkis alkis Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

README.md Outdated
@@ -89,38 +89,38 @@ more pages.
This file and the [Thrift definition](src/main/thrift/parquet.thrift) should be read together to understand the format.

4-byte magic number "PAR1"
<Column 1 Chunk 1 + Column Metadata>
<Column 2 Chunk 1 + Column Metadata>
<Column 1 Chunk 1 + Page Metadata>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@emkornfield emkornfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

running CI now but this looks good to me.

@emkornfield
Copy link
Contributor

Going to merge now. Thanks everyone for feedback and @etseidl for writing this up.

@mapleFU
Copy link
Member

mapleFU commented Aug 3, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants