• 🔍 Trigger a full review

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.

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

[PR Coverage check]

😍 pass : 231 / 372 (62.10%)

file detail

⚠️ Enterprise CI Failed

The enterprise test suite failed for this PR.

Build: View Details
Tested Commit: b8ae724abff6458af0ecb0d253380ce37b785085

Please investigate the failure before merging.

@CodeRabbit pls review this PR critically

#!/bin/bash
# Get the PR diff to understand the changes
gh pr diff 6715 --repo questdb/questdb > pr_diff.txt
head -2000 pr_diff.txt

#!/bin/bash
# Get the list of changed files
gh pr view 6715 --repo questdb/questdb --json files --jq '.files[].path'

#!/bin/bash
# Check for test files related to parquet
fd -e rs -e java test | rg -i parquet

#!/bin/bash
# Look at the Enterprise CI failure details
gh pr checks 6715 --repo questdb/questdb

#!/bin/bash
# Get more details from the PR diff
tail -2000 pr_diff.txt

@bluestreak01, I've performed a critical review of this PR. Here are my findings:

🚨 Critical Issues

1. Debug Statement in Production Code

Line 1273 in file.rs contains a println! that should be removed:

println!(
    "column_chunk_to_pages_with_dict_state: column={}, chunk_offset={}, include_dict={}",
    column.name, chunk_offset, include_dict
);

This will pollute production logs and should either be removed or converted to proper logging using a logging framework.

2. CI Failures Need Investigation

• Enterprise CI failed - This is blocking and needs investigation
• Rust Test and Lint failed on linux-jdk17 (8m20s) - Check what tests are failing

3. Low Test Coverage on Critical Code

• file.rs: 53.47% coverage (154/288 lines) - This file contains the core fix
• Os.java: 0% coverage (debug print line)
• lib.rs: 0% coverage (trivial refactor)

The new functions like compute_max_symbol_keys, build_symbol_dict_for_max_key, and compute_row_group_dicts_for_slice need better test coverage.

⚠️ Design & Performance Concerns

4. Memory Impact: Dictionary Cloning

In symbol.rs line 336:

let dict_page = Page::Dict(DictPage::new(
    info.dict_buffer.clone(),  // <-- Full dictionary buffer clone
    info.num_values,
    false,
));

For high-cardinality symbol columns, this could allocate significant memory. Consider whether this clone is necessary or if you can work with references.

5. Complex State Management

The solution introduces:

• SymbolDictInfo struct with Arc wrapping
• MultiPartitionColumnIteratorWithDict (renamed from MultiPartitionColumnIterator)
• Multiple new helper functions with _with_dict_state suffixes

This complexity increases the risk of bugs in edge cases. Have you considered:

• What happens with empty partitions?
• Symbol columns with no values (all nulls)?
• Symbol columns where max_key is at boundary conditions?

6. Error Path Coverage

build_symbol_dict_for_max_key has multiple error conditions:

"Symbol key {} has invalid UTF-16 length"
"Symbol key {} has offset {} outside chars buffer"
"Symbol key {} is outside offsets buffer"

Are these error paths tested? What happens in production if corrupted data triggers these?

7. Unsafe Code in Hot Path

Line 1152 in file.rs:

let k = unsafe { *keys.get_unchecked(i) };

While this may be an optimization, ensure bounds are guaranteed by the loop.

📝 Code Quality Issues

8. Unnecessary Change

In lib.rs, the refactor from:

let result = a + b;
result

to:

a + b

is style-only and adds noise to the PR.

9. Debug Print in Os.java

Line 394:

System.err.println("Loading DEV Rust library: " + devRustLib);

Should this be permanent or was this for debugging?

🧪 Testing Gaps

10. Edge Cases to Verify

The new tests cover basic scenarios, but consider adding tests for:

• Empty dictionaries (all NULL symbols)
• Single-value dictionaries (edge case for bit width calculation)
• Dictionary with gaps (missing symbol keys in the middle)
• Maximum key at boundary (e.g., exactly at power of 2)
• Very high cardinality (1M+ unique symbols)
• Multi-partition row groups with different symbol distributions

Questions

1. What is causing the Enterprise CI failure? Can you share the specific test failures?
2. Why is the file size changing in existing tests? (e.g., 51882 → 51540 bytes) - Is this expected compression improvement?
3. Have you validated with PyArrow specifically? The bug report mentions PyArrow fails - do you have a reproduction that now passes?
4. Memory profiling: Have you tested memory usage with high-cardinality symbol columns (e.g., 100K unique symbols)?

Recommendations

1. ✅ Remove the debug println! statement
2. ✅ Remove or properly guard the debug System.err.println
3. ✅ Investigate and fix CI failures before merge
4. ✅ Add tests for edge cases mentioned above
5. ✅ Consider adding benchmarks for memory usage with high-cardinality symbols
6. ⚠️ Document the new dictionary management strategy in code comments
7. ⚠️ Consider adding assertions to verify Parquet spec compliance (one dict per column chunk)

The core fix appears sound, but these issues should be addressed before merging to production.