Add parameters to control row group size in Parquet writer #9677

vuule · 2021-11-12T22:44:32Z

Closes #9615

Adds the following API to the Parquet writer:

Set maximum row group size, in bytes (minimum of 512KB);
Set maximum row group size, in rows (minimum of 5000).

The API is more limited than its ORC equivalent because of limitation in Parquet page size control/estimation.

Other changes:

Fix naming in some ORC APIs to be consistent.
Change rowgroup to row_group in APIs, since Parquet specs refer to this as "row group", not "rowgroup".
Replace some uint32_t use in Parquet writer.
Remove unused target_page_size.

…fea-parquet-rowgroup-size

bdice

Nice cleanup. Comments attached.

cpp/include/cudf/io/parquet.hpp

cpp/src/io/parquet/writer_impl.cu

python/cudf/cudf/_lib/cpp/io/orc.pxd

Co-authored-by: Bradley Dice <[email protected]>

bdice

The commentary about page size was a helpful improvement. Thanks!

PointKernel

LGTM. Just some coding-style comments for your reference.

cpp/include/cudf/io/orc.hpp

PointKernel · 2021-11-15T22:16:12Z

cpp/include/cudf/io/orc.hpp


  /**
   * @brief Returns maximum stripe size, in bytes.
   */
-  auto stripe_size_bytes() const { return _stripe_size_bytes; }
+  auto get_stripe_size_bytes() const { return _stripe_size_bytes; }


I remember a while back, @codereport and @jrhemstad had a long discussion on whether get prefix should be used or not and the outcome is that no prefix is more readable IIRC.

That's true in general. However, we made a conscious decision to use get_ prefix for the _options classes. I'm unable to find the discussion that led to this (perhaps Jake had the discussion with Conor later on).
We can choose to remove the prefix, but it should be done for all options. Either way, I prefer to keep the API consistent, so I'm adding these options with the prefix.

cpp/src/io/parquet/writer_impl.cu

Co-authored-by: Yunsong Wang <[email protected]>

python/cudf/cudf/io/parquet.py

codecov · 2021-11-16T08:58:47Z

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.02@36b3344). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-22.02    #9677   +/-   ##
===============================================
  Coverage                ?   10.51%           
===============================================
  Files                   ?      118           
  Lines                   ?    20249           
  Branches                ?        0           
===============================================
  Hits                    ?     2130           
  Misses                  ?    18119           
  Partials                ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36b3344...a9e04f7. Read the comment docs.

vuule · 2021-11-17T18:31:07Z

@gpucibot merge

This PR fixes #9615 Adds two more parameters to the Parquet writer options objects to control page sizes. One sets a target page size in bytes (defaults to 512 KiB), and a second to set a maximum number of rows per page (defaults to 20000, see [this](https://blog.cloudera.com/speeding-up-select-queries-with-parquet-page-indexes/) for the rationale behind this choice). ~~This also removes the validation logic (and unit tests) from the row_group_size setters, since the page size is no longer fixed. Perhaps validation could be moved to the build() function.~~ I've tried to follow the naming convention from #9677 Still needs help on python bindings and unit tests. Authors: - https://github.com/etseidl Approvers: - Devavret Makkar (https://github.com/devavret) - Vukasin Milovanovic (https://github.com/vuule) - Bradley Dice (https://github.com/bdice) URL: #10882

vuule added 9 commits November 11, 2021 00:19

move constants; fix types

7944d9a

API

eb9610d

rename APIs to match other getters

d67f5bf

fix naming; remove unused API

0e16b42

rename merge_rowgroup_metadata

c3a3420

negative C++ test

e3ae909

Python API

eff9cbd

Python test

8699174

Merge branch 'branch-21.12' of https://github.com/rapidsai/cudf into …

40f3827

…fea-parquet-rowgroup-size

vuule added feature request New feature or request cuIO cuIO issue breaking Breaking change labels Nov 12, 2021

vuule self-assigned this Nov 12, 2021

github-actions bot added Python Affects Python cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels Nov 12, 2021

clean up

5c2c59b

vuule marked this pull request as ready for review November 12, 2021 23:45

vuule requested review from a team as code owners November 12, 2021 23:45

vuule requested review from harrism, bdice and shwina November 12, 2021 23:45

bdice requested changes Nov 13, 2021

View reviewed changes

vuule and others added 3 commits November 15, 2021 10:33

Apply suggestions from code review

7462241

Co-authored-by: Bradley Dice <[email protected]>

missed renames

9b4fc67

Co-authored-by: Bradley Dice <[email protected]>

clarify that the minimum row group size is based on page size

61d4e6a

vuule requested a review from bdice November 15, 2021 20:03

bdice approved these changes Nov 15, 2021

View reviewed changes

vuule requested a review from PointKernel November 15, 2021 20:35

PointKernel reviewed Nov 15, 2021

View reviewed changes

vuule and others added 2 commits November 15, 2021 15:03

review suggestion

2404be8

Co-authored-by: Yunsong Wang <[email protected]>

auto

cbc8ecd

vuule requested a review from PointKernel November 15, 2021 23:54

PointKernel approved these changes Nov 15, 2021

View reviewed changes

shwina reviewed Nov 16, 2021

View reviewed changes

python/cudf/cudf/io/parquet.py Show resolved Hide resolved

test fix

de502cf

py docs

a9e04f7

vuule requested a review from shwina November 16, 2021 20:18

shwina approved these changes Nov 17, 2021

View reviewed changes

rapids-bot bot merged commit 9114104 into rapidsai:branch-22.02 Nov 17, 2021

vuule deleted the fea-parquet-rowgroup-size branch November 17, 2021 18:31

bdice mentioned this pull request Dec 10, 2021

Row-group-level partitioning for Parquet #9849

Closed

4 tasks

etseidl mentioned this pull request May 17, 2022

Add parameters to control page size in Parquet writer #10882

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parameters to control row group size in Parquet writer #9677

Add parameters to control row group size in Parquet writer #9677

vuule commented Nov 12, 2021 •

edited

Loading

bdice left a comment

bdice left a comment

PointKernel left a comment

PointKernel Nov 15, 2021

vuule Nov 15, 2021

codecov bot commented Nov 16, 2021 •

edited

Loading

vuule commented Nov 17, 2021

Add parameters to control row group size in Parquet writer #9677

Add parameters to control row group size in Parquet writer #9677

Conversation

vuule commented Nov 12, 2021 • edited Loading

bdice left a comment

Choose a reason for hiding this comment

bdice left a comment

Choose a reason for hiding this comment

PointKernel left a comment

Choose a reason for hiding this comment

PointKernel Nov 15, 2021

Choose a reason for hiding this comment

vuule Nov 15, 2021

Choose a reason for hiding this comment

codecov bot commented Nov 16, 2021 • edited Loading

Codecov Report

vuule commented Nov 17, 2021

vuule commented Nov 12, 2021 •

edited

Loading

codecov bot commented Nov 16, 2021 •

edited

Loading