rgehan commented on code in PR #24101:
URL: https://github.com/apache/datafusion/pull/24101#discussion_r3719637804


##########
datafusion/datasource-parquet/src/row_group_filter.rs:
##########
@@ -1371,6 +1381,37 @@ mod tests {
         assert_pruned(row_groups, ExpectedPruning::Some(vec![1, 2]));
     }
 
+    #[test]
+    fn bloom_filter_pruning_evaluates_row_groups_with_bloom_filters() {
+        // c1 = 15
+        let schema =
+            Arc::new(Schema::new(vec![Field::new("c1", DataType::Int32, 
false)]));
+        let expr = logical2physical(&col("c1").eq(lit(15)), &schema);
+        let pruning_predicate = PruningPredicate::try_new(expr, 
schema).unwrap();
+
+        // Row group 0 has no bloom filter; row group 1 has a bloom filter
+        // whose values do not include 15, so it can be pruned.
+        let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap();
+        sbbf.insert(&1_i32);
+        sbbf.insert(&2_i32);
+        let mut with_bloom_filter = BloomFilterStatistics::new();
+        with_bloom_filter.insert("c1", sbbf, PhysicalType::INT32, 4);
+
+        let metrics = parquet_file_metrics();
+        let mut row_groups = 
RowGroupAccessPlanFilter::new(ParquetAccessPlan::new_all(2));
+        row_groups.prune_by_bloom_filters(
+            &pruning_predicate,
+            &metrics,
+            &[BloomFilterStatistics::new(), with_bloom_filter],
+        );
+
+        // The bloom-filter-less row group is kept without evaluation while
+        // the row group with a bloom filter is still evaluated and pruned.
+        assert_pruned(row_groups, ExpectedPruning::Some(vec![0]));
+        assert_eq!(metrics.row_groups_pruned_bloom_filter.pruned(), 1);
+        assert_eq!(metrics.row_groups_pruned_bloom_filter.matched(), 1);
+    }

Review Comment:
   Not a big fan of this test, as it would pass even without the fix, and 
doesn't really distinguish between "skipped predicate" and "evaluated predicate 
that matches".
   
   An alternative would be to test with an expensive predicate, and assert on 
the timer, but that sounds fragile.



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