Awesome06 commented on code in PR #55453:
URL: https://github.com/apache/arrow/pull/55453#discussion_r4533328738
##########
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]
