Copilot commented on code in PR #23999:
URL: https://github.com/apache/datafusion/pull/23999#discussion_r3683425670
##########
datafusion/proto/src/physical_plan/from_proto.rs:
##########
@@ -660,7 +660,7 @@ impl TryFromProto<&protobuf::PartitionedFile> for
PartitionedFile {
pf = pf.with_range(file_range.start, file_range.end);
}
if let Some(proto_stats) = val.statistics.as_ref() {
- pf = pf.with_statistics(Arc::new(proto_stats.try_into()?));
+ pf.statistics = Some(Arc::new(proto_stats.try_into()?));
Review Comment:
This directly mutates the `PartitionedFile` field while the surrounding code
uses builder-style methods (`with_range`, etc.). If `PartitionedFile`
invariants are intended to be maintained through setters/builders, consider
adding/using a dedicated method that sets statistics without merging (e.g.,
`set_statistics`/`with_decoded_statistics`) to keep construction patterns
consistent and prevent future callers from reintroducing duplication bugs.
##########
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:
The test relies on a somewhat implicit invariant that adding one partition
value increases `column_statistics.len()` to 2 (data column + partition
column). This is a fragile assertion if the internal representation of column
statistics changes (e.g., schema-driven vs value-driven stats). Consider
asserting the actual bug outcome more directly (e.g., that the decoded
`column_statistics.len()` equals the original length, and/or that it does not
grow after roundtrip) rather than asserting an exact precondition length of `2`.
--
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]
