Awesome06 commented on code in PR #55453:
URL: https://github.com/apache/arrow/pull/55453#discussion_r4533328657
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -2024,7 +2024,8 @@ Status
TypedColumnWriterImpl<ParquetType>::WriteArrowDictionary(
::arrow::compute::Unique(*chunk_indices,
&exec_ctx));
// On first run, we might be able to re-use the existing dictionary
- if (referenced_indices.length() == dictionary->length()) {
+ if (chunk_indices->null_count() == 0 &&
Review Comment:
`arrow::compute::Unique` includes null in its output. If a chunk has 1 valid
index and 1 null, but the dictionary has 2 values (`["a", "b"]`), `Unique`
returns 2 items (`[0, null]`).
Without `null_count() == 0`, `referenced_indices.length() ==
dictionary->length()` passed (2 == 2), so the writer reused the entire
dictionary and incorrectly added unreferenced values (like `"b"`) into min/max
statistics and Bloom filters.
This affected both V1 and V2 pages. I've added a regression test covering
both: `TestArrowWriteDictionary.StatisticsWithNullsMatchingDictLength`.
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -2064,12 +2065,12 @@ Status
TypedColumnWriterImpl<ParquetType>::WriteArrowDictionary(
AddIfNotNull(rep_levels, offset));
std::shared_ptr<Array> writeable_indices =
indices->Slice(value_offset, batch_num_spaced_values);
- if (page_statistics_ || bloom_filter_writer_) {
- update_stats(/*num_chunk_levels=*/batch_size, writeable_indices);
- }
PARQUET_ASSIGN_OR_THROW(
writeable_indices,
MaybeReplaceValidity(writeable_indices, null_count, ctx->memory_pool));
+ if (page_statistics_ || bloom_filter_writer_) {
+ update_stats(/*num_chunk_levels=*/batch_size, writeable_indices);
+ }
Review Comment:
For nested dictionary columns (e.g. `struct<inner: dictionary not null>
nullable`), parent nulls exist only in definition levels—the leaf array itself
has `null_count == 0`.
Because `update_stats` was called before `MaybeReplaceValidity`, page
statistics recorded 0 nulls instead of parent nulls. In V2, `BuildDataPageV2`
has a `DCHECK` verifying page statistics match header null count, which tripped
on CI. In V1, there was no DCHECK, so it silently wrote corrupted statistics
into metadata. Moving `update_stats` after `MaybeReplaceValidity` aligns this
with `WriteArrowDense`.
This affected both V1 and V2 pages. I've added a regression test covering
both: `TestArrowWriteDictionary.NestedDictionaryParentNullStatistics`.
--
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]
