Skip to content

Add read_orc_metadata to libcudf - #11815

Merged
rapids-bot[bot] merged 27 commits into
NVIDIA:branch-22.12from
vuule:fea-read_orc_metadata
Nov 1, 2022
Merged

Add read_orc_metadata to libcudf#11815
rapids-bot[bot] merged 27 commits into
NVIDIA:branch-22.12from
vuule:fea-read_orc_metadata

Conversation

@vuule

@vuule vuule commented Sep 29, 2022

Copy link
Copy Markdown
Contributor

Description

Issue #11675

Adds a C++ interface to get information about an ORC file. It is meant to be an efficient way to get information like column names and types, as well as file structure (e.g. number of stripes). The returned structure can be expanded to include more types of metadata, for now it only returns info that we found relevant internally.

The returned column hierarchy matches the one used in ORC (i.e. root struct column included), not the hierarchy of a cuDF dataframe that the file would be read as (root column children become top level cuDF columns).

This PR also includes improvements to ORC reader benchmarks, enabled by the new metadata API.

Checklist

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

@vuule vuule added feature request New feature or request non-breaking Non-breaking change labels Sep 29, 2022
@vuule vuule self-assigned this Sep 29, 2022
@github-actions github-actions Bot added conda libcudf Affects libcudf (C++/CUDA) code. labels Sep 29, 2022
@vuule

vuule commented Sep 29, 2022

Copy link
Copy Markdown
Contributor Author

Cool aside and an argument against num_rows - read_orc benchmark results :)
(with NROWS and STRIPES file is read in 8 equal-ish chunks)

| column_selection | row_selection | uses_index | uses_numpy_dtype | timestamp_type | Samples |  CPU Time  | Noise |  GPU Time  | Noise | bytes_per_second | peak_memory_usage | encoded_file_size |
|------------------|---------------|------------|------------------|----------------|---------|------------|-------|------------|-------|------------------|-------------------|-------------------|
|              ALL |           ALL |        YES |              YES |          EMPTY |     87x | 173.101 ms | 4.27% | 173.098 ms | 4.27% |       3101548373 |       771.392 MiB |       175.078 MiB |
|              ALL |       STRIPES |        YES |              YES |          EMPTY |     74x | 205.027 ms | 3.95% | 205.015 ms | 3.95% |       2618687346 |       104.208 MiB |       175.078 MiB |
|              ALL |         NROWS |        YES |              YES |          EMPTY |     26x | 581.968 ms | 1.59% | 581.961 ms | 1.59% |        922519967 |       117.808 MiB |       175.078 MiB |

@vuule
vuule marked this pull request as ready for review September 29, 2022 19:21
@vuule
vuule requested review from a team as code owners September 29, 2022 19:21

@hyperbolic2346 hyperbolic2346 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good to me, some minor nits.

cudf::rmm_pool_raii rmm_pool;

auto constexpr num_chunks = 1;
auto constexpr num_chunks = RowSelection == row_selection::ALL ? 1 : 8;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 8?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No specific reason, just a value that's high enough for any overhead from chunked reading to presumably show up.

Comment thread cpp/src/io/functions.cpp Outdated
Comment thread cpp/src/io/functions.cpp
Comment thread cpp/tests/io/orc_test.cpp Outdated
Comment thread cpp/tests/io/orc_test.cpp Outdated
Comment thread cpp/tests/io/orc_test.cpp Outdated
Comment thread cpp/tests/io/orc_test.cpp Outdated
@vuule vuule added 0 - Waiting on Author Waiting for author to respond to review cuIO cuIO issue labels Oct 3, 2022
vuule and others added 4 commits October 11, 2022 13:22
@vuule
vuule requested a review from hyperbolic2346 October 11, 2022 22:56

@nvdbaranec nvdbaranec left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple small comments

Comment thread cpp/benchmarks/io/orc/orc_reader_options.cpp Outdated
Comment thread cpp/src/io/functions.cpp Outdated

@hyperbolic2346 hyperbolic2346 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiny nit, but otherwise this looks great to me.

Comment thread cpp/src/io/functions.cpp Outdated
Comment thread cpp/tests/io/orc_test.cpp Outdated
Comment thread cpp/include/cudf/io/orc_metadata.hpp
@vuule

vuule commented Oct 15, 2022

Copy link
Copy Markdown
Contributor Author

Benchmarked the API. Takes between 60 and 250 microseconds for 512MB dataframes, and 25 to 150 for a 512KB dataframes. Reading the same 512MB files takes 100-200 milliseconds, so the perf benefit is definitely there :)

@codecov

codecov Bot commented Oct 15, 2022

Copy link
Copy Markdown

Codecov Report

Base: 87.40% // Head: 88.13% // Increases project coverage by +0.72% 🎉

Coverage data is based on head (bba422a) compared to base (f72c4ce).
Patch has no changes to coverable lines.

Additional details and impacted files
@@               Coverage Diff                @@
##           branch-22.12   #11815      +/-   ##
================================================
+ Coverage         87.40%   88.13%   +0.72%     
================================================
  Files               133      133              
  Lines             21833    21982     +149     
================================================
+ Hits              19084    19373     +289     
+ Misses             2749     2609     -140     
Impacted Files Coverage Δ
python/strings_udf/strings_udf/__init__.py 86.27% <0.00%> (-10.61%) ⬇️
python/cudf/cudf/io/text.py 91.66% <0.00%> (-8.34%) ⬇️
python/cudf/cudf/core/_base_index.py 81.28% <0.00%> (-4.27%) ⬇️
python/strings_udf/strings_udf/_typing.py 94.73% <0.00%> (-1.06%) ⬇️
python/cudf/cudf/utils/utils.py 89.91% <0.00%> (-0.69%) ⬇️
python/cudf/cudf/testing/dataset_generator.py 72.83% <0.00%> (-0.42%) ⬇️
python/dask_cudf/dask_cudf/core.py 73.72% <0.00%> (-0.41%) ⬇️
python/dask_cudf/dask_cudf/backends.py 84.90% <0.00%> (-0.37%) ⬇️
python/cudf/cudf/core/dataframe.py 93.67% <0.00%> (-0.10%) ⬇️
python/cudf/cudf/io/orc.py 92.94% <0.00%> (-0.09%) ⬇️
... and 24 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@vuule
vuule requested a review from nvdbaranec October 17, 2022 03:47

@hyperbolic2346 hyperbolic2346 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing the transform. It is informative. I would consider it a wash at best overall, but it is the preferred style in cudf.

I like the performance wins here, good work!

@ajschmidt8 ajschmidt8 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving ops-codeowner file changes

@vuule vuule added 4 - Needs cuIO Reviewer and removed 0 - Waiting on Author Waiting for author to respond to review labels Oct 21, 2022

@nvdbaranec nvdbaranec left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment.

Comment thread cpp/benchmarks/io/orc/orc_reader_options.cpp Outdated
@vuule
vuule requested a review from nvdbaranec October 31, 2022 21:18
@vuule

vuule commented Nov 1, 2022

Copy link
Copy Markdown
Contributor Author

@gpucibot merge

@rapids-bot
rapids-bot Bot merged commit 41fca6e into NVIDIA:branch-22.12 Nov 1, 2022
@vuule
vuule deleted the fea-read_orc_metadata branch November 1, 2022 17:34
@vyasr vyasr added 4 - Needs Review Waiting for reviewer to review or respond and removed 4 - Needs cuIO Reviewer labels Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4 - Needs Review Waiting for reviewer to review or respond cuIO cuIO issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants