alamb commented on code in PR #25818:
URL: https://github.com/apache/datafusion/pull/25818#discussion_r4145209077
##########
datafusion/datasource-parquet/src/metadata.rs:
##########
@@ -591,15 +592,20 @@ impl<'a> DFParquetMetadata<'a> {
column_byte_sizes: &mut column_byte_sizes,
distinct_counts_array: &mut
distinct_counts_array,
};
- summarize_column_statistics(
+ if let Err(e) = summarize_column_statistics(
Review Comment:
DataFusionError is expensive to create because it has a String in it -- is
there a way to avoid creating an error just to ignore it?
##########
datafusion/datasource-parquet/src/metadata.rs:
##########
@@ -1410,6 +1452,195 @@ mod tests {
ParquetMetaData::new(file_meta, row_groups)
}
+ #[test]
+ fn test_timestamp_bound_exactness_after_conversion() -> Result<()> {
Review Comment:
Do these tests add additional coverage over the .slt tests? For example, are
there bugs that would fail these tests but leave the slt tests still passing?
##########
datafusion/datasource-parquet/src/metadata.rs:
##########
@@ -753,10 +759,40 @@ fn summarize_column_statistics(
) -> Result<()> {
let parquet_index = stats_converter.parquet_column_index();
+ accumulators.null_counts_array[logical_schema_index] =
+ summarize_null_counts(stats_converter, row_groups_metadata)?;
+
+ let arrow_field = logical_file_schema.field(logical_schema_index);
+ let data_type = min_max_aggregate_data_type(arrow_field.data_type());
+ let file_data_type =
+ min_max_aggregate_data_type(stats_converter.arrow_field().data_type());
+ let distinct_count = summarize_distinct_counts(parquet_index,
row_groups_metadata);
+ // A type conversion can merge distinct values, for example when reducing
Review Comment:
This makes sense but is sort of buried in the code. Can we extract it
somewhere / into a function we can document? I also think the concern of a
narrowing cast is more specific than for just timestamps (for example, how
about converting float64 --> float32 🤔 )
I feel like there was already some code that had this notion (of "widening
/ narrowing casts") -- is there any way to use the pre-existing code / checks?
--
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]
