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.

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

Make use of Parquet size statistics when reading #14714

Closed
wants to merge 43 commits into from

Conversation

etseidl
Copy link
Contributor

@etseidl etseidl commented Jan 5, 2024

Description

#14000 added writing of page size statistics to the Parquet writer. This PR is an attempt to use those statistics to skip some preprocessing steps, primarily the page string size calculations.

Still a work-in-progress...feedback welcome.

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 link

copy-pr-bot bot commented Jan 5, 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 Jan 5, 2024
@GregoryKimball GregoryKimball added feature request New feature or request cuIO cuIO issue and removed feature request New feature or request labels Jan 8, 2024
@vuule vuule self-requested a review January 8, 2024 23:44
@vuule vuule added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 8, 2024
@vuule
Copy link
Contributor

vuule commented Jan 8, 2024

/ok to test

@etseidl
Copy link
Contributor Author

etseidl commented Jan 10, 2024

Started some perf testing, and found that reading the indexes was having a negative impact on the non-string benchmarks. After dealing with that, the current code is anywhere from 1-20% faster for strings.

# parquet_read_decode

## [0] NVIDIA RTX A6000

|  data_type  |    io_type    |  cardinality  |  run_length  |   Ref Time |   Ref Noise |   Cmp Time |   Cmp Noise |         Diff |   %Diff |  
|-------------|---------------|---------------|--------------|------------|-------------|------------|-------------|--------------|---------|
|  INTEGRAL   | DEVICE_BUFFER |       0       |      1       |  13.827 ms |       0.74% |  13.950 ms |       0.81% |   122.719 us |   0.89% |   
|  INTEGRAL   | DEVICE_BUFFER |     1000      |      1       |  12.945 ms |       0.39% |  12.989 ms |       0.46% |    44.126 us |   0.34% |   
|  INTEGRAL   | DEVICE_BUFFER |       0       |      32      |  11.600 ms |       0.34% |  11.613 ms |       0.37% |    13.641 us |   0.12% |   
|  INTEGRAL   | DEVICE_BUFFER |     1000      |      32      |  11.043 ms |       0.70% |  11.014 ms |       0.50% |   -28.921 us |  -0.26% |   
|   STRING    | DEVICE_BUFFER |       0       |      1       |  46.105 ms |       0.48% |  45.537 ms |       0.45% |  -567.535 us |  -1.23% |   
|   STRING    | DEVICE_BUFFER |     1000      |      1       |  13.635 ms |       0.72% |  12.844 ms |       0.77% |  -790.722 us |  -5.80% |   
|   STRING    | DEVICE_BUFFER |       0       |      32      |  46.278 ms |       0.49% |  45.663 ms |       0.44% |  -615.361 us |  -1.33% |   
|   STRING    | DEVICE_BUFFER |     1000      |      32      |  12.796 ms |       1.30% |  10.352 ms |       1.49% | -2444.198 us | -19.10% |   
|    LIST     | DEVICE_BUFFER |       0       |      1       |  75.977 ms |       0.50% |  75.457 ms |       0.14% |  -520.594 us |  -0.69% |   
|    LIST     | DEVICE_BUFFER |     1000      |      1       |  63.353 ms |       0.45% |  62.867 ms |       0.48% |  -485.231 us |  -0.77% |   
|    LIST     | DEVICE_BUFFER |       0       |      32      |  53.426 ms |       1.04% |  53.189 ms |       1.19% |  -236.433 us |  -0.44% |   
|    LIST     | DEVICE_BUFFER |     1000      |      32      |  54.762 ms |       1.00% |  54.658 ms |       1.03% |  -103.946 us |  -0.19% |   
|   STRUCT    | DEVICE_BUFFER |       0       |      1       |  53.255 ms |       2.06% |  53.375 ms |       2.04% |   119.537 us |   0.22% |   
|   STRUCT    | DEVICE_BUFFER |     1000      |      1       |  32.553 ms |       0.43% |  32.295 ms |       0.67% |  -257.358 us |  -0.79% |   
|   STRUCT    | DEVICE_BUFFER |       0       |      32      |  52.191 ms |       2.68% |  52.075 ms |       2.62% |  -116.575 us |  -0.22% |   
|   STRUCT    | DEVICE_BUFFER |     1000      |      32      |  30.491 ms |       0.22% |  29.165 ms |       0.41% | -1325.684 us |  -4.35% | 

@etseidl
Copy link
Contributor Author

etseidl commented Jan 10, 2024

For comparison, reading the page stats when they aren't used has the following impact:

|  data_type  |    io_type    |  cardinality  |  run_length  |   Ref Time |   Ref Noise |   Cmp Time |   Cmp Noise |         Diff |   %Diff |
|-------------|---------------|---------------|--------------|------------|-------------|------------|-------------|--------------|---------|
|  INTEGRAL   | DEVICE_BUFFER |       0       |      1       |  13.827 ms |       0.74% |  15.025 ms |       2.35% |     1.198 ms |   8.66% |
|  INTEGRAL   | DEVICE_BUFFER |     1000      |      1       |  12.945 ms |       0.39% |  13.954 ms |       1.06% |     1.009 ms |   7.79% |
|  INTEGRAL   | DEVICE_BUFFER |       0       |      32      |  11.600 ms |       0.34% |  12.603 ms |       0.64% |     1.003 ms |   8.65% |
|  INTEGRAL   | DEVICE_BUFFER |     1000      |      32      |  11.043 ms |       0.70% |  12.062 ms |       0.36% |     1.019 ms |   9.23% |

@vuule
Copy link
Contributor

vuule commented Jan 11, 2024

/ok to test

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.

No fundamental concerns with the changes, just some minor comments/questions.

cpp/src/io/parquet/reader_impl_helpers.hpp Outdated Show resolved Hide resolved
cpp/src/io/parquet/parquet_gpu.hpp Outdated Show resolved Hide resolved
cpp/src/io/parquet/reader_impl_helpers.cpp Show resolved Hide resolved
cpp/src/io/parquet/reader_impl_helpers.cpp Outdated Show resolved Hide resolved
cpp/src/io/parquet/reader_impl_helpers.cpp Outdated Show resolved Hide resolved
cpp/src/io/parquet/reader_impl_chunking.cu Show resolved Hide resolved
@etseidl etseidl changed the base branch from branch-24.02 to branch-24.04 January 18, 2024 20:06
@etseidl
Copy link
Contributor Author

etseidl commented Jan 19, 2024

Going to close this for now and wait for #14360 to merge before trying again.

@etseidl etseidl closed this Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuIO cuIO issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants