adamreeve commented on code in PR #55453:
URL: https://github.com/apache/arrow/pull/55453#discussion_r4532557232
##########
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:
Same comment as above, does this need a new regression test that would catch
this with both V1 and V2 pages?
##########
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:
Could you please explain why the `null_count() == 0` check is needed here?
Does this fix an issue that was caught by a failing test after switching the
default? It might be worth an explicit test for this scenario that would have
caught this with V1 pages too.
--
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]
