Skip to content

Add BYTE_STREAM_SPLIT support to Parquet#15311

Merged
rapids-bot[bot] merged 55 commits intorapidsai:branch-24.06from
etseidl:byte_stream_split
Apr 24, 2024
Merged

Add BYTE_STREAM_SPLIT support to Parquet#15311
rapids-bot[bot] merged 55 commits intorapidsai:branch-24.06from
etseidl:byte_stream_split

Conversation

@etseidl
Copy link
Copy Markdown
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 link
Copy Markdown

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
Copy Markdown
Contributor

vuule commented Mar 14, 2024

/ok to test

@etseidl
Copy link
Copy Markdown
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
Copy Markdown
Member

mhaseeb123 commented Apr 9, 2024

/ok to test

mhaseeb123
mhaseeb123 reviewed Apr 9, 2024
View reviewed changes
mhaseeb123
mhaseeb123 reviewed Apr 9, 2024
View reviewed changes
Copy link
Copy Markdown
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.

@mhaseeb123
Copy link
Copy Markdown
Member

mhaseeb123 commented Apr 10, 2024 •
edited
Loading

Looks good to me. Thanks for the effort @etseidl

@mhaseeb123
Copy link
Copy Markdown
Member

mhaseeb123 commented Apr 10, 2024

/ok to test

@mhaseeb123
Copy link
Copy Markdown
Member

mhaseeb123 commented Apr 16, 2024

/ok to test

vuule
vuule reviewed Apr 17, 2024
View reviewed changes
Copy link
Copy Markdown
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_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
Copy Markdown
Contributor

@vuule vuule Apr 17, 2024

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.

@vuule
Copy link
Copy Markdown
Contributor

vuule commented Apr 17, 2024

/ok to test

vuule
vuule approved these changes Apr 18, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

vuule commented Apr 18, 2024

/ok to test

@vuule
Copy link
Copy Markdown
Contributor

vuule commented Apr 24, 2024

/ok to test

@vuule
Copy link
Copy Markdown
Contributor

vuule commented Apr 24, 2024

/merge

@rapids-bot rapids-bot bot merged commit 117eff6 into rapidsai:branch-24.06 Apr 24, 2024
@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

@mhaseeb123 mhaseeb123 mhaseeb123 approved these changes

@vuule vuule vuule 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