rgbuilds commented on code in PR #26441:
URL: https://github.com/apache/datafusion/pull/26441#discussion_r4507642700


##########
datafusion/catalog-listing/src/table.rs:
##########
@@ -1431,4 +1534,137 @@ mod tests {
         let result = derive_common_ordering_from_files(&file_groups);
         assert_eq!(result, Some(ordering));
     }
+
+    fn prune_test_files(expr: Expr, statistics: Vec<Option<Statistics>>) -> 
Vec<String> {
+        use arrow::datatypes::DataType;
+        let schema = Arc::new(Schema::new(vec![Field::new("d", 
DataType::Int64, true)]));
+        let physical = datafusion_physical_expr::create_physical_expr(
+            &expr,
+            &DFSchema::try_from(schema.as_ref().clone()).unwrap(),
+            &ExecutionProps::new(),
+            &PhysicalPlanningContext::default(),
+        )
+        .unwrap();
+        let predicate = PruningPredicateBuilder::new()
+            .with_file_schema(Arc::clone(&schema))
+            .try_build(physical)
+            .unwrap();
+        let files = FileGroup::new(
+            statistics
+                .into_iter()
+                .enumerate()
+                .map(|(i, stats)| {
+                    let mut file = PartitionedFile::new(format!("f{i}"), 10);
+                    file.statistics = stats.map(Arc::new);
+                    file
+                })
+                .collect(),
+        );
+        prune_collected_files(&predicate, &schema, files)
+            .iter()
+            .map(|f| f.object_meta.location.to_string())
+            .collect()
+    }
+
+    fn bounds(
+        min: Precision<datafusion_common::ScalarValue>,
+        max: Precision<datafusion_common::ScalarValue>,
+    ) -> Statistics {
+        Statistics {
+            num_rows: Precision::Exact(10),
+            column_statistics: vec![datafusion_common::ColumnStatistics {
+                min_value: min,
+                max_value: max,
+                ..Default::default()
+            }],
+            ..Default::default()
+        }
+    }
+
+    #[test]
+    fn statistics_pruning_keeps_missing_and_inexact_bounds() {
+        use datafusion_expr::{col, lit};
+        let exact = |v: i64| Precision::Exact(v.into());
+        let inexact = |v: i64| Precision::Inexact(v.into());
+        assert_eq!(
+            prune_test_files(
+                col("d").gt_eq(lit(20_i64)),
+                vec![
+                    None,
+                    Some(bounds(Precision::Absent, Precision::Absent)),
+                    Some(bounds(inexact(0), inexact(9))),
+                    Some(bounds(exact(0), exact(9))),
+                    Some(bounds(exact(20), exact(29)))
+                ]
+            ),
+            ["f0", "f1", "f2", "f4"]
+        );
+    }
+
+    #[test]
+    fn statistics_pruning_uses_independent_bounds() {
+        use datafusion_expr::{col, lit};
+        assert_eq!(
+            prune_test_files(
+                col("d").lt(lit(20_i64)),
+                vec![
+                    Some(bounds(Precision::Exact(30_i64.into()), 
Precision::Absent)),
+                    Some(bounds(Precision::Absent, 
Precision::Exact(10_i64.into())))
+                ]
+            ),
+            ["f1"]
+        );
+    }
+
+    #[test]
+    fn statistics_pruning_preserves_null_semantics() {
+        use datafusion_expr::{col, lit};
+        let mut all_null = bounds(Precision::Absent, Precision::Absent);
+        all_null.column_statistics[0].null_count = Precision::Exact(10);
+        let mut no_null = bounds(
+            Precision::Exact(0_i64.into()),
+            Precision::Exact(9_i64.into()),
+        );
+        no_null.column_statistics[0].null_count = Precision::Exact(0);
+        assert_eq!(
+            prune_test_files(
+                col("d").is_null(),
+                vec![Some(all_null.clone()), Some(no_null.clone())]
+            ),
+            ["f0"]
+        );
+        assert_eq!(
+            prune_test_files(
+                col("d").is_not_null(),
+                vec![Some(all_null.clone()), Some(no_null.clone())]
+            ),
+            ["f1"]
+        );
+        assert!(
+            prune_test_files(col("d").eq(lit(5_i64)), 
vec![Some(all_null)]).is_empty()
+        );
+        assert_eq!(
+            prune_test_files(
+                col("d").eq(lit(5_i64)).or(col("d").is_null()),
+                vec![Some(no_null)]
+            ),
+            ["f0"]
+        );
+    }
+
+    #[test]
+    fn statistics_pruning_keeps_file_on_evaluation_error() {
+        use datafusion_expr::{col, lit};
+        // A malformed statistics type cannot establish that the file is empty.
+        assert_eq!(
+            prune_test_files(
+                col("d").gt(lit(10_i64)),
+                vec![Some(bounds(
+                    Precision::Exact("invalid".into()),
+                    Precision::Exact("invalid".into())
+                ))]
+            ),

Review Comment:
   @jayzhan211 Addressed. This test now uses Binary bounds, so it exercises the 
pruning error path and verifies that the file is retained on evaluation 
failure. Thanks for catching this.



-- 
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]

Reply via email to