feat(core): add RLE dictionary encoding support for STRING columns in Parquet

[RaphDal]

Summary

• Add RLE dictionary decoding for STRING columns in the Parquet reader by wiring the existing BaseVarDictDecoder + RleDictionarySlicer + StringColumnSink together in decode_byte_array_dispatch
• Change the default writer encoding for VARCHAR columns from RleDictionary to DeltaLengthByteArray, aligning it with STRING and Binary defaults
• Add string_dict benchmark cases to the decode_page benchmark suite

Test plan

• Add test_string_rle_dictionary decode test covering all writer versions and null patterns (no nulls, sparse, dense), both full and filtered decode
• Verify all existing schema validation tests pass with the updated default encoding
• Run full cargo test suite (680 tests pass)
• Verify code coverage with cargo llvm-cov — new match arm fully covered (12 executions across monomorphizations)
• Benchmark string_dict_c{10,100,256,1000}_n{0,20} shows 49–73 Melem/s throughput

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9fc5b6bb-d01d-40c9-8457-ed607cb6e20e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch rd_parquet_string_rle

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[bluestreak01]

@RaphDal

PR #6909 Review: feat(core): add RLE dictionary encoding support for STRING columns in Parquet

Critical

1. Multi-partition Parquet export fallback is a no-op for STRING columns

core/rust/qdbr/src/parquet_write/file.rs:499-507:

// non-Symbol dict columns would emit multiple DictPages per column chunk
// (invalid Parquet). Fall back to the type's default encoding.
if num_partitions > 1
    && col_encoding == Encoding::RleDictionary
    && !first_partition_column.data_type.is_symbol()
{
    col_encoding = super::schema::encoding_map(first_partition_column.data_type);
}

Since encoding_map(String) now returns RleDictionary, this fallback reassigns RleDictionary back to itself — a no-op. The comment explicitly states this produces invalid Parquet (multiple DictPages per column chunk). Multi-partition Parquet exports with STRING columns will now use dictionary encoding without the intended fallback to a non-dictionary encoding.

Nuance: VARCHAR also defaults to RleDictionary and has the same no-op fallback, so this is a pre-existing pattern. However, this PR extends it to STRING columns, increasing the blast radius. The fix should change the fallback to use a specific non-dictionary encoding (e.g., Encoding::DeltaLengthByteArray) rather than encoding_map():

col_encoding = match first_partition_column.data_type.tag() {
    ColumnTypeTag::String => Encoding::DeltaLengthByteArray,
    ColumnTypeTag::Varchar => Encoding::DeltaLengthByteArray,
    _ => Encoding::Plain,
};

---

Moderate

2. File size regression with no justification

The encoding default change from DeltaLengthByteArray to RleDictionary increases Parquet file sizes across the board:

Test case	Before	After	Increase
basic_parquet	1,231 B	1,251 B	+1.6%
json_conn_table	1,683 B	1,787 B	+6.2%
bloom_single_test	81,711 B	99,354 B	+21.6%
large_export	927,480 B	1,026,961 B	+10.7%

RleDictionary is typically larger than DeltaLengthByteArray for high-cardinality string data (many unique values) because it stores a full dictionary + indices. The PR description only mentions throughput benchmarks (49–73 Melem/s) but does not address the file size tradeoff or explain why the regression is acceptable. The PR should either:

• Justify the tradeoff (e.g., decode speed gain vs. file size cost)
• Keep DeltaLengthByteArray as the default and only add reader support for RleDictionary (to read files from other tools)

3. UTF-8 decode errors silently swallowed (pre-existing, not blocking)

core/rust/qdbr/src/parquet_read/column_sink/var.rs:106-110 — StringColumnSink stores UTF-8 errors in a private error field (line 155), but no code ever reads it. The Pushable trait has no result() method. Invalid UTF-8 strings are silently converted to NULLs. This is pre-existing (affects VARCHAR too) and not introduced by this PR, but the PR copies the pattern. Consider filing a follow-up issue.

4. PlainDictionary encoding not tested

The new match arm at decode.rs:1194-1195 handles both Encoding::RleDictionary | Encoding::PlainDictionary, but only RleDictionary is tested. PlainDictionary is a legacy Parquet v1 encoding that external tools may produce. A test should be added for Encoding::PlainDictionary with STRING columns.

---

Minor

5. PR title focuses on implementation details

Per CLAUDE.md: "PR title descriptions must speak to the end-user about the positive impact, not about internal implementation details." The current title describes the implementation. A better title would focus on the user impact, e.g., feat(core): add Parquet import support for dictionary-encoded STRING columns.

6. Missing edge case tests

The decode test covers version variants, null patterns (none/sparse/dense), and filtered decode. However, several edge cases are not tested:

• Empty strings (all test strings are 7+ characters)
• All-NULL columns (only 0%, 10%, 50% null tested)
• Single-row tables (all tests use COUNT=1000)
• Cardinality=1 (all identical strings)

---

Downgraded (false positives from initial review)

Original claim	Why dismissed
Parameter order mismatch in RleDictionarySlicer::try_new	row_hi = mode.source_row_count() maps to param row_count (3rd) and row_count = mode.sliced_row_count() maps to param sliced_row_count (4th) — correct order, just confusing variable names.
Empty buffer panic in RleDictionarySlicer::try_new	Pre-existing in the VARCHAR path. Parquet RLE spec mandates a bit-width byte; only reachable with corrupt files.
Integer overflow in StringColumnSink::reserve()	Requires count > usize::MAX / 8, impossible in practice since row counts are bounded by JNI i32 parameters.

---

Summary

The core reader change (adding the StringColumnSink + RleDictionarySlicer match arm) is correct and properly mirrors the existing VARCHAR path. However, the multi-partition fallback no-op (Critical #1) can produce invalid Parquet files for STRING columns in multi-partition exports, and the file size regression (Moderate #2) needs justification.

Review stats: 9 draft findings verified, 3 downgraded as false positives.

[glasstiger]

[PR Coverage check]

😍 pass : 77 / 102 (75.49%)

file detail

	path	covered line	new line	coverage
🔵	qdbr/src/parquet_read/column_sink/var.rs	1	2	50.00%
🔵	qdbr/src/parquet_read/decode.rs	47	70	67.14%
🔵	qdbr/tests/common/encode.rs	4	5	80.00%
🔵	qdbr/tests/encode_symbol.rs	2	2	100.00%
🔵	qdbr/tests/encode_primitives.rs	2	2	100.00%
🔵	qdbr/benches/decode_page.rs	13	13	100.00%
🔵	qdbr/tests/decode_strings.rs	3	3	100.00%
🔵	qdbr/src/parquet_read/decoders/dictionary.rs	1	1	100.00%
🔵	qdbr/src/parquet_write/schema.rs	1	1	100.00%
🔵	qdbr/tests/encode_decimal.rs	1	1	100.00%
🔵	qdbr/tests/encode_array.rs	2	2	100.00%