Add DELTA_BINARY_PACKED encoder for Parquet writer #14100

etseidl · 2023-09-13T01:53:10Z

Description

Part of #13501. Adds ability to fall back on DELTA_BINARY_PACKED encoding when V2 page headers are selected and dictionary encoding is not possible.

Checklist

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

copy-pr-bot · 2023-09-13T01:53:14Z

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.

…code_delta_binary

PointKernel · 2023-09-18T18:30:07Z

/ok to test

…binary

cpp/src/io/parquet/delta_enc.cuh

ttnghia · 2023-10-18T16:18:34Z

cpp/src/io/parquet/delta_enc.cuh

+constexpr int values_per_mini_block = block_size / num_mini_blocks;
+constexpr int buffer_size           = 2 * block_size;
+
+// extra sanity checks to enforce compliance with the parquet specification


Nit: It is cleaner to have any sentence (including comments) begins with a capitalized letter and ends with a period. Sorry if I'm nitpicking too much.

No worries...hitting caps is an extra keystroke and I'm lazy 😅

ttnghia · 2023-10-18T16:21:43Z

cpp/src/io/parquet/delta_enc.cuh

+  uint8_t* _dst;                             // sink to dump encoded values to
+  size_type _current_idx;                    // index of first value in buffer
+  uint32_t _num_values;                      // total number of values to encode
+  size_type _values_in_buffer;               // current number of values stored in _buffer
+  T* _buffer;                                // buffer to store values to be encoded
+  uint8_t _mb_bits[delta::num_mini_blocks];  // bitwidth for each mini-block


Can you group pointers and values please?

ttnghia

LGTM. I just left a few comments about coding style.

…binary

ttnghia · 2023-10-18T22:07:31Z

The tests here seem to do round trip write/read. Can we have file written somewhere else that can be tested here? I'm not sure if we can do that in C++ code or have to do it in python?

etseidl · 2023-10-18T22:40:02Z

The tests here seem to do round trip write/read. Can we have file written somewhere else that can be tested here? I'm not sure if we can do that in C++ code or have to do it in python?

So the inverse of test_delta_binary in test_parquet.py, which writes with pandas and reads with cudf? So write with cudf and read with pyarrow or pandas? The issue is we don't yet expose the options necessary to write V2 with python. Can we use the arrow compatibility stuff? So on the C++ side, write with cudf, then read with arrow-cpp, convert arrow to cudf and compare?

vuule · 2023-10-18T22:51:11Z

The tests here seem to do round trip write/read. Can we have file written somewhere else that can be tested here? I'm not sure if we can do that in C++ code or have to do it in python?

So the inverse of test_delta_binary in test_parquet.py, which writes with pandas and reads with cudf? So write with cudf and read with pyarrow or pandas? The issue is we don't yet expose the options necessary to write V2 with python. Can we use the arrow compatibility stuff? So on the C++ side, write with cudf, then read with arrow-cpp, convert arrow to cudf and compare?

FWIW, we do have reader tests for this encoding that does not depend on the cudf writer. I guess it does not prove that reader only reads correctly encoded data, so writer could write something only cudf can read. Might be best to add the V2 API to Python...

ttnghia · 2023-10-19T21:46:57Z

Might be best to add the V2 API to Python...

Agree. The test can be added here or either in a separate PR.

ttnghia · 2023-10-20T05:46:40Z

/ok to test

vuule · 2023-10-20T16:31:11Z

/merge

During the review of #14100 there was a suggestion to add a test of writing using cudf and then reading the resulting file back with pyarrow. This PR adds the necessary python bindings to perform this test. NOTE: there is currently an issue with encoding 32-bit values where the deltas exceed 32-bits. parquet-mr and arrow truncate the deltas for the INT32 physical type and allow values to overflow, whereas cudf currently uses 64-bit deltas, which avoids the overflow, but can result in requiring 33-bits when encoding. The current cudf behavior is allowed by the specification (and in fact is readable by parquet-mr), but using the extra bit is not in the Parquet spirit of least output file size. This will be addressed in follow-on work. Authors: - Ed Seidl (https://github.com/etseidl) - Vukasin Milovanovic (https://github.com/vuule) Approvers: - Vukasin Milovanovic (https://github.com/vuule) - MithunR (https://github.com/mythrocks) - GALI PREM SAGAR (https://github.com/galipremsagar) URL: #14316

add DELTA_BINARY_PACKED encoder

91f7943

github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Sep 13, 2023

etseidl and others added 8 commits September 13, 2023 10:31

clean up some consts and change kernel mask to enum class

478f8ac

more cleanup

cd4df51

remove some FIXMEs

2e33420

change FIXME to TODO for TS scaling

0bd64c8

use switch rather than if/else block

fffc659

get rid of some magic numbers

