alamb commented on code in PR #24074:
URL: https://github.com/apache/datafusion/pull/24074#discussion_r3715778163
##########
datafusion/pruning/src/pruning_predicate.rs:
##########
@@ -461,7 +483,19 @@ impl PruningPredicate {
/// returns a new expression.
/// It is recommended that you pass the expressions through
[`PhysicalExprSimplifier`]
/// before calling this method to make sure the expressions can be used
for pruning.
- pub fn try_new(mut expr: Arc<dyn PhysicalExpr>, schema: SchemaRef) ->
Result<Self> {
+ pub fn try_new(expr: Arc<dyn PhysicalExpr>, schema: SchemaRef) ->
Result<Self> {
+ Self::try_new_with_max_in_list_size(expr, schema,
MAX_LIST_VALUE_SIZE_REWRITE)
Review Comment:
Same comment above related to simplifying this API via a builder rather than
more methods
##########
datafusion/datasource-parquet/src/opener/mod.rs:
##########
@@ -1860,6 +1875,7 @@ mod test {
enable_row_group_stats_pruning: false,
coerce_int96: None,
max_predicate_cache_size: None,
+ pruning_max_in_list_size: MAX_LIST_VALUE_SIZE_REWRITE,
Review Comment:
it is strange to me that these names are not the same -- I would expect
something like
```rust
pruning_max_in_list_size: PRUNING_MAX_IN_LIST_SIZE,
```
##########
datafusion/pruning/src/pruning_predicate.rs:
##########
@@ -388,7 +388,29 @@ pub fn build_pruning_predicate(
file_schema: &SchemaRef,
predicate_creation_errors: &Count,
) -> Option<Arc<PruningPredicate>> {
- match PruningPredicate::try_new(predicate, Arc::clone(file_schema)) {
+ build_pruning_predicate_with_max_in_list_size(
+ predicate,
+ file_schema,
+ predicate_creation_errors,
+ MAX_LIST_VALUE_SIZE_REWRITE,
Review Comment:
Why not just add the parameter to `build_pruning_predicate` ?
If we are going to introduce a new API, perhaps we can make one that is more
future proof, like a builder
```rust
let pruning_predicate = PruningPredicaateBuilder::new()
.with_file_schema(file_schema)
.with_error_counter(predicate_creation_errors)
.build(predicate)?;
```
That way if we add new parameters we have a place to put them
##########
datafusion/common/src/config.rs:
##########
@@ -1189,6 +1189,22 @@ config_namespace! {
/// parquet reader setting. 0 means no caching.
pub max_predicate_cache_size: Option<usize>, default = None
+ /// Maximum number of values in an `IN (...)` list for which the
+ /// pruning predicate will rewrite the list into a chain of per-value
+ /// statistics checks. Lists longer than this fall back to the
+ /// unhandled-predicate hook (defaulting to "keep the container"),
+ /// which effectively skips container-level pruning for large IN
+ /// lists.
+ ///
+ /// Higher values keep row-group / file-range statistics pruning
+ /// effective for larger IN lists (for example, REST endpoints that
+ /// filter by a batch of ~25-100 identifiers), at the cost of a
+ /// larger rewritten predicate expression evaluated for every
+ /// container. Set to 0 to disable the rewrite path entirely.
+ ///
+ /// The default of 20 preserves the previous hardcoded behaviour.
+ pub pruning_max_in_list_size: usize, default = 20
Review Comment:
Also I suggest changing this to be something more conisstent with the others
names like `max_predicate_cache_size`
Perhaps something like`max_in_list_size` or `max_in_list_pruning_size`
##########
datafusion/common/src/config.rs:
##########
@@ -1189,6 +1189,22 @@ config_namespace! {
/// parquet reader setting. 0 means no caching.
pub max_predicate_cache_size: Option<usize>, default = None
+ /// Maximum number of values in an `IN (...)` list for which the
+ /// pruning predicate will rewrite the list into a chain of per-value
+ /// statistics checks. Lists longer than this fall back to the
+ /// unhandled-predicate hook (defaulting to "keep the container"),
+ /// which effectively skips container-level pruning for large IN
+ /// lists.
+ ///
+ /// Higher values keep row-group / file-range statistics pruning
+ /// effective for larger IN lists (for example, REST endpoints that
+ /// filter by a batch of ~25-100 identifiers), at the cost of a
+ /// larger rewritten predicate expression evaluated for every
+ /// container. Set to 0 to disable the rewrite path entirely.
+ ///
+ /// The default of 20 preserves the previous hardcoded behaviour.
Review Comment:
I think we could rewrite this to focus more on the end user visible effects
to make it clearer what was going on
```suggestion
/// Maximum number of values in an `IN (...)` list for which pruning
will
/// occur. Longer lists will not be used to prune files, row groups,
or
/// data pages.
///
/// Higher values help in cases such as a list of
/// of ~25-100 identifiers, but also makes the predicate
/// more expensive to evaluate. Set to 0 to disable the IN (..) list
pruning entirely
///
/// Defaults to 20.
```
--
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]
