[BUG] Fix ORC chunked writer root row statistics - #23118
Conversation
|
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:
📝 WalkthroughWalkthroughUpdates ORC persisted statistics to accumulate row counts across chunked writes, widens row-count types to ChangesORC chunked statistics row accumulation
Estimated code review effort: 2 (Simple) | ~10 minutes 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: 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/src/io/orc/writer_impl.cu`:
- Line 398: The row-count accumulator in persisted_statistics is still using an
int, so chunked writes in writer_impl.cu can overflow across multiple write()
calls and produce an incorrect footer row count. Update
persisted_statistics::num_rows and the footer serialization path to use a 64-bit
type, and make sure the num_rows += num_table_rows accumulation in the write
flow and any related getter/setter or serialization code in writer_impl.cu
consistently use the widened type.
🪄 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: 1ee9c44a-788c-4a41-a642-1d465e484c69
📒 Files selected for processing (2)
cpp/src/io/orc/writer_impl.cucpp/tests/io/orc_test.cpp
There was a problem hiding this comment.
Pull request overview
Fixes ORC root column statistics for chunked ORC writes so the file-level root numberOfValues reflects the total number of rows across multiple non-empty orc_chunked_writer::write() calls (rather than only the final table), which affects downstream aggregate pushdown behavior.
Changes:
- Accumulate
persisted_statistics::num_rowsacross chunked writes instead of overwriting it per write. - Add a regression test validating root file statistics row-count accumulation for multiple writes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cpp/src/io/orc/writer_impl.cu | Accumulates persisted root row count across multiple write() calls to produce correct root footer statistics. |
| cpp/tests/io/orc_test.cpp | Adds a regression test asserting root number_of_values equals the sum of rows written across multiple chunk writes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/ok to test 8089674 |
|
/ok to test 9887cf3 |
| } | ||
|
|
||
| void persist(int num_table_rows, | ||
| void persist(uint64_t num_table_rows, |
There was a problem hiding this comment.
uint64_t is now used directly by this header, but <cstdint> is only available through a transitive include. Please add its declaring header to the standard-library include group, as required by the project's include-what-you-use rule:
+#include <cstdint>
#include <memory>| stats_dtypes = std::move(intermediate_stats.stats_dtypes); | ||
| col_types = std::move(intermediate_stats.col_types); | ||
| num_rows = num_table_rows; | ||
| if (num_rows == 0) { return; } | ||
| num_rows += num_table_rows; | ||
| if (num_table_rows == 0) { return; } |
There was a problem hiding this comment.
An empty chunk has an empty intermediate_stats.stats_dtypes, so assigning it before this early return discards the descriptors from prior non-empty chunks. close() later indexes the empty vector while merging the already-persisted stripe statistics. Please preserve the last non-empty descriptors by moving that assignment after the zero-row check, while still updating col_types for an all-empty file:
| stats_dtypes = std::move(intermediate_stats.stats_dtypes); | |
| col_types = std::move(intermediate_stats.col_types); | |
| num_rows = num_table_rows; | |
| if (num_rows == 0) { return; } | |
| num_rows += num_table_rows; | |
| if (num_table_rows == 0) { return; } | |
| col_types = std::move(intermediate_stats.col_types); | |
| num_rows += num_table_rows; | |
| if (num_table_rows == 0) { return; } | |
| stats_dtypes = std::move(intermediate_stats.stats_dtypes); |
Please also add a regression case that writes a non-empty table followed by an empty table, closes the writer, and parses the resulting statistics.
| auto const stats = cudf::io::read_parsed_orc_statistics(cudf::io::source_info{filepath}); | ||
| ASSERT_FALSE(stats.file_stats.empty()); | ||
| ASSERT_TRUE(stats.file_stats.front().number_of_values.has_value()); | ||
| EXPECT_EQ(*stats.file_stats.front().number_of_values, 6); |
There was a problem hiding this comment.
This six-row assertion detects replacement with the final chunk's count, but it cannot detect regression of the widened accumulator to 32 bits. The existing SizeTypeRowsOverflow test already writes 2,500,000,000 rows; please extend that test after its metadata assertions to verify the root statistic produced by the accumulator:
+ auto const stats =
+ cudf::io::read_parsed_orc_statistics(cudf::io::source_info{cudf::host_span<std::byte const>{
+ reinterpret_cast<std::byte const*>(out_buffer.data()), out_buffer.size()}});
+ ASSERT_FALSE(stats.file_stats.empty());
+ ASSERT_TRUE(stats.file_stats.front().number_of_values.has_value());
+ EXPECT_EQ(*stats.file_stats.front().number_of_values, static_cast<uint64_t>(total_rows));
|
/ok to test 3e8418f |
vuule
left a comment
vuule
left a comment
There was a problem hiding this comment.
one non-blocking comment, good otherwise.
| } | ||
|
|
||
| void persisted_statistics::persist(int num_table_rows, | ||
| void persisted_statistics::persist(uint64_t num_table_rows, |
There was a problem hiding this comment.
This change was not necessary because we pass a value previously stored in size_type.
There was a problem hiding this comment.
Any hardening against ~2B row limits is fine with me.
|
/ok to test 4d5d9b0 |
Signed-off-by: Allen Xu <allxu@nvidia.com>
Signed-off-by: Allen Xu <allxu@nvidia.com>
Signed-off-by: Allen Xu <allxu@nvidia.com>
Signed-off-by: Allen Xu <allxu@nvidia.com>
4d5d9b0 to
fa88fb5
Compare
mhaseeb123
added
4 - Needs Review
|
/ok to test fa88fb5 |
|
/merge |
mhaseeb123
added
5 - Ready to Merge
f03e9cd
into
NVIDIA:release/26.08
Description
Fix ORC root column statistics when an
orc_chunked_writerreceives multiple non-empty tables.persisted_statistics::persistreplacednum_rowson every write, so the rootnumberOfValuesstored in the footer reflected only the final table. Accumulate the row count across writes instead, while retaining the existing early return for an empty current table.This caused Spark ORC aggregate pushdown to return the number of output files instead of the row count for
COUNTon a top-level struct written by the RAPIDS Accelerator. Related issue: NVIDIA/cudf-spark#15186.Validation:
OrcChunkedWriterTest.RootStatisticsAccumulateRowsfailed with rootnumber_of_values = 1, expected6.ORC_TEST:197 tests from 36 test suites,197 passed,4 disabled.Checklist