a98b338

replace operator overload with templated function

9d1b2f2

Merge branch 'branch-23.10' into encode_delta_binary

dc14171

etseidl marked this pull request as ready for review September 14, 2023 00:00

etseidl requested a review from a team as a code owner September 14, 2023 00:00

etseidl requested review from vyasr and divyegala September 14, 2023 00:00

etseidl and others added 9 commits September 14, 2023 11:10

Merge branch 'rapidsai:branch-23.10' into encode_delta_binary

39b498c

redo typing of the delta bit packer

ea6f3c0

add const

2d41d3a

add test for delta binary writer

1d94621

remove unsupported types from delta test

4e2c4a5

make second column unordered

b3b25de

Merge branch 'rapidsai:branch-23.10' into encode_delta_binary

ce5378f

remove leftover experiment

00b248f

Merge branch 'encode_delta_binary' of github.com:etseidl/cudf into en…

1c75898

…code_delta_binary

PointKernel added cuIO cuIO issue 4 - Needs cuIO Reviewer non-breaking Non-breaking change feature request New feature or request labels Sep 18, 2023

Merge branch 'branch-23.10' into encode_delta_binary

dd526fa

etseidl and others added 5 commits October 9, 2023 10:49

finish merge

3db131b

Merge branch 'rapidsai:branch-23.12' into encode_delta_binary

057dc00

Merge remote-tracking branch 'origin/branch-23.12' into encode_delta_…

d766012

…binary

Merge branch 'branch-23.12' into encode_delta_binary

44ec4ff

Merge branch 'rapidsai:branch-23.12' into encode_delta_binary

28ed028

GregoryKimball requested a review from ttnghia October 18, 2023 16:13

ttnghia reviewed Oct 18, 2023

View reviewed changes

cpp/src/io/parquet/delta_enc.cuh Outdated Show resolved Hide resolved

ttnghia reviewed Oct 18, 2023

View reviewed changes

cpp/src/io/parquet/delta_enc.cuh Outdated Show resolved Hide resolved

ttnghia reviewed Oct 18, 2023

View reviewed changes

etseidl added 2 commits October 18, 2023 09:44

Merge remote-tracking branch 'origin/branch-23.12' into encode_delta_…

0bc8169

…binary

clean ups suggested in review

5db6312

vuule requested a review from ttnghia October 18, 2023 21:20

get rid of TODO

c1445ed

ttnghia approved these changes Oct 19, 2023

View reviewed changes

Merge branch 'branch-23.12' into encode_delta_binary

fab105a

rapids-bot bot merged commit f7ad66f into rapidsai:branch-23.12 Oct 20, 2023
54 checks passed

etseidl deleted the encode_delta_binary branch October 20, 2023 16:45

davidwendt mentioned this pull request Oct 23, 2023

[BUG] memcheck error reported in gpuEncodeDeltaBinaryPages in ParquetWriterDeltaTest/0.SupportedDeltaTestTypes #14310

Closed

etseidl mentioned this pull request Oct 23, 2023

Add python tests for Parquet DELTA_BINARY_PACKED encoder #14316

Merged

3 tasks

vyasr added 4 - Needs Review Waiting for reviewer to review or respond and removed 4 - Needs cuIO Reviewer labels Feb 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DELTA_BINARY_PACKED encoder for Parquet writer #14100

Add DELTA_BINARY_PACKED encoder for Parquet writer #14100

etseidl commented Sep 13, 2023

copy-pr-bot bot commented Sep 13, 2023

PointKernel commented Sep 18, 2023

ttnghia Oct 18, 2023 •

edited

Loading

etseidl Oct 18, 2023

ttnghia Oct 18, 2023

ttnghia left a comment

ttnghia commented Oct 18, 2023

etseidl commented Oct 18, 2023

vuule commented Oct 18, 2023

ttnghia commented Oct 19, 2023

ttnghia commented Oct 20, 2023

vuule commented Oct 20, 2023

Add DELTA_BINARY_PACKED encoder for Parquet writer #14100

Add DELTA_BINARY_PACKED encoder for Parquet writer #14100

Conversation

etseidl commented Sep 13, 2023

Description

Checklist

copy-pr-bot bot commented Sep 13, 2023

PointKernel commented Sep 18, 2023

ttnghia Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

etseidl Oct 18, 2023

Choose a reason for hiding this comment

ttnghia Oct 18, 2023

Choose a reason for hiding this comment

ttnghia left a comment

Choose a reason for hiding this comment

ttnghia commented Oct 18, 2023

etseidl commented Oct 18, 2023

vuule commented Oct 18, 2023

ttnghia commented Oct 19, 2023

ttnghia commented Oct 20, 2023

vuule commented Oct 20, 2023

ttnghia Oct 18, 2023 •

edited

Loading