[Parquet] Implement num distinct values for parquet writer - #10654
Open
Rich-T-kid wants to merge 5 commits into
Open
[Parquet] Implement num distinct values for parquet writer#10654Rich-T-kid wants to merge 5 commits into
Rich-T-kid wants to merge 5 commits into
Conversation
Rich-T-kid
force-pushed
the
rich-t-kid/parquet-write-nvd
branch
from
August 12, 2026 13:09
cda6e4a to
5111139
Compare
Rich-T-kid
commented
Aug 12, 2026
Comment on lines
+905
to
+909
| /// # Performance | ||
| /// | ||
| /// Computing the distinct count requires hashing every non-null value in the column. | ||
| /// For large row groups or columns with many values this adds measurable overhead. | ||
| /// Benchmark your workload before enabling this globally. |
Contributor
Author
There was a problem hiding this comment.
worth noting that turning the flag on does cause a 1.4x-8.8x regressions for the arrow writer.
Happy to make a separate issue to improve the perf of the current approach. I suspect this will also cause a decent regression when turned on since we need an exact count which requires a hash set which is both expensive in term of compute and memory.
Contributor
There was a problem hiding this comment.
I think I'd prefer "significant" to "measurable".
Contributor
Author
There was a problem hiding this comment.
I think I'd prefer "significant" to "measurable".
@etseidl updated it
Rich-T-kid
force-pushed
the
rich-t-kid/parquet-write-nvd
branch
from
August 12, 2026 13:16
5111139 to
5676cca
Compare
etseidl
changed the title
Rich-T-kid
commented
Aug 14, 2026
Comment on lines
+1988
to
+2043
| fn update_distinct_values_seen( | ||
| array: &dyn arrow_array::Array, | ||
| non_null_indices: &[usize], | ||
| seen: &mut DistinctValuesSet, | ||
| ) { | ||
| let data = array.to_data(); | ||
| let offset = data.offset(); | ||
|
|
||
| match array.data_type() { | ||
| ArrowDataType::Boolean => { | ||
| let arr = array | ||
| .as_any() | ||
| .downcast_ref::<arrow_array::BooleanArray>() | ||
| .unwrap(); | ||
| for &row in non_null_indices { | ||
| seen.insert(arr.value(row) as u64); | ||
| } | ||
| } | ||
| ArrowDataType::Utf8 | ArrowDataType::Binary => { | ||
| let offsets = data.buffers()[0].typed_data::<i32>(); | ||
| let values = data.buffers()[1].as_slice(); | ||
| for &row in non_null_indices { | ||
| let start = offsets[offset + row] as usize; | ||
| let end = offsets[offset + row + 1] as usize; | ||
| seen.insert(hash_bytes(&values[start..end])); | ||
| } | ||
| } | ||
| ArrowDataType::LargeUtf8 | ArrowDataType::LargeBinary => { | ||
| let offsets = data.buffers()[0].typed_data::<i64>(); | ||
| let values = data.buffers()[1].as_slice(); | ||
| for &row in non_null_indices { | ||
| let start = offsets[offset + row] as usize; | ||
| let end = offsets[offset + row + 1] as usize; | ||
| seen.insert(hash_bytes(&values[start..end])); | ||
| } | ||
| } | ||
| ArrowDataType::FixedSizeBinary(byte_width) => { | ||
| let byte_width = *byte_width as usize; | ||
| let buffer = data.buffers()[0].as_slice(); | ||
| for &row in non_null_indices { | ||
| let start = (offset + row) * byte_width; | ||
| seen.insert(hash_bytes(&buffer[start..start + byte_width])); | ||
| } | ||
| } | ||
| data_type => { | ||
| if let Some(width) = fixed_byte_width(data_type) { | ||
| let buffer = data.buffers()[0].as_slice(); | ||
| for &row in non_null_indices { | ||
| let pos = (offset + row) * width; | ||
| seen.insert(hash_bytes(&buffer[pos..pos + width])); | ||
| } | ||
| } | ||
| // Utf8View, BinaryView, nested types: skip | ||
| } | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
In a follow up PR it'd be nice to add some benchmarks for this
| seen.insert(hash_bytes(&buffer[pos..pos + width])); | ||
| } | ||
| } | ||
| // Utf8View, BinaryView, nested types: skip |
Contributor
Author
There was a problem hiding this comment.
Can also make a follow up issue to support the rest of the types
Comment on lines
+1125
to
+1127
| // For dictionary arrays, hash the integer keys rather than the actual values. | ||
| // Key cardinality equals value cardinality, so distinct-value counting stays | ||
| // correct while avoiding the cost of hashing arbitrary-length values. |
Contributor
Author
There was a problem hiding this comment.
a similar approach may or may not be worth it for REE's, worth benchmarking first
Rich-T-kid
force-pushed
the
rich-t-kid/parquet-write-nvd
branch
from
August 14, 2026 14:27
48c40aa to
d1fa404
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
see #8608, #10650 & apache/datafusion#24114
What changes are included in this PR?
Adds
set_write_row_group_number_distinct_valuesto WriterProperties, which when enabled causes the ArrowWriter to track the exact number of distinct non-null values per column across the full row group and write it into the column chunk statistics footer asdistinct_count. Tracking is implemented by hashing each non-null value using XxHash64 into a per-column HashSet that persists across batch writes and is finalized inclose(). The flag defaults to false so there is no impact on existing writers.Are these changes tested?
yes, see test.
Are there any user-facing changes?
yes, users will not be able to write
distinct_countmeta data to parquet files.