adriangb commented on code in PR #23999:
URL: https://github.com/apache/datafusion/pull/23999#discussion_r3683588463
##########
datafusion/proto/src/physical_plan/from_proto.rs:
##########
@@ -858,6 +857,22 @@ mod tests {
);
}
+ #[test]
+ fn partitioned_file_statistics_roundtrip_with_partition_values() {
+ use datafusion_common::Statistics;
+
+ let file_schema = Schema::new(vec![Field::new("a", DataType::Int32,
true)]);
+ let pf = PartitionedFile::new("foo/bar.parquet", 1234)
+ .with_partition_values(vec![ScalarValue::from("2024-01-01")])
+ .with_statistics(Arc::new(Statistics::new_unknown(&file_schema)));
+ assert_eq!(pf.statistics.as_ref().unwrap().column_statistics.len(), 2);
Review Comment:
Agreed on the magic `2`, though I'd narrow the scope of the concern: the
assertion that actually pins this bug is `assert_eq!(decoded.statistics,
pf.statistics)` at the end of the test. That one is already
representation-independent and strictly stronger than any length comparison, so
an additional post-roundtrip length check would be redundant with it.
The only genuinely fragile line is the precondition. Deriving it from the
inputs keeps it self-documenting without hardcoding:
```suggestion
// `statistics` covers the full table schema: file columns followed
by one
// entry per partition column.
let expected_len = file_schema.fields().len() +
pf.partition_values.len();
assert_eq!(
pf.statistics.as_ref().unwrap().column_statistics.len(),
expected_len
);
```
Verified locally: compiles, the test passes, and `cargo fmt --check` is
clean.
--
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]
