Copilot commented on code in PR #50233:
URL: https://github.com/apache/arrow/pull/50233#discussion_r3449038421
##########
cpp/src/parquet/size_statistics_test.cc:
##########
@@ -60,6 +60,12 @@ TEST(SizeStatistics, UpdateLevelHistogram) {
UpdateLevelHistogram(std::vector<int16_t>{}, histogram);
EXPECT_THAT(histogram, ::testing::ElementsAre(3, 3, 2));
}
+ {
+ // Empty levels / null span
+ std::vector<int64_t> histogram(2, 0);
+ UpdateLevelHistogram(std::span<const int16_t>{}, histogram);
+ EXPECT_THAT(histogram, ::testing::ElementsAre(0, 0));
+ }
Review Comment:
This regression block exercises `parquet::UpdateLevelHistogram` with an
empty `std::span`, but it doesn’t cover the UB/crash scenario fixed in
`TypedColumnWriterImpl::UpdateLevelHistogram` (construction of `std::span` from
raw pointers). To guard against regressions, add a test that runs through the
column writer path with size statistics enabled and passes `rep_levels ==
nullptr` / `def_levels == nullptr` (e.g. required/non-repeated column) to
ensure no crash.
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -1809,7 +1809,8 @@ class TypedColumnWriterImpl : public ColumnWriterImpl,
auto add_levels = [](std::vector<int64_t>& level_histogram, const int16_t*
levels,
int64_t num_levels, int16_t max_level) {
- if (max_level == 0) {
+ ARROW_DCHECK(levels != nullptr || max_level == 0);
+ if (max_level == 0 || levels == nullptr) {
return;
}
Review Comment:
`ARROW_DCHECK(levels != nullptr || max_level == 0)` will DCHECK-fail for
empty writes where `num_levels == 0` and callers legitimately pass `levels ==
nullptr` even when `max_level > 0` (no data to read, and the early-return
already avoids span construction). Consider allowing the empty-batch case in
the DCHECK so debug builds don’t abort on a no-op update.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
