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

Write parquet column chunk file_offset #15882

Conversation

pmattione-nvidia
Copy link
Contributor

@pmattione-nvidia pmattione-nvidia commented May 29, 2024

Set the file_offset member of the parquet column chunk metadata, which was not set before.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@pmattione-nvidia pmattione-nvidia added libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels May 29, 2024
@pmattione-nvidia pmattione-nvidia requested a review from vuule May 29, 2024 20:00
@pmattione-nvidia pmattione-nvidia self-assigned this May 29, 2024
@pmattione-nvidia pmattione-nvidia requested a review from a team as a code owner May 29, 2024 20:00
@pmattione-nvidia pmattione-nvidia requested a review from harrism May 29, 2024 20:00
@vuule
Copy link
Contributor

vuule commented May 29, 2024

CC @etseidl, the resident Parquet writer wizard

auto& column_chunk_meta = row_group.columns[i].meta_data;
auto const chunk_offset = _current_chunk_offset[p];
auto& column_chunk = row_group.columns[i];
column_chunk.file_offset = chunk_offset;
Copy link
Contributor

Choose a reason for hiding this comment

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

Danger Will Robinson! There appears to be confusion in the community about what file_offset is. According to the parquet thrift this points to the chunk metadata, not the start of the chunk. This allows for the metadata to be separated from the thrift in the footer, both to allow for faster parsing, and to support data living in a file separate from where the footer is located. (See apache/parquet-format#242 (comment) for some discussion of the latter use case). It seems as though parquet-mr incorrectly populates this with the offset of the first page, while arrow does what the spec mandates and uses it to point to a duplicate copy of the ColumnMetaData that is located at the end of a column chunk (as per the figure in the spec). Since libcudf does not write this copy, it's best to leave this field as '0'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see. OK, I'll close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants