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.

Sign up for GitHub

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

Jump to bottom

Add BYTE_STREAM_SPLIT support to Parquet #15311

Merged
merged 55 commits into from
Apr 24, 2024
Merged

Add BYTE_STREAM_SPLIT support to Parquet #15311

merged 55 commits into from
Apr 24, 2024

Conversation

etseidl
Copy link
Contributor

@etseidl etseidl commented Mar 14, 2024

Description

Closes #15226. Part of #13501. Adds support for reading and writing BYTE_STREAM_SPLIT encoded Parquet data. Includes a "microkernel" version like those introduced by #15159.

Checklist

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

@etseidl etseidl requested a review from a team as a code owner March 14, 2024 21:40
@etseidl etseidl requested review from karthikeyann and vuule March 14, 2024 21:41
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Mar 14, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Mar 14, 2024
@vuule vuule added feature request New feature or request non-breaking Non-breaking change labels Mar 14, 2024
@vuule
Copy link
Contributor

vuule commented Mar 14, 2024

/ok to test

@etseidl
Copy link
Contributor Author

etseidl commented Mar 14, 2024

BYTE_STREAM_SPLIT was originally just for floating point, but it is being expanded to all fixed-width types (apache/parquet-format#229). I did a quick comparison with some mixed double/integer data to demonstrate the utility of the new encoding. While the uncompressed size of the data matches PLAIN encoding, when coupled with compression BYTE_STREAM_SPLIT can outperform the other encoding options in terms of both speed and compressed data size. The one exception being uncompressed DELTA_BINARY_PACKED which achieves a data reduction comparable to ZSTD compression, for a far lower computational cost.

             dict   plain   delta    bss   delta (no comp)
decompress   214ms     52     217     26      0
decode         8ms      6      10      9     10
encode        13ms      6      13      7     17
compress     438ms    369     333    223      0
round trp   1155ms    800     972    611    465
uncomp sz     25MB     40      18     40     18
comp sz       16MB     15      16     14     18

@GregoryKimball GregoryKimball mentioned this pull request Mar 15, 2024
Closed
@mhaseeb123
Copy link
Member

/ok to test

mhaseeb123
mhaseeb123 reviewed Apr 9, 2024
View reviewed changes
Copy link
Member

@mhaseeb123 mhaseeb123 left a comment

Choose a reason for hiding this comment

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

A few comments about the use of ts_scale in TIME_MILLIS type as seconds and days are encoded as millis in Parquet plus corresponding time units in tests.

cpp/tests/io/parquet_writer_test.cpp Outdated Show resolved Hide resolved
cpp/src/io/parquet/page_data.cu Show resolved Hide resolved
@mhaseeb123
Copy link
Member

mhaseeb123 commented Apr 10, 2024 •
edited
Loading

Looks good to me. Thanks for the effort @etseidl

@mhaseeb123
Copy link
Member

/ok to test

@mhaseeb123
Copy link
Member

/ok to test

vuule
vuule reviewed Apr 17, 2024
View reviewed changes
Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

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

looks great, just a few small comments

cpp/src/io/parquet/page_data.cuh Outdated Show resolved Hide resolved
cpp/src/io/parquet/page_enc.cu Outdated
Comment on lines 1675 to 1677
is_split_stream ? Encoding::BYTE_STREAM_SPLIT
: determine_encoding(
s->page.page_type, physical_type, s->ck.use_dictionary, write_v2_headers);
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: consider moving the is_split_stream check into determine_encoding. This way we have the entire logic in determine_encoding.

cpp/src/io/parquet/page_data.cu Outdated Show resolved Hide resolved
cpp/src/io/parquet/decode_fixed.cu Outdated Show resolved Hide resolved
@vuule
Copy link
Contributor

vuule commented Apr 17, 2024

/ok to test

vuule
vuule approved these changes Apr 18, 2024
View reviewed changes
Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

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

🔥

@vuule
Copy link
Contributor

vuule commented Apr 18, 2024

/ok to test

@vuule
Copy link
Contributor

vuule commented Apr 24, 2024

/ok to test

@vuule
Copy link
Contributor

vuule commented Apr 24, 2024

/merge

@rapids-bot rapids-bot bot merged commit 117eff6 into rapidsai:branch-24.06 Apr 24, 2024
71 checks passed
@etseidl etseidl deleted the byte_stream_split branch April 24, 2024 19:31
@etseidl etseidl mentioned this pull request May 22, 2024
Merged
3 tasks
rapids-bot bot pushed a commit that referenced this pull request Jun 24, 2024
@etseidl
Add test of interoperability of cuDF and arrow BYTE_STREAM_SPLIT enco…
ed41668 
…ders (#15832)

BYTE_STREAM_SPLIT encoding was recently added to cuDF (#15311). The Parquet specification was recently changed (apache/parquet-format#229) to extend the datatypes that can be encoded as BYTE_STREAM_SPLIT, and this was only recently implemented in arrow (apache/arrow#40094). This PR adds a check that cuDF and arrow can produce compatible files using BYTE_STREAM_SPLIT encoding.

Authors:
  - Ed Seidl (https://github.com/etseidl)

Approvers:
  - Lawrence Mitchell (https://github.com/wence-)

URL: #15832
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vuule vuule vuule approved these changes

@mhaseeb123 mhaseeb123 mhaseeb123 approved these changes

@karthikeyann karthikeyann Awaiting requested review from karthikeyann karthikeyann is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

@nvdbaranec nvdbaranec Awaiting requested review from nvdbaranec

Assignees
No one assigned
Labels
feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
Parquet continuous improvement
Development

Successfully merging this pull request may close these issues.

[FEA] Parquet reader and writer should support BYTE_STREAM_SPLIT encoding
4 participants
@etseidl @vuule @mhaseeb123 @GregoryKimball