sunchao commented on code in PR #24526:
URL: https://github.com/apache/datafusion/pull/24526#discussion_r3866798608
##########
datafusion/pruning/src/pruning_predicate.rs:
##########
@@ -1582,6 +1629,16 @@ fn build_predicate_expression(
}
}
if let Some(in_list) = expr.downcast_ref::<phys_expr::InListExpr>() {
+ // Preserve the existing per-value representation for lists within
+ // the default limit. Use the compact form only when callers raise
+ // the cap; MAX_IN_LIST_SIZE is not a measured performance crossover.
+ if in_list.list().len() > MAX_IN_LIST_SIZE
Review Comment:
Made this explicit beside the condition in
[80898fa](https://github.com/apache/datafusion/pull/24526/commits/80898fa91dd2c514175884e59c4877a8c3d25189):
the lower bound is a scope/compatibility choice, not a measured performance
threshold. It preserves the existing expression shape for lists of at most 20
values. I also added the exact inclusive boundaries to the builder docs and PR
description.
##########
datafusion/datasource-parquet/src/opener/mod.rs:
##########
@@ -1814,10 +1817,12 @@ fn create_initial_plan(
pub(crate) fn build_page_pruning_predicate(
predicate: &Arc<dyn PhysicalExpr>,
file_schema: &SchemaRef,
+ max_in_list_size: usize,
) -> Arc<PagePruningAccessPlanFilter> {
- Arc::new(PagePruningAccessPlanFilter::new(
+ Arc::new(PagePruningAccessPlanFilter::new_with_max_in_list_size(
Review Comment:
Agreed that a builder would be the next step if we add more options. For
this PR I kept the public `new` API unchanged and used the internal helper only
to pass the existing configured cap through to page pruning. I'll leave the
broader constructor refactor separate.
##########
datafusion/datasource-parquet/src/page_filter.rs:
##########
@@ -138,15 +138,25 @@ impl PagePruningResult {
impl PagePruningAccessPlanFilter {
/// Create a new [`PagePruningAccessPlanFilter`] from a physical
- /// expression.
+ /// expression, using the default `IN (...)` pruning limit.
#[expect(clippy::needless_pass_by_value)]
pub fn new(expr: &Arc<dyn PhysicalExpr>, schema: SchemaRef) -> Self {
+ Self::new_with_max_in_list_size(expr, &schema, MAX_IN_LIST_SIZE)
+ }
+
+ /// Create a page filter using the same `IN (...)` limit as row-group
pruning.
+ pub(crate) fn new_with_max_in_list_size(
+ expr: &Arc<dyn PhysicalExpr>,
+ schema: &SchemaRef,
+ max_in_list_size: usize,
+ ) -> Self {
// extract any single column predicates
let predicates = split_conjunction(expr)
.into_iter()
.filter_map(|predicate| {
let pp = match PruningPredicateBuilder::new()
- .with_file_schema(Arc::clone(&schema))
+ .with_file_schema(Arc::clone(schema))
+ .with_max_in_list_size(max_in_list_size)
Review Comment:
Yes, the construction already delegates to `PruningPredicateBuilder`. I kept
the page-filter wrapper change narrow here; consolidating its options into a
builder makes sense when another option is added.
--
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]
