perf: use compact pruning for large string IN lists - #24526
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24526 +/- ##
==========================================
+ Coverage 81.44% 81.46% +0.01%
==========================================
Files 1118 1119 +1
Lines 399602 400126 +524
Branches 399602 400126 +524
==========================================
+ Hits 325460 325947 +487
+ Misses 55146 55141 -5
- Partials 18996 19038 +42 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
cc @alamb |
## Why are the changes needed? ### Which issue does this PR close? Part of apache#10586. This fixes the unsigned string/binary byte-array case; it does not claim to resolve every Parquet ordering issue. It is also the correctness prerequisite for apache#24526. ### Rationale for this change A Parquet scan can skip a file, row group, or page when its statistics prove that no row can satisfy the filter. If those statistics use a different comparison order from the query, that proof is invalid: DataFusion can silently discard a row that should be returned. For example, suppose a Parquet row group contains `'aé'`, `'az'`, and `'b'`: ```sql SELECT s FROM t WHERE s = 'az'; -- Expected: one row containing 'az' ``` Parquet's deprecated byte-array `min`/`max` fields use signed comparison, whereas Arrow compares strings using unsigned UTF-8 bytes. The same values therefore have two different orders: | Comparison | Values in ascending order | Minimum / maximum | | --- | --- | --- | | Legacy signed-byte order | `aé < az < b` | `aé` / `b` | | Arrow's unsigned-byte order | `az < aé < b` | `az` / `b` | The first UTF-8 byte of `é` is `0xC3`: it sorts before `z`'s `0x7A` as a signed byte, but after it as an unsigned byte. If DataFusion interprets the legacy interval `['aé', 'b']` using Arrow's ordering, it sees `'az' < 'aé'` and can incorrectly skip the row group. Merely checking that `min <= max` does not help: the two reported endpoints are still in ascending order. The newer `min_value`/`max_value` fields and page-index bounds also need an ordering declaration that the reader understands. A missing or unknown `column_orders` entry is not enough to justify assuming Arrow's ordering. The [Parquet statistics definition](https://github.com/apache/parquet-format/blob/master/src/main/thrift/parquet.thrift) and [logical-type ordering rules](https://github.com/apache/parquet-format/blob/master/LogicalTypes.md) describe these distinctions. This bug is independently observable with an ordinary equality filter; it does not require a large `IN` list. ## What changes were proposed in this PR? ### What changes are included in this PR? The change makes a recognized comparison order a prerequisite for using unsigned byte-array bounds. At the point where Parquet metadata becomes Arrow statistics, DataFusion checks whether the column's physical/logical type and footer establish the expected unsigned order. It also rejects row-group bounds taken from the deprecated signed-order fields. When that evidence is missing, min/max is reported as unknown, so pruning keeps potentially matching data instead of guessing. The rule is applied at the granularity where the statistics are used. An unsafe row group prevents DataFusion from claiming a trustworthy bound for the whole file, but it does not make other row groups' valid bounds unusable. Page-index bounds are checked against the footer independently. The same safeguards cover static and runtime row-group pruning, as well as the inverse predicates used to decide whether every row already satisfies a filter. Null counts and unrelated columns' statistics remain available. The row-group pruning API gains a metadata-aware entry point so callers can supply the footer needed for this decision. The existing entry point remains source-compatible and behaves conservatively when that information is unavailable. Signed logical types such as decimals retain their existing behavior. ### Are there any user-facing changes? Queries no longer discard matching data because of these untrustworthy byte-array bounds. Older files, or files with an unrecognized ordering, may require more scanning. Modern files with trustworthy bounds retain min/max pruning. There is no file-format change or breaking public API change. ## How was this PR tested? ### Are these changes tested? The regression uses actual serialized Parquet files containing the example above, with modern, deprecated, missing-order, and unknown-order metadata. It checks that the matching `az` row survives file, row-group, runtime, and page pruning, while valid statistics can still eliminate unrelated data. On unchanged Apache `f1f0449a`, the adapted equality regression fails because the deprecated-order case loses `az`; the modern-statistics control passes. The dedicated Parquet-crate run passed 231 unit tests and four doctests. Its seven focused ordering tests also cover mixed safe/unsafe row groups, null counts, fixed-length binary and UUID, signed decimal, and logical types with undefined ordering. Formatting, all-targets/all-features Clippy with warnings denied, and `./dev/rust_lint.sh` passed. The extended workspace run passed 10,666 Rust tests, with eight ignored, and all 503 SQL-logic files. The existing metadata benchmark was run on Apache `f1f0449a` and this patch using the same valid modern-footer fixture. Across nine full-statistics cases there was no material regression; the largest case, with 256 columns and 128 row groups, measured 1.330 ms before and 1.332 ms after. Both runs used Rust 1.97.0, `release-nonlto`, 20 samples, and separate build directories on an Apple M5 Max. <details> <summary>Validation commands</summary> ```sh cargo test --locked --profile ci -p datafusion-datasource-parquet cargo bench --locked --profile release-nonlto -p datafusion-datasource-parquet \ --bench parquet_metadata_statistics -- \ metadata_full --sample-size 20 --warm-up-time 0.5 --measurement-time 1 --noplot RUST_BACKTRACE=1 cargo test --locked --profile ci \ --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-cli \ --workspace --lib --tests --bins \ --features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption ``` </details>
|
FYI I think @geoffreyclaude has many optimizations for IN list in flight @geoffreyclaude can you help review this PR and let me know if it conflicts with what you have planned? (BTW @sunchao is the original author of the arrow-rs parquet reader, one of the original contributors of DataFusion Comet, and a long time friend of the project) |
b20b0e7 to
9447680
Compare
|
Thanks @alamb for the intro :) It's great to be back to the DataFusion community! cc @viirya @comphead @andygrove too - once integrated and enabled, this PR lets Comet’s native Parquet scans efficiently skip row groups and pages using large string |
viirya
left a comment
There was a problem hiding this comment.
Nice result — replacing the OR tree with a sorted-domain intersection keeps the gap precision while making large lists cheap to represent and evaluate, and the eligibility gate reads conservatively: positive IN only, string (or dictionary-of-string) types only, any non-string literal in the list bails out, oversized string buffers degrade to all-NULL, and the result is combined with a non-null check so an all-NULL container is handled by the null-count branch. Good catch on page-level pruning too — it now honours the same max_in_list_size as row-group pruning rather than silently using the default.
Re @alamb's question about whether this conflicts with #19241: as far as I can tell it doesn't — the two work at different layers. #19241 optimizes row-level IN evaluation (per-row bitmap / hash membership), and this PR explicitly leaves InListExpr as the row filter ("this expression is used only for pruning; the original IN remains the row filter"). This PR works at the container-pruning layer, deciding from parquet min/max statistics whether a row group or page needs to be read at all. They compose rather than overlap: fewer containers read, and the rows that are read still go through whatever the fastest row filter is.
The one place I'd flag for @geoffreyclaude is Utf8View/dictionary handling: #24088 (all-inline Utf8View/BinaryView) and #24658 (avoid dictionary filter copies) touch those on the row-filter side, while this PR adds its own Utf8View/dictionary normalization here — including the u32-offset overflow guard and preserving dictionary values' logical nulls. Probably an opportunity for shared helpers rather than a conflict, but worth a look from both sides.
A few questions inline.
| if min > max { | ||
| return None; | ||
| } | ||
| let index = self.values.partition_point(|v| v.as_bytes() < min); |
There was a problem hiding this comment.
The set is sorted with values.sort_unstable() (Rust String/str ordering) and compared here via as_bytes(), while the interval bounds come from parquet statistics. Parquet specifies unsigned byte-wise ordering for BYTE_ARRAY/UTF8 and Rust's str Ord is also byte-wise, so these line up — but it is the load-bearing assumption of the whole optimization and it's currently implicit. Could a comment state it?
Relatedly: is there any path where the column's ColumnOrder is unset or defines a different collation, where these min/max shouldn't be trusted for an ordered comparison? The existing per-value min <= v AND v <= max path has the same dependency, so this may be pre-existing rather than something this PR introduces — I mostly want to know whether it's been considered.
There was a problem hiding this comment.
Following up on my own question: I see now that #24525 — the PR this one is stacked on — is precisely about this, distinguishing the legacy signed-byte min/max from the unsigned ordering Arrow uses, and it calls itself "the correctness prerequisite for #24526". So the ordering question is already handled upstream in the stack; sorry for asking you to re-explain it.
What's still worth confirming is narrower: does the compact path here sit behind the same ordering gate that #24525 introduces? Since StringInListPruningExpr does its own byte comparisons on the statistics arrays rather than going through the per-value comparison expressions, I wanted to check that a column whose statistics ordering is unusable is rejected before it reaches this expression, rather than only being filtered on the path #24525 touched.
There was a problem hiding this comment.
Yes—the compact expression reads the same gated min/max columns as the per-value expressions; it does not access raw Parquet metadata. The row-group adapter withholds bounds for unusable footer ordering and masks deprecated byte-array bounds to NULL. The page adapter also withholds untrusted bounds, and runtime row-group pruning reuses the row-group adapter.
The distinction is that we suppress unusable bounds, not the entire predicate: the compact expression can still run with NULL bounds and return UNKNOWN, keeping the container unless independent statistics, such as an all-null count, safely exclude it.
The 22-value page regression explicitly asserts IN_SET_INTERSECTS and verifies that the matching az row survives missing/unknown column order.
Agreed about making the ordering assumption explicit: Rust string ordering and these byte comparisons both use unsigned lexicographic UTF-8 byte order. Generic PruningStatistics providers retain the existing responsibility to supply bounds in the comparison order, or mark them unavailable. A short comment here would make that contract clearer.
| Some(self.values.get(index).is_some_and(|v| v.as_bytes() <= max)) | ||
| } | ||
| // A single known bound can still exclude the whole domain. | ||
| (Some(min), None) |
There was a problem hiding this comment.
These single-bound arms exclude via values.last() / values.first() and otherwise fall through to None (unknown, so the container is kept). That looks right to me, but the asymmetry with the two-bound case is worth a comment: when only max is known the interval is unbounded below, so values.first() > max is the only thing that can rule it out — no gap reasoning is possible. Is that the intended reading, i.e. is first() > max the complete exclusion condition here?
There was a problem hiding this comment.
Yes, that is the intended reading. With only max, the possible interval is unbounded below, so a nonempty sorted domain is disjoint exactly when values.first() > max. Symmetrically, with only min, it is disjoint exactly when values.last() < min. Equality must keep the container; otherwise we return UNKNOWN. Gaps between requested values cannot exclude an interval without its other endpoint.
The interval regression checks both exclusions and equality at the endpoints for lists of 20, 21, 256, and 10,000 values. Agreed that spelling out the unbounded-interval interpretation would improve the comment.
| Ok(DataType::Boolean) | ||
| } | ||
|
|
||
| fn nullable(&self, _input_schema: &Schema) -> Result<bool> { |
There was a problem hiding this comment.
IN_SET_INTERSECTS returns NULL for the degenerate min > max case and for unknown bounds, which pruning should treat as "cannot rule out" and keep the container. Is that pinned down by a test end-to-end — a container whose statistics are inverted or missing, asserted to be kept rather than pruned? string_in_list_pruning.rs covers a lot of ground and I couldn't tell whether the inverted-statistics case is among them.
There was a problem hiding this comment.
Yes at the final container-decision level, although these cases are in pruning_predicate.rs, not the Parquet integration module:
large_string_in_list_handles_unicode_and_unknown_boundsincludes inverted boundsmin="z", max="m"and asserts that the container is kept.large_string_in_list_preserves_dictionary_nullschecks missing bounds from NULL keys/values and inverted boundsmin="zz", max=""; those containers are also kept.
Both call PruningPredicate::prune and assert the final Boolean decisions, so they cover UNKNOWN becoming “keep,” rather than only checking the expression's NULL result.
There is not a dedicated full Parquet scan test with an inverted footer. Separately, the compact page-order regression reads a real Parquet fixture and verifies that matching rows survive missing/unknown statistics ordering.
| } | ||
| } | ||
| if let Some(in_list) = expr.downcast_ref::<phys_expr::InListExpr>() { | ||
| if in_list.list().len() > MAX_IN_LIST_SIZE |
There was a problem hiding this comment.
The window is len() > MAX_IN_LIST_SIZE && len() <= max_in_list_size, so lists at or below the default keep the per-value path. Since the compact form looks both cheaper and (as far as I can tell) exactly as precise as the OR tree, what's the reason for keeping the lower bound rather than using the compact form for every eligible positive string list? If it's to avoid perturbing existing plans and EXPLAIN output for small lists, that would be worth saying in the comment.
There was a problem hiding this comment.
The lower bound preserves the existing pruning-expression representation for lists of at most 20 values and keeps this PR focused on the cost of large lists when callers explicitly raise the cap. The value 20 comes from the existing default; it is not an established performance crossover.
Using the compact form for smaller eligible string lists is worth considering separately. The current benchmark keeps the 20-value case on the legacy path, so it does not establish that compact construction/normalization is cheaper across the small-list cases. A focused comparison could justify broadening the optimization later. Agreed that the scope/default-behavior rationale should be stated next to this condition.
|
@adriangb cc |
| } | ||
|
|
||
| /// Create a page filter using the same `IN (...)` limit as row-group pruning. | ||
| #[expect(clippy::needless_pass_by_value)] |
There was a problem hiding this comment.
do we need to pass schema by val?
There was a problem hiding this comment.
Good point, I'll change it to pass &SchemaRef instead.
There was a problem hiding this comment.
Addressed in df65b7c. new_with_max_in_list_size now takes &SchemaRef, and the opener passes its existing reference. The public new signature is unchanged.
I also added the agreed ordering, single-bound, and threshold comments. Formatting, full all-targets/all-features Clippy with warnings denied, and ./dev/rust_lint.sh passed, as did 10,796 Rust tests (8 ignored) and all 505 SQL logic files. The extended rerun used a 65,536 open-file limit after the initial run hit the environment's 1,024 limit.
| /// The default (see [`MAX_IN_LIST_SIZE`]) preserves the | ||
| /// historical behaviour. Callers wiring config through can override via | ||
| /// `datafusion.execution.max_in_list_size`. | ||
| /// `datafusion.execution.parquet.max_in_list_size`. |
Yes, as
Therefore this is especially useful when we want to push down a large number of sparse IDs or keys to Parquet. |
Thanks for the ping @alamb. I took a high level look at this and I don't see any conflict with #19241: rather it's complimentary as @viirya noted. |
| use datafusion_physical_expr::{PhysicalExpr, PhysicalExprRef}; | ||
| use datafusion_physical_plan::ColumnarValue; | ||
|
|
||
| /// Tests whether a sorted string domain intersects an inclusive statistics interval. |
There was a problem hiding this comment.
minor documentation suggestion: could the type-level comment mention that evaluate returns one Boolean per min/max statistics interval? IIUC, true means the IN set intersects the interval, false it is disjoint, and NULL means the bounds are incomplete or invalid.
| batch.num_rows(), | ||
| )))); | ||
| } | ||
| // Dictionary values can be NULL behind valid keys. Preserve their |
There was a problem hiding this comment.
My Codex review pointed me to apache/arrow-rs#10510 which seems to fix this issue. Can we just add a short TODO to review again once the Arrow dependency is upgraded?
adriangb
left a comment
There was a problem hiding this comment.
This is a very cool change! Did you base this on some existing system / literature or is it a novel optimization?
There's a couple interesting things worth discussing with the limits:
- The PR / documentation could be a bit clearer around what the limits are and how things behave after this PR. My understanding is that the regime is now
len() < min(20, limit) is always dense,min(20, limit) < len() < limituses the new representation if the array is strings,limit < len()disables pruning completely. I had to think about it / read the code and description a couple of times and ask an agent to verify. - There is a footgun in the
min(20, limit) < len() < limitregime for non-strings: we'd still build the large OR tree for e.g.x in (1, 2, 3..., N)(more on this below). So a user setting a larger limit so that they can use this optimization is exposed to degenerate behavior that they may not control (if they run user provided queries). - The proposal discards the option of
vmin <= c1_max AND c1_min <= vmaxciting that it would loose pruning power. This is true, but in cases like ULIDs the remaining pruning power might still do a lot of work. And at thelen() > limitregime which currently has no pruning coverage it's pretty much a free win (it's O(1) to evaluate and build no matter the length of the list).
Regarding type support: is there any reason we don't support other types? Ints in particular seem straightforward to support. Byte types are pretty much an identical implementation to string types. Would you plan these as followups or are there reasons to not implement them?
Regarding NOT IN and NULL-containing lists: is there a fix we could make on main to allow these to be supported? We have some special handling of nulls in identify_fully_matched_row_groups, we could expand it. I'm not sure if that is the source of the issue or it's more widespread.
| max_in_list_size: usize, | ||
| ) -> Arc<PagePruningAccessPlanFilter> { | ||
| Arc::new(PagePruningAccessPlanFilter::new( | ||
| Arc::new(PagePruningAccessPlanFilter::new_with_max_in_list_size( |
There was a problem hiding this comment.
This seems to me like the kind of thing where we would want a PagePruningAccessPlanFilterBuilder in the future if we add any more options / constructors. But maybe this is fine for now.
| 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) |
There was a problem hiding this comment.
Indeed seems like this is all a thin wrapper around a builder, might be a sign...
| pub(crate) struct StringInListPruningExpr { | ||
| min: PhysicalExprRef, | ||
| max: PhysicalExprRef, | ||
| values: Arc<[String]>, |
There was a problem hiding this comment.
Have we analyzed if this is the best representation? E.g. could we use a StringArray instead?
| impl PartialEq for StringInListPruningExpr { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| self.min.eq(&other.min) && self.max.eq(&other.max) && self.values == other.values | ||
| } | ||
| } | ||
|
|
||
| impl Hash for StringInListPruningExpr { | ||
| fn hash<H: Hasher>(&self, state: &mut H) { | ||
| self.min.hash(state); | ||
| self.max.hash(state); | ||
| self.values.hash(state); | ||
| } | ||
| } |
There was a problem hiding this comment.
These both walk every value. It might be worth measuring what impact that has at the extremes, it would depend on how much these expression trees are walk / if there's any dedup or expression optimization run on them. We could always build a digest of the values upfront. Probably fine to leave for now.
comphead
left a comment
There was a problem hiding this comment.
Nice PR, thanks @sunchao
I would be honest, I checked the same in DuckDB and seems this implementation is better
┌──────────────────────┬─────────────────────────────────────────────────────────────────────────────────────────┬──────────────────────────────────────────────────────┐
│ │ DuckDB │ DataFusion PR │
├──────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────┼──────────────────────────────────────────────────────┤
│ Container test │ Inclusive [min,max] byte-interval membership per constant │ Same interval, tested via sorted-domain intersection │
├──────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────┼──────────────────────────────────────────────────────┤
│ Order │ Unsigned memcmp + length tiebreak (string_type.hpp:217) │ Rust &[u8] Ord │
├──────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────┼──────────────────────────────────────────────────────┤
│ IN evaluation │ Linear scan of the list per container, 2 memcmp/value, early-out (string_stats.cpp:704) │ Sort+dedup once, one partition_point per container │
├──────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────┼──────────────────────────────────────────────────────┤
│ Cost per container │ O(n) │ O(log n) │
├──────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────┼──────────────────────────────────────────────────────┤
│ Collation in zonemap │ None (byte-wise) │ None (byte-wise) │
└──────────────────────┴─────────────────────────────────────────────────────────────────────────────────────────┴──────────────────────────────────────────────────────┘
viirya
left a comment
There was a problem hiding this comment.
Took another pass over the latest head (df65b7cc). Two of the three comments from the earlier threads have landed — the unsigned-byte-order assumption at the partition_point call and the unbounded-interval reasoning on the single-bound arms both read clearly now.
I also went and checked the ordering-gate claim from our earlier thread rather than leaving it as a question, and it holds: RowGroupPruningStatistics::min_values/max_values route through mask_untrusted_byte_array_stats, which nullifs untrusted byte-array bounds per row group, and has_untrusted_min_max_order withholds the column entirely. So the compact expression really does read the same gated columns as the per-value path, and unusable bounds arrive as NULL rather than as misordered values. Thanks for the pointers — that was the part I couldn't tell from the diff alone.
Worth noting for other reviewers: LiteralGuarantee::analyze runs on the original predicate, so bloom-filter / contained() pruning is unaffected by the rewrite. Given #8668 is in the same neighbourhood, that seemed worth stating explicitly.
Overall this looks ready to me. The algorithm is precision-equivalent to the OR tree (sorted deduped domain plus a binary search for the first value at or above min, then one comparison against max), the eligibility gate is conservative on every axis I checked (positive IN only, string or dictionary-of-string only, any non-string literal in the list bails out, column name must match), and every degenerate case I could think of resolves to "keep the container": inverted bounds, missing bounds, dictionary values that are NULL behind valid keys, and string buffers too large for Utf8View's offsets. The tests cover those, and the correctness prerequisite (#24525) is already merged. The page-level max_in_list_size fix is a nice bonus — that path was silently using the default before.
Approving. Two comment-level follow-ups inline, neither blocking. Separately, codecov reports 73% patch coverage on string_in_list.rs; from reading the tests the risky branches (inverted, missing, oversized, dictionary nulls) are covered, so I assume the gap is boilerplate like Display/Hash/with_new_children, but flagging it in case something substantive is untested.
| return None; | ||
| } | ||
| // Rust string ordering and these byte comparisons both use | ||
| // unsigned lexicographic UTF-8 order. Statistics providers |
There was a problem hiding this comment.
This comment states a contract on statistics providers generally, but the enforcement it points at is Parquet-specific. I checked the other providers that can reach this expression — eligibility keys only on the schema type being a string, so it carries no knowledge of who supplied the bounds:
PartitionPruningStatistics— bounds are partition values DataFusion produced itself, so they are in Arrow order by construction.PrunableStatistics(built infile_pruner.rsfrom a file'sColumnStatistics) — bounds come from whatever theTableProviderreported, with no ordering gate on that path.
The exposure isn't new; the per-value min <= v AND v <= max path has the same dependency. But since this is the first place the requirement is written down, the comment should say which providers are known to satisfy it and which are taken on trust — as written it reads as if the invariant is enforced everywhere. (If you think it belongs closer to the source, PruningStatistics::min_values might be the better home for it.)
| // 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 |
There was a problem hiding this comment.
This is the one remaining item from the earlier thread — you agreed the scope/default-behaviour rationale should sit next to this condition, and it doesn't appear to be in df65b7cc yet. A sentence saying the lower bound is a scope/compatibility choice rather than a measured threshold would stop the next reader assuming 20 is tuned.
Why are the changes needed?
Which issue does this PR close?
Related to #8668 and #8609; follows #24074.
This PR is stacked on #24525, which must merge first.Rationale for this change
Queries often select a sparse set of string identifiers. Parquet min/max statistics can make these queries much cheaper by ruling out row groups or pages that cannot contain any requested identifier. For example, this query asks for 21 IDs, spaced ten apart:
A row group whose values fall between
id003andid007cannot contain a match. The default pruning limit is 20, so this list is not eligible for theINmin/max rewrite unless the caller raises the limit. #24074 made that limit configurable. With a raised limit, DataFusion can already reject this row group, but it does so by constructing a growing expression tree resembling:For hundreds or thousands of identifiers, building and evaluating that tree can become expensive in its own right. Replacing the list with one enclosing range,
[id000, id200], would be cheaper, but would lose the gaps: that broad range overlaps[id003, id007]even though none of the requested IDs is present there.The aim is to keep the useful pruning precision of the existing per-value checks while making large lists cheaper to represent and evaluate. In the included local microbenchmark, evaluating 1,024 values against 4,096 intervals falls from 68.8 ms to 0.198 ms. This measures pruning work only, not end-to-end query speedup.
What changes were proposed in this PR?
What changes are included in this PR?
Eligible large string lists are stored as a sorted, deduplicated set of values inside the pruning predicate, rather than expanded into one comparison branch per value. For each inclusive statistics interval, DataFusion finds the first requested value at or after the interval's minimum, then checks whether that value is also at or before its maximum.
In the example, the first requested ID at or after
id003isid010. Sinceid010 > id007, the row group can be skipped. An interval such as[id019, id021]must be kept because it could containid020. This takes a binary search per interval after sorting the values once per constructed predicate, and the expression tree no longer grows with the number of IDs.The result remains a conservative pruning decision. An overlapping interval means only that a match is possible; the original
INexpression still performs exact row filtering. The original literal information also remains available to other pruning mechanisms, including Bloom filters. Missing or inverted bounds cannot prove that data is safe to skip.The existing limit continues to control eligibility. Its default remains 20, setting it to zero disables the
INmin/max rewrite, and lists beyond the configured cap remain ineligible. Only eligible positive, non-null literal string lists larger than 20 take the compact path.NOT IN, NULL-containing lists, and unsupported expressions keep their existing handling. Page-index pruning now receives the same configured cap as row-group pruning, so raising the limit can benefit both.The dependency on #24525 matters for correctness: an interval search is only meaningful when the stored bounds use the same comparison order as the query. That companion PR handles legacy or unrecognized Parquet byte-array ordering. It must land before the newly enabled large-list page-pruning path here.
Are there any user-facing changes?
Users who raise
datafusion.execution.parquet.max_in_list_sizeget cheaper min/max pruning for eligible large string lists, and page pruning now honors that setting. The configuration default, exact query results, and existing public APIs are unchanged. This is a focused optimization for literal string lists, not a general rewrite of every largeINexpression.How was this PR tested?
Are these changes tested?
The pruning-crate suite passed 93 tests. The standalone Parquet regressions use lists of 20, 21, 256, and 1,024 values and check both exact query results and scan/pruning metrics. They cover gaps inside the list's enclosing range, row-group pruning, page-only pruning, and the default and zero-cap controls.
Two correctness regressions exercise the less obvious interactions. A direct physical-source test uses
NOT IN (..., NULL), row-filter pushdown, andLIMIT 1, so logical optimizer folding cannot hide an incorrect decision to bypass filtering. A real-file test combines this PR with #24525 and verifies that compact page pruning cannot lose a matching row when the footer's ordering is missing or unknown. Against unchanged Apachef1f0449a, the positive row-group/page tests fail as expected; theNOT IN (..., NULL)control passes.The benchmark compares the actual raised-cap
INpath on Apachef1f0449aand this patch, using separate build directories and checking that both return the same nontrivial pruning results. Local results on an Apple M5 Max (18 CPUs, 128 GiB), Rust 1.97.0,release-nonlto, 20 samples:A balanced explicit OR tree is included as another comparison: at 1,024 values it takes 8.03 ms to evaluate the same intervals. These measurements isolate pruning overhead; no end-to-end workload improvement is claimed.
Formatting, all-targets/all-features Clippy with warnings denied, and
./dev/rust_lint.shpassed on the combined stack. The extended workspace run passed 10,674 Rust tests, with eight ignored, and all 503 SQL-logic files.Validation commands