Improve Parquet case-insensitive column selection semantics - #22729
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
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:
📝 WalkthroughWalkthroughAdds case-sensitivity-aware column-path utilities and factories, replaces ad-hoc normalization with transparent container-based maps/sets in Parquet expression and hybrid-scan code, wires the new source into the build, and adds an NVBench verifying AST filter name-to-column resolution. ChangesCase-Sensitive Column Path Utilities and Reader Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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)
Comment |
PointKernel
left a comment
There was a problem hiding this comment.
Very clean code. Only some non-blocking nits.
Thanks!
bfb83c0 to
a96800e
Compare
d2993e8 to
c3ac600
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/src/io/parquet/column_path_helpers.cpp (1)
32-35: ⚡ Quick winSimplify the comparison lambda for clarity.
The
std::equal_to<>{}call is unnecessarily verbose. Direct==comparison is more idiomatic and clearer.♻️ Simplified comparison
return std::equal( lhs.begin(), lhs.end(), rhs.begin(), [](unsigned char lhs_char, unsigned char rhs_char) { - return std::equal_to<>{}(std::tolower(lhs_char), std::tolower(rhs_char)); + return std::tolower(lhs_char) == std::tolower(rhs_char); });🤖 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/column_path_helpers.cpp` around lines 32 - 35, The lambda inside the std::equal call can be simplified: replace the verbose std::equal_to<>{}(std::tolower(lhs_char), std::tolower(rhs_char)) expression in the lambda (the one taking lhs_char and rhs_char) with a direct == comparison of the two std::tolower results; update the lambda used in the std::equal(...) invocation in column_path_helpers.cpp accordingly for clarity.
🤖 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.
Nitpick comments:
In `@cpp/src/io/parquet/column_path_helpers.cpp`:
- Around line 32-35: The lambda inside the std::equal call can be simplified:
replace the verbose std::equal_to<>{}(std::tolower(lhs_char),
std::tolower(rhs_char)) expression in the lambda (the one taking lhs_char and
rhs_char) with a direct == comparison of the two std::tolower results; update
the lambda used in the std::equal(...) invocation in column_path_helpers.cpp
accordingly for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4d0880da-19dd-4dcb-ba6d-e4499b47a5a5
📒 Files selected for processing (9)
cpp/CMakeLists.txtcpp/benchmarks/io/parquet/parquet_reader_metadata.cppcpp/src/io/parquet/column_path_helpers.cppcpp/src/io/parquet/column_path_helpers.hppcpp/src/io/parquet/experimental/hybrid_scan_helpers.cppcpp/src/io/parquet/expression_transform_helpers.cppcpp/src/io/parquet/expression_transform_helpers.hppcpp/src/io/parquet/reader_impl_helpers.cppcpp/src/io/parquet/reader_impl_helpers.hpp
🚧 Files skipped from review as they are similar to previous changes (5)
- cpp/CMakeLists.txt
- cpp/src/io/parquet/expression_transform_helpers.cpp
- cpp/src/io/parquet/column_path_helpers.hpp
- cpp/src/io/parquet/experimental/hybrid_scan_helpers.cpp
- cpp/src/io/parquet/reader_impl_helpers.hpp
…in multiple files. Local complie pass, now trigger CI
c3ac600 to
c336e0a
Compare
|
/merge |


Description
Close #21864
Follow-up to #21700. That PR repeated
normalize_column_path(...)at every insertion/lookup of the column-name hash containers. As suggested, this PR encapsulates that policy in a small pair of hash/equality functors that carry thecase_sensitive_namespolicy.Checklist