Transcode parquet to cuDF dictionaries for flat STRING columns - #22532
Merged
Conversation
github-actions
Bot
added
libcudf
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds optional Parquet reader support for returning fully dictionary-encoded flat string columns as ChangesParquet Dictionary Transcode Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
cpp/src/io/parquet/reader_impl_dict_transcode.cu (1)
175-185: ⚡ Quick winDrop the blocking stream sync here.
Line 185 forces a full
_stream.synchronize()even though the rewrittensubpass.pages.host_to_device_async(_stream)is already ordered before later decode work. That adds a host-side barrier to the transcode path without obvious correctness benefit.As per coding guidelines, "Avoid unnecessary host-device synchronization that blocks the GPU pipeline."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/io/parquet/reader_impl_dict_transcode.cu` around lines 175 - 185, Remove the blocking host-device sync: delete the call to _stream.synchronize() after subpass.pages.host_to_device_async(_stream) and after updating subpass.kernel_mask via std::transform_reduce; rely on the existing host_to_device_async ordering for correctness and add a short comment next to subpass.pages.host_to_device_async(_stream) explaining that no explicit host-side synchronization is required (keeps GPU pipeline unblocked). Ensure references: subpass.pages.host_to_device_async, subpass.kernel_mask, std::transform_reduce, and _stream.synchronize() are the locations you change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/io/parquet/reader_impl_dict_transcode.cu`:
- Around line 148-173: The code flips output buffer types via
_output_buffers[_input_columns[i].nesting[0]].type and sets
_dict_transcode_eligible before verifying any_rewritten; move the entire
std::for_each that changes out_buf.type (and any mutation of
_dict_transcode_eligible if present) to occur only after the host-page rewrite
succeeds (i.e., after the any_rewritten check and early return), so that you
only retag buffers to INT32 when pages were actually updated from
decode_kernel_mask::STRING_DICT to decode_kernel_mask::DICT_INT32;
alternatively, if you prefer the current order, roll back the type and
eligibility mutations before returning when any_rewritten is false.
In `@cpp/src/io/parquet/reader_impl.cpp`:
- Around line 923-937: The current logic eagerly calls
dictionary::detail::encode on any remaining top-level STRING columns when
_options.try_output_dict_columns is true (iterating over out_columns), which can
convert columns to DICTIONARY32 before predicate evaluation and break AST
filters that don't support dictionary columns; fix by deferring this fallback
encode until after the filter/predicate branch completes (i.e., move the loop
that encodes STRING columns to DICTIONARY32 to run after the predicate
evaluation/filter logic that lives around the predicate handling block), or
alternatively explicitly reject try_output_dict_columns when
options.get_filter() is set by adding a validation check on options.get_filter()
that returns an error if try_output_dict_columns is requested with a filter
present; update call sites involving _options.try_output_dict_columns,
out_columns, dictionary::detail::encode, and options.get_filter() accordingly.
In `@cpp/tests/io/parquet_reader_dict_test.cpp`:
- Around line 8-11: Add the test framework header include
<cudf_test/cudf_gtest.hpp> to the top of the test file (alongside the existing
includes) so the file uses the repository test convention instead of including
gtest directly; update the include list near the existing includes
(cudf_test/base_fixture.hpp, cudf_test/column_utilities.hpp,
cudf_test/column_wrapper.hpp) to add cudf_test/cudf_gtest.hpp.
- Around line 35-76: The tests only generate ASCII strings; update the
generators make_low_cardinality_strings() and
make_low_cardinality_lists_of_strings() to include non-ASCII UTF-8 samples
(e.g., accented characters, CJK, emoji) by building a small pool of values that
mixes "str_<n>" with UTF-8 literals and picking from that pool via value_dist
(use the existing seed and list_strings_seed for determinism); ensure both the
top-level strings vector and the child_strings used for lists sometimes select
those UTF-8 entries so transcoding/fallback paths are exercised while preserving
null logic and list offsets.
- Around line 112-177: Add two edge-case tests for the DICTIONARY32 transcode
path: (1) an empty-input test modeled on
ParquetReaderDictTest::FlatStringDictTranscode that constructs an empty column
(zero rows) from the same low-cardinality string generator, writes/reads it via
write_parquet and read_parquet_as_dict, and asserts read_table->num_rows()==0,
read column type is DICTIONARY32, and CUDF_TEST_EXPECT_COLUMNS_EQUAL compares
the decoded empty input and decoded read column; (2) a sliced-column test that
creates a sliced view of make_low_cardinality_strings (use column_view slicing
or cudf::slice), writes that sliced input, reads with read_parquet_as_dict, and
assert num_rows/num_columns, read column type==cudf::type_id::DICTIONARY32 and
use cudf::dictionary::encode/decode + CUDF_TEST_EXPECT_COLUMNS_EQUAL to verify
equality of the sliced original and decoded read column; locate code around
ParquetReaderDictTest::FlatStringDictTranscode, read_parquet_as_dict,
make_low_cardinality_strings, and CUDF_TEST_EXPECT_COLUMNS_EQUAL to implement
these tests.
- Around line 20-23: Add the missing header for std::min by including
<algorithm> in the top include block; locate the include list (where <memory>,
<random>, <string>, <vector> are included) in parquet_reader_dict_test.cpp and
add the <algorithm> include so uses of std::min (used around the code
referencing std::min at line ~89) are directly supported rather than relying on
transitive includes.
---
Nitpick comments:
In `@cpp/src/io/parquet/reader_impl_dict_transcode.cu`:
- Around line 175-185: Remove the blocking host-device sync: delete the call to
_stream.synchronize() after subpass.pages.host_to_device_async(_stream) and
after updating subpass.kernel_mask via std::transform_reduce; rely on the
existing host_to_device_async ordering for correctness and add a short comment
next to subpass.pages.host_to_device_async(_stream) explaining that no explicit
host-side synchronization is required (keeps GPU pipeline unblocked). Ensure
references: subpass.pages.host_to_device_async, subpass.kernel_mask,
std::transform_reduce, and _stream.synchronize() are the locations you change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 691612f6-fd44-4f0c-b37c-a4df33fc8bbb
📒 Files selected for processing (9)
cpp/CMakeLists.txtcpp/include/cudf/io/parquet.hppcpp/src/io/parquet/decode_fixed.cucpp/src/io/parquet/parquet_gpu.hppcpp/src/io/parquet/reader_impl.cppcpp/src/io/parquet/reader_impl.hppcpp/src/io/parquet/reader_impl_dict_transcode.cucpp/tests/CMakeLists.txtcpp/tests/io/parquet_reader_dict_test.cpp
mhaseeb123
self-requested a review
May 15, 2026 23:54
y2kiran
marked this pull request as draft
May 16, 2026 01:03
y2kiran
force-pushed
the
ykiran-pq-decode
branch
from
June 15, 2026 20:31
88a021a to
8c8d35e
Compare
y2kiran
force-pushed
the
ykiran-pq-decode
branch
from
June 24, 2026 16:49
8c8d35e to
a561787
Compare
PointKernel
added
feature request
Member
|
@y2kiran, could you please address all CodeRabbit review comments and mark the PR as ready for final review once everything is resolved? That will help us kick off the CI process at the right time. Thanks! |
y2kiran
force-pushed
the
ykiran-pq-decode
branch
from
June 29, 2026 20:30
4030a68 to
7b6833f
Compare
y2kiran
marked this pull request as ready for review
June 29, 2026 20:49
y2kiran
changed the title
Contributor
Author
|
@PointKernel Addressed all coderabbit feedback. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/benchmarks/io/parquet/parquet_reader_dict.cpp`:
- Around line 76-84: The sanity check in parquet_reader_dict.cpp only inspects
probe.tbl->view().column(0), so it can miss cases where other output columns
fall back to STRING and the benchmark measures a mixed path. Update the
validation in the try_dict block after cudf::io::read_parquet(read_opts) to
verify every column in probe.tbl matches the expected DICTIONARY32 type, not
just the first one. Use the existing num_cols check and the probe.tbl/view()
access pattern to iterate through all output columns and fail fast if any column
is not DICTIONARY32.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 22be6276-fd6d-4391-90b7-81612485179e
📒 Files selected for processing (3)
cpp/CMakeLists.txtcpp/benchmarks/CMakeLists.txtcpp/benchmarks/io/parquet/parquet_reader_dict.cpp
✅ Files skipped from review due to trivial changes (1)
- cpp/CMakeLists.txt
Member
|
/ok to test bde4dc8 |
Contributor
|
Hi @y2kiran, looks like we have failing cudf-cpp-tests related to this PR |
mhaseeb123
requested changes
Jun 30, 2026
Comment on lines
+86
to
+99
| /** | ||
| * @brief Fold a single chunk's properties into its column's eligibility state. | ||
| * | ||
| * @param e The per-column eligibility state to update in place | ||
| * @param chunk The column chunk descriptor to classify | ||
| */ | ||
| void update_from_chunk(column_eligibility& e, ColumnChunkDesc const& chunk) | ||
| { | ||
| e.has_any_chunk = true; | ||
| if (chunk.max_nesting_depth != 1 or chunk.max_level[level_type::REPETITION] != 0 or | ||
| not is_host_byte_array_string_chunk(chunk) or chunk.num_dict_pages < 1) { | ||
| e.all_chunks_string = false; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Confused here, why are we relying on information from one column chunks instead of checking all?
Contributor
There was a problem hiding this comment.
Also please note that in Parquet V2, it is possible for some pages in a column chunk to be dict encoded some not (multiple encodings per chunk) is allowed and totally legal.
Contributor
Author
There was a problem hiding this comment.
If I'm not mistaken, we are still processing every chunk here. And update all_chunks_string.
| // pre-shift indices into a global keyspace, because `cudf::dictionary::detail::concatenate` | ||
| // already re-maps the indices using `compute_children_offsets_fn`. Pre-shifting would cause | ||
| // double-offsetting and out-of-bounds reads in the `dispatch_compute_indices` kernel. | ||
| std::for_each( |
Contributor
There was a problem hiding this comment.
Since @davidwendt is working on #22839, we could just adjust the key indices and leave everything any cross chunk duplicates as is (no need to concatenate anything)
Contributor
|
@coderabbitai full review |
y2kiran
force-pushed
the
ykiran-pq-decode
branch
from
August 7, 2026 18:14
5cb1f8e to
a435cda
Compare
Member
|
/merge |
3 tasks
3 tasks
3 tasks
rapids-bot Bot
pushed a commit
that referenced
this pull request
Aug 21, 2026
This PR adds benchmarks to test the newly added `output_dict_columns` options for the Parquet reader, which was introduced in this [PR](#22532) Authors: - https://github.com/y2kiran Approvers: - Vukasin Milovanovic (https://github.com/vuule) - Muhammad Haseeb (https://github.com/mhaseeb123) - Lawrence Mitchell (https://github.com/wence-) URL: #23596
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds a prototype implementation of transcoding dictionaries from parquet to cuDF directly when possible and specified for flat string type columns.
It also adds new tests to check the new transcode path, as well as a benchmark to evaluate performance.
Checklist