Write variable bit-width keys for Parquet dictionary encoded pages - #22279
Conversation
Repurpose the unused mapped_type slot in populate_chunk_hash_maps_kernel to carry the fragment index of the block that first inserts each value. Rewrite collect_map_entries_kernel to bucket dict_ids by that fragment index so pages that only reference earlier fragments' values see small max dict_index. No file-size delta in isolation; prerequisite for the per-page bit-width change landing next. Made-with: Cursor
…#13995) Each data page now RLE-encodes dictionary indices using ceil(log2(page_max_dict_index + 1)) bits instead of the chunk-wide maximum. Combined with the first-appearance dict_id ordering from the prior commit, this closes the ~30% file-size gap vs Spark on moderate-cardinality INT64 and STRING workloads. Page size estimation continues to use the chunk-wide bits as a conservative upper bound; the dictionary page itself is unaffected. Made-with: Cursor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR extends the cuDF Parquet writer to compute per-page dictionary RLE bit widths after page boundaries are finalized, replacing chunk-level dictionary settings. It widens fragment structures, introduces GPU kernels for per-page bit-width calculation, and updates writer orchestration to call the new compute function with finalized pages. Tests validate the per-page behavior across variable-distribution scenarios. ChangesPer-Page Dictionary RLE Bit-Width Computation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cpp/src/io/parquet/chunk_dict.cu (1)
136-143:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe fragment hint is still nondeterministic.
This records the
frag_idxof whichever block wins the insert, not the earliest fragment that observed the value. On out-of-order block scheduling, the same input can still receive different dictionary IDs and therefore different per-page bit widths/file sizes across runs, so the new exact page-bit assertions remain flaky until first-fragment stamping is deterministic.🤖 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/chunk_dict.cu` around lines 136 - 143, The current insertion uses map_insert_ref.insert(slot_type{static_cast<key_type>(val_idx), frag_idx}) which records whichever block wins the race, producing nondeterministic frag_idx; change this to use the static_map's insert_or_apply API with cuco::op::min so the stored value for key_type(val_idx) is always the minimum frag_idx seen (i.e., first fragment). Locate the insert call in chunk_dict.cu (map_insert_ref.insert and the surrounding uniq_elem_size lambda) and replace the insert with insert_or_apply passing a min operation (cuco::op::min) so ties resolve deterministically to the earliest fragment index.
🤖 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/writer_impl.cu`:
- Around line 2125-2127: compute_per_page_dict_bits is being called using stale
fragment-local dictionary IDs assigned earlier from row_group_fragments, so when
the second pass mutates ck.fragments/ck.num_fragments the per-page bit widths no
longer match the actual fragments that formed the pages; to fix this, ensure
compute_per_page_dict_bits is invoked with page/fragment info reflecting the
post-pass fragment ordering — either recompute fragment-local dictionary IDs
(re-run the row_group_fragments assignment) or rebuild the pages buffer from the
updated ck.fragments/ck.num_fragments before calling compute_per_page_dict_bits,
and move this call to after any mutation of ck.fragments/ck.num_fragments so the
bit-widths are computed against the final fragment layout.
---
Duplicate comments:
In `@cpp/src/io/parquet/chunk_dict.cu`:
- Around line 136-143: The current insertion uses
map_insert_ref.insert(slot_type{static_cast<key_type>(val_idx), frag_idx}) which
records whichever block wins the race, producing nondeterministic frag_idx;
change this to use the static_map's insert_or_apply API with cuco::op::min so
the stored value for key_type(val_idx) is always the minimum frag_idx seen
(i.e., first fragment). Locate the insert call in chunk_dict.cu
(map_insert_ref.insert and the surrounding uniq_elem_size lambda) and replace
the insert with insert_or_apply passing a min operation (cuco::op::min) so ties
resolve deterministically to the earliest fragment index.
🪄 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: 49a19954-7286-4c84-bc55-adbbef540835
📒 Files selected for processing (9)
cpp/benchmarks/CMakeLists.txtcpp/benchmarks/io/parquet/parquet_writer_dict.cppcpp/src/io/parquet/chunk_dict.cucpp/src/io/parquet/page_enc.cucpp/src/io/parquet/parquet_gpu.cuhcpp/src/io/parquet/parquet_gpu.hppcpp/src/io/parquet/writer_impl.cucpp/tests/io/parquet_misc_test.cppcpp/tests/io/parquet_writer_test.cpp
|
/ok to test 1ceaaef |
|
/ok to test d2b1028 |
|
/ok to test e84a14f |
|
/merge |
Description
Contributes to #13995
This PR enables the Parquet writer to assign lower dictionary indices to elements appearing earlier in each column chunk thereby writing variable (reduced) number of dictionary key bits for earlier pages.
New algorithm:
map_insert_fnnow inserts this pair in the static map:{row_idx, frag_idx}instead of{row_idx, row_idx}.frag_idxis justblockIdx.xand indicates which page fragment actually inserted this entry in the static map (CAS race dependent but earlier thread blocks generally win this race)map_insert_fnalso writes thePageFragment::num_dict_valsfield for each page fragment to keep a record of number of unique values inserted by it.collect_map_entries_kernel(where we assign keys to dict values), we optionally assign spatially-local keys to dictionary values inserted by the same page fragment. See algorithm details herecompute_page_dict_bits_kernelto compute the max bit width required to encode dict keys for each parquet page. See algorithm hereSee follow up PR #22323 that deterministically writes the index of the first page fragment that sees each dictionary key
Checklist