Read FIXED_LEN_BYTE_ARRAY as binary in parquet reader by PointKernel · Pull Request #13437 · NVIDIA/cudf

PointKernel · 2023-05-24T20:17:46Z

Description

This PR adds support of reading FIXED_LEN_BYTE_ARRAY as lists of INT8 in the parquet reader.

Checklist

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

…ed-len-binary

etseidl · 2023-05-24T20:35:13Z

Can this be deferred until #13302 is finished (or rejected >.<)? I think the fixed width byte arrays can be processed more efficiently since they'll behave like any other fixed-width type (sizing can be known up front).

PointKernel · 2023-05-24T20:39:12Z

Can this be deferred until #13302 is finished (or rejected >.<)? I think the fixed width byte arrays can be processed more efficiently since they'll behave like any other fixed-width type (sizing can be known up front).

@etseidl sure, this is still a draft PR and I'm putting it up just for testing.

revans2 · 2023-05-25T19:15:37Z

I ran some tests with this and it looks good to me. Note that I just did some very simple tests though.

PointKernel · 2023-05-25T19:22:39Z

I ran some tests with this and it looks good to me. Note that I just did some very simple tests though.

Thanks for testing. I have some improvements to apply once #13302 is merged. In the meanwhile, @revans2 please let me know if you find any further issues with the current work.

hyperbolic2346

I like how this is working out. Nice cleanups along the way as well!

hyperbolic2346 · 2023-06-20T18:46:21Z

 {
  auto [ptr, len] = gpuGetStringData(s, sb, src_pos);
-  if (s->dtype_len == 4) {
+  if (s->dtype_len == 4 and (s->col.data_type & 7) == BYTE_ARRAY) {


Should this really be a logical and? It seems in the past we would take this path if the data size was 4 bytes, but now we only do it if the data size if 4 bytes AND it is a byte array.

Before this PR, only BYTE_ARRAY invokes gpuOutputString and the input length cannot be 4 in that case. Now with function being potentially invoked by FIXED_LEN_BYTE_ARRAY where the length could be 4, this and logic is needed.

maybe a comment along the lines of "// make sure to only hash variable length byte arrays when specified with the output type size"?

hyperbolic2346 · 2023-06-20T18:52:08Z

    return;
  }

+  auto const data_type = s->col.data_type & 7;


I liked seeing this change above to use this type, but why not move this above and use this variable all around?

data_type is not used that often in other functions. Let me have a further look.

IMO ColumnChunkDesc should have data_type() and type_len() so the bitwise packing madness is hidden from the rest of the code.
data_type & 7 appears 11 times in the code :\

Can we use parquet::Type directly instead of uint16_t?
https://github.com/rapidsai/cudf/blob/41f0caf53662cfde8146647574e705982eb558b1/cpp/src/io/parquet/parquet_gpu.hpp#L266-L267

(for some reason) data_type holds the type in the last three bits and length in the rest of the number. Using Type here would make this data member even more misleading.

…ed-len-binary

PointKernel · 2023-07-05T21:38:45Z

 {
  auto [ptr, len] = gpuGetStringData(s, sb, src_pos);
-  if (s->dtype_len == 4) {
+  if (s->dtype_len == 4 and (s->col.data_type & 7) == BYTE_ARRAY) {


Before this PR, only BYTE_ARRAY invokes gpuOutputString and the input length cannot be 4 in that case. Now with function being potentially invoked by FIXED_LEN_BYTE_ARRAY where the length could be 4, this and logic is needed.

etseidl · 2023-07-05T22:18:59Z

So will this take FIXED_LEN_BYTE_ARRAY data in and produce a string column on output? Would it be better to produce a fixed length UINT8 array? I'm a little bit worried about getting the column statistics right.

PointKernel · 2023-07-05T22:32:30Z

So will this take FIXED_LEN_BYTE_ARRAY data in and produce a string column on output? Would it be better to produce a fixed length UINT8 array? I'm a little bit worried about getting the column statistics right.

Right, similar to BYTE_ARRAY, FIXED_LEN_BYTE_ARRAY will be handled as string columns by default.

Would it be better to produce a fixed length UINT8 array?

This work is based on the assumption that users may switch between string and binary as the eventual output of FIXED_LEN_BYTE_ARRAY. Are you suggesting producing binary as default and converting back to strings when requested?

I'm a little bit worried about getting the column statistics right.

Valid concern, I didn't think about this.

…ed-len-binary

…f into parquet-fixed-len-binary

vuule

Small change, but tricky! Cool stuff 👍
Partial review, will continue tomorrow.

vuule · 2023-08-17T02:01:29Z

 {
  auto [ptr, len] = gpuGetStringData(s, sb, src_pos);
-  if (s->dtype_len == 4) {
+  if (s->dtype_len == 4 and (s->col.data_type & 7) == BYTE_ARRAY) {


not really related to this PR, but we have this dtype_len == 4 condition as a stand-in for "output hash" in a few places and it really requires detailed knowledge of the code to understand. Nothing actionable, I just don't like it :D

…ed-len-binary

vuule

looks good, just a small suggestion

Co-authored-by: Vukasin Milovanovic <vmilovanovic@nvidia.com>

revans2

Reran my tests and they still all pass

Co-authored-by: GALI PREM SAGAR <sagarprem75@gmail.com>

vuule · 2023-08-24T23:28:42Z

/merge

#13437 added the ability to consume FIXED_LEN_BYTE_ARRAY encoded data and represent it as lists of `UINT8`. When trying to write this data back to Parquet there are two problems. 1) the notion of fixed length is lost, and 2) the `UINT8` data is written as a list of `INT32` which can quadruple the storage required. This PR addresses both issues by adding fields to the input and output metadata to allow for preserving the form of the original data. Authors: - Ed Seidl (https://github.com/etseidl) - Vukasin Milovanovic (https://github.com/vuule) Approvers: - Vukasin Milovanovic (https://github.com/vuule) - Muhammad Haseeb (https://github.com/mhaseeb123) URL: #15600

PointKernel added 2 commits May 24, 2023 13:11

Read fixed len byte array as string

138cf3f

Merge remote-tracking branch 'upstream/branch-23.08' into parquet-fix…

f39a02f

…ed-len-binary

github-actions Bot added the libcudf Affects libcudf (C++/CUDA) code. label May 24, 2023

PointKernel self-assigned this May 24, 2023

PointKernel added feature request New feature or request cuIO cuIO issue non-breaking Non-breaking change labels May 24, 2023

revans2 mentioned this pull request May 25, 2023

Add in support for FIXED_LEN_BYTE_ARRAY as binary NVIDIA/cudf-spark#8404

Merged

GregoryKimball mentioned this pull request Jun 2, 2023

[FEA] Support V2 encodings in Parquet reader and writer #13501

Closed

etseidl mentioned this pull request Jun 7, 2023

Write string data directly to column_buffer in Parquet reader #13302

Merged

3 tasks

hyperbolic2346 requested changes Jun 20, 2023

View reviewed changes

PointKernel added 6 commits July 3, 2023 12:24

Merge with upstream

64783d2

Merge remote-tracking branch 'upstream/branch-23.08' into parquet-fix…

fdc8b7e

…ed-len-binary

TEST

c7b32b2

Cleanups

2c685a2

Revert test changes

cb6825f

Cleanups

c75675b

PointKernel commented Jul 5, 2023

View reviewed changes

etseidl reviewed Jul 5, 2023

View reviewed changes

Comment thread cpp/src/io/parquet/page_string_decode.cu Outdated

etseidl reviewed Jul 5, 2023

View reviewed changes

Comment thread cpp/src/io/parquet/page_string_decode.cu Outdated

Set up dtype_len properly

fac2eff

PointKernel added 3 commits July 5, 2023 15:34

updates

86e41a7

Cleanup

7f363b1

Use switch instead of if conditions

70ef046

Minor fix: compare the lower 3 bits only

5d01183

PointKernel commented Aug 11, 2023

View reviewed changes

Comment thread python/cudf/cudf/tests/data/parquet/fixed_len_byte_array.parquet

PointKernel added 4 commits August 14, 2023 15:47

Merge branch 'branch-23.10' into parquet-fixed-len-binary

9f5ff20

Use a smaller test file

50ec387

Merge remote-tracking branch 'upstream/branch-23.10' into parquet-fix…

f756e56

…ed-len-binary

Merge branch 'parquet-fixed-len-binary' of github.com:PointKernel/cud…

6304001

…f into parquet-fixed-len-binary

vuule reviewed Aug 17, 2023

View reviewed changes

PointKernel added 4 commits August 17, 2023 09:22

Minor cleanup: init right after declaration to get rid of if branches

a969c00

Cleanups: descriptive name for intermediate vars

277557c

Add comment

b712f5f

Merge remote-tracking branch 'upstream/branch-23.10' into parquet-fix…

efc495d

…ed-len-binary

vuule approved these changes Aug 17, 2023

View reviewed changes

Comment thread cpp/src/io/parquet/reader_impl.cpp Outdated

PointKernel and others added 2 commits August 17, 2023 11:04

Update cpp/src/io/parquet/reader_impl.cpp

2427416

Co-authored-by: Vukasin Milovanovic <vmilovanovic@nvidia.com>

Merge branch 'branch-23.10' into parquet-fixed-len-binary

77b3d38

hyperbolic2346 approved these changes Aug 23, 2023

View reviewed changes

vuule added 4 - Needs cuDF (Python) Reviewer and removed 4 - Needs cuIO Reviewer labels Aug 23, 2023

revans2 approved these changes Aug 23, 2023

View reviewed changes

vuule added 2 commits August 23, 2023 12:32

Merge branch 'branch-23.10' into parquet-fixed-len-binary

6453662

Merge branch 'branch-23.10' into parquet-fixed-len-binary

dfdcb76

galipremsagar reviewed Aug 24, 2023

View reviewed changes

Comment thread python/cudf/cudf/tests/test_parquet.py Outdated

Update python/cudf/cudf/tests/test_parquet.py

ed1c81c

Co-authored-by: GALI PREM SAGAR <sagarprem75@gmail.com>

galipremsagar approved these changes Aug 24, 2023

View reviewed changes

rapids-bot Bot merged commit 6095a92 into NVIDIA:branch-23.10 Aug 24, 2023

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

etseidl mentioned this pull request Apr 25, 2024

Round trip FIXED_LEN_BYTE_ARRAY data properly in Parquet writer #15600

Merged

3 tasks

PointKernel deleted the parquet-fixed-len-binary branch May 23, 2024 22:47

Uh oh!

Conversation

PointKernel commented May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

etseidl commented May 24, 2023

Uh oh!

PointKernel commented May 24, 2023

Uh oh!

revans2 commented May 25, 2023

Uh oh!

PointKernel commented May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hyperbolic2346 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

etseidl commented Jul 5, 2023

Uh oh!

PointKernel commented Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

vuule left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vuule left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

revans2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vuule commented Aug 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

PointKernel commented May 24, 2023 •

edited

Loading

PointKernel commented May 25, 2023 •

edited

Loading

PointKernel commented Jul 5, 2023 •

edited

Loading