Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25469 +/- ##
==========================================
+ Coverage 82.42% 82.64% +0.22%
==========================================
Files 1138 1147 +9
Lines 435429 445677 +10248
Branches 435429 445677 +10248
==========================================
+ Hits 358889 368332 +9443
- Misses 54839 54977 +138
- Partials 21701 22368 +667 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| fn struct_field_mapping( | ||
| &self, | ||
| literal_args: &[Option<ScalarValue>], | ||
| ) -> Option<StructFieldMapping> { | ||
| Some(StructFieldMapping { | ||
| field_accessor: Arc::new(ScalarUDF::from(GetFieldFunc::new())), | ||
| fields: (0..literal_args.len()) | ||
| .map(|i| (vec![ScalarValue::Utf8(Some(format!("c{i}")))], i)) | ||
| .collect(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
StructFunc from datafusion-functions crate already provides support for this, but instead of adding another dependency I thought it might be a better idea to have it defined separately here instead
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_tuple_in_bloom_pruning_preserves_correlation() -> Result<()> { |
There was a problem hiding this comment.
The unit test nicely covers both pruning and the crossed-pair correlation case, while the SLT exercises the real Parquet scan path. Would it be valuable to combine these in one end-to-end test with multiple row groups? For example, the test could include:
- A row group containing an exact tuple match.
- A row group that statistics cannot eliminate but Bloom filters can, verifying that it is counted as pruned in row_groups_pruned_bloom_filter.
- A crossed-pair row group that satisfies the derived per-column guarantees and therefore survives Bloom pruning, but produces no rows after evaluation of the original tuple predicate.
This would verify that the derived per-column conditions are used only for pruning, while the original tuple predicate is still applied for exact row filtering.
There was a problem hiding this comment.
thanks for the review! this makes sense to me, will extend this test
| let batch = RecordBatch::try_from_iter([("tuple", tuples)]).ok()?; | ||
|
|
||
| let mut projected = Vec::new(); | ||
| for (accessor_args, source_index) in mapping.fields { |
There was a problem hiding this comment.
I think duplicate named_struct field names can make this inference unsound.
named_struct currently permits duplicate names and emits a mapping entry for each field, while get_field uses StructArray::column_by_name, which selects the first matching field when names are duplicated.
For example:
named_struct('k', x, 'k', y)
IN (named_struct('k', 1, 'k', 10))Both mapping entries evaluate get_field(..., 'k') against the first literal field. This appears to derive:
x IN (1)
y IN (1)
However, (x=1, y=10) satisfies the original tuple predicate.
A row group containing the matching row (1,10) and a filler row (0,0) would have y_min=0 and y_max=10, so statistics cannot eliminate y=1. Its Bloom filter could nevertheless prove that y=1 is absent and incorrectly prune the row group containing the valid (1,10) match.
Would it be safer for named_struct::struct_field_mapping to return no mapping when an accessor name is repeated, with a focused regression test for this case?
There was a problem hiding this comment.
Thanks. Returning no mapping when a field name is repeated prevents the ambiguous inference, and the focused regression test covers the case I raised. This resolves my concern.
|
Thanks for adding the three-row-group end-to-end scan case. This addresses my earlier review comment: the revised test now exercises:
I also ran While reviewing the revised guarantee-extraction path, I found a duplicate-field-name case in I’m requesting changes pending resolution of that correctness case. |
rgbuilds
left a comment
There was a problem hiding this comment.
Thanks for the updates. The duplicate-field-name correctness concern and the earlier end-to-end scan coverage are both addressed. The latest changes look good to me.
Which issue does this PR close?
Closes #25463
Rationale for this change
Multi-column joins can produce filters such as:
DataFusion can evaluate this filter on individual rows, but it does not extract the allowed values for each column. This prevents bloom filters from using those values to skip row groups.
What changes are included in this PR?
a IN (1, 2)andb IN (10, 20).(1, 20)are still rejected.This will only enable bloom filter pruning for row groups.
What is the testing strategy for this PR?
Added tests for value extraction, NULLs, dictionary values, and reordered named fields.
Are there any user-facing changes?
Queries with supported tuple IN filters, including dynamic filters from multi-column joins, may read fewer row groups when Bloom filters are available. Query results remain unchanged.