jayzhan211 commented on code in PR #25228:
URL: https://github.com/apache/datafusion/pull/25228#discussion_r3998401708
##########
datafusion/datasource-parquet/src/metadata.rs:
##########
@@ -785,13 +785,42 @@ fn summarize_column_statistics(
/// parquet statistics. `row_group_exactness` rebuilds the exactness as a
Boolean
/// array and is only called for the rare case where row groups disagree.
fn summarize_bound<A: Accumulator>(
- acc: &mut A,
+ acc: &mut Option<A>,
values: &ArrayRef,
parquet_index: Option<usize>,
row_groups_metadata: &[RowGroupMetaData],
is_exact: impl Fn(&ParquetStatistics) -> bool,
row_group_exactness: impl FnOnce() -> Result<BooleanArray>,
) -> Result<Option<bool>> {
+ // A NULL converted bound can mean missing statistics, not just all-NULL
+ // data. Ignoring it in MIN/MAX would let another row group's exact
endpoint
+ // incorrectly establish an exact bound for the whole file. Drop this bound
+ // unless the row group is empty or is proven to contain only NULLs.
+ if values.null_count() > 0
+ && parquet_index.is_some_and(|column_index| {
+ row_groups_metadata
+ .iter()
+ .enumerate()
+ .any(|(index, group)| {
+ if values.is_valid(index) || group.num_rows() == 0 {
+ return false;
+ }
+ let column = group.column(column_index);
+ let all_null = column.num_values() == group.num_rows()
+ && column
+ .statistics()
+ .and_then(|stats| stats.null_count_opt())
+ .is_some_and(|nulls| {
+ i64::try_from(nulls).ok() ==
Some(group.num_rows())
+ });
+ !all_null
+ })
+ })
+ {
+ *acc = None;
+ return Ok(None);
+ }
+ let acc = acc.as_mut().expect("caller checked accumulator is present");
Review Comment:
```suggestion
let Some(acc) = acc.as_mut() else { return Ok(None) };present");
```
Suggest to not panic
##########
datafusion/datasource-parquet/src/metadata.rs:
##########
@@ -785,13 +785,42 @@ fn summarize_column_statistics(
/// parquet statistics. `row_group_exactness` rebuilds the exactness as a
Boolean
/// array and is only called for the rare case where row groups disagree.
fn summarize_bound<A: Accumulator>(
- acc: &mut A,
+ acc: &mut Option<A>,
values: &ArrayRef,
parquet_index: Option<usize>,
row_groups_metadata: &[RowGroupMetaData],
is_exact: impl Fn(&ParquetStatistics) -> bool,
row_group_exactness: impl FnOnce() -> Result<BooleanArray>,
) -> Result<Option<bool>> {
+ // A NULL converted bound can mean missing statistics, not just all-NULL
+ // data. Ignoring it in MIN/MAX would let another row group's exact
endpoint
+ // incorrectly establish an exact bound for the whole file. Drop this bound
+ // unless the row group is empty or is proven to contain only NULLs.
+ if values.null_count() > 0
+ && parquet_index.is_some_and(|column_index| {
+ row_groups_metadata
+ .iter()
+ .enumerate()
+ .any(|(index, group)| {
+ if values.is_valid(index) || group.num_rows() == 0 {
+ return false;
+ }
+ let column = group.column(column_index);
+ let all_null = column.num_values() == group.num_rows()
+ && column
+ .statistics()
+ .and_then(|stats| stats.null_count_opt())
+ .is_some_and(|nulls| {
+ i64::try_from(nulls).ok() ==
Some(group.num_rows())
+ });
Review Comment:
```suggestion
let all_null = column
.statistics()
.and_then(|stats| stats.null_count_opt())
.is_some_and(|nulls| nulls == column.num_values() as u64);
```
##########
datafusion/sqllogictest/src/test_context.rs:
##########
@@ -415,6 +418,76 @@ pub async fn register_partition_table(test_ctx: &mut
TestContext) {
.unwrap();
}
+/// Write row groups with different statistics settings using the public
writer API.
+async fn register_parquet_missing_bounds(test_ctx: &mut TestContext) {
+ use datafusion::parquet::column::writer::ColumnWriterImpl;
+ use datafusion::parquet::data_type::{ByteArray, ByteArrayType};
+ use datafusion::parquet::file::properties::{EnabledStatistics,
WriterProperties};
+ use datafusion::parquet::file::writer::{
+ SerializedFileWriter, SerializedPageWriter, TrackedWrite,
+ };
+ use datafusion::parquet::schema::parser::parse_message_type;
+
+ test_ctx.enable_testdir();
+ let path = test_ctx.testdir_path().join("missing_bounds.parquet");
+ let column_path = test_ctx.testdir_path().join("column.pages");
+ let schema = Arc::new(
+ parse_message_type("message schema { REQUIRED BINARY a (UTF8);
}").unwrap(),
+ );
+ let mut writer = SerializedFileWriter::new(
+ File::create(&path).unwrap(),
+ schema,
+ Arc::new(WriterProperties::default()),
+ )
+ .unwrap();
+ let long_value = "z".repeat(8192);
+ for (values, statistics) in [
+ (["a", "b"], EnabledStatistics::Chunk),
+ (
+ [long_value.as_str(), long_value.as_str()],
+ EnabledStatistics::None,
+ ),
+ ] {
+ // parquet-rs truncates long extrema rather than omitting them. Disable
+ // statistics for the second chunk to exercise the missing-bound case
+ // produced naturally by writers such as PyArrow, without editing
metadata.
+ let properties = Arc::new(
+ WriterProperties::builder()
+ .set_statistics_enabled(statistics)
+ .build(),
+ );
+ let mut buffer =
TrackedWrite::new(File::create(&column_path).unwrap());
+ let mut column = ColumnWriterImpl::<ByteArrayType>::new(
+ writer.schema_descr().column(0),
+ properties,
+ Box::new(SerializedPageWriter::new(&mut buffer)),
+ );
+ let values = values.map(ByteArray::from);
+ column.write_batch(&values, None, None).unwrap();
+ let result = column.close().unwrap();
+ assert_eq!(
+ result.metadata.statistics().is_some(),
+ statistics == EnabledStatistics::Chunk
+ );
+ drop(buffer);
Review Comment:
drop(buffer) flushes the TrackedWrite's internal BufWriter with errors
discarded. Use buffer.into_inner().unwrap() so a write failure surfaces there
rather than as a confusing append_column error. The column.pages scratch file
is also left in the test dir after the loop; harmless, but std::fs::remove_file
after writer.close() would keep the temp dir tidy.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
