Exp/r6 pruningpredicate rates#13
Open
adriangb wants to merge 18 commits into
Open
Conversation
github-actions
Bot
added
documentation
Owner
Author
|
run benchmarks baseline:
ref: main
env:
DATAFUSION_EXECUTION_PARQUET_PUSHDOWN_FILTERS: false
DATAFUSION_EXECUTION_PARQUET_REORDER_FILTERS: false
changed:
ref: HEAD
env:
DATAFUSION_EXECUTION_PARQUET_PUSHDOWN_FILTERS: true
DATAFUSION_EXECUTION_PARQUET_REORDER_FILTERS: true |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing HEAD (ada0472) to main diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing HEAD (ada0472) to main diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing HEAD (ada0472) to main diff using: tpch File an issue against this benchmark runner |
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
…UE window functions (apache#22112) ## Which issue does this PR close? - Closes apache#22108 ## Rationale for this change lead and lag both preserve metadata from the input field but nth_value, first_value, and last_value do not. ## What changes are included in this PR? The mechanism to calcluate the return field for nth_value was changed to match lead/lag (which had already been fixed). ## Are these changes tested? Yes, I added tests to metadata.slt ## Are there any user-facing changes? No
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Closes apache#22137 . ## Rationale for this change `RANGE` window frames with a value offset (e.g. `RANGE BETWEEN 4 PRECEDING AND 4 FOLLOWING`) panicked with `attempt to add/subtract with overflow` whenever the boundary target (`value ± delta`) wrapped past the type's representable range. Affected inputs include values close to `i64::MAX`/`i64::MIN`, `u64::MAX`, and any analogous boundary for other integer/decimal/timestamp types. Two `// TODO: Handle ... overflows.` markers in `WindowFrameStateRange::calculate_index_of_row` had been left for this case; the unchecked `ScalarValue::add` / `sub` silently wrapped the target, after which `search_in_slice` was handed a nonsensical (wrapped) value and downstream code tripped a debug-assert subtraction in `functions-window/src/nth_value.rs`. Semantically, an overflowed boundary is *unbounded* with respect to the data in the partition — every real value lies strictly inside the wrapped sentinel — so the correct behavior is to collapse the search to the appropriate partition edge rather than to search with a wrapped target. ## What changes are included in this PR? `datafusion/expr/src/window_state.rs` - Replace `ScalarValue::add` / `sub` with their `*_checked` counterparts in the boundary computation. - On overflow, short-circuit to the correct partition edge: `search_start` for `PRECEDING`-direction searches, `length` for `FOLLOWING`-direction searches. The collapse direction depends only on the const-generic `SEARCH_SIDE` (the add branch and sub branch both reduce to `!SEARCH_SIDE` once you expand the `SEARCH_SIDE == is_descending` invariant that selects each arithmetic branch). - The pre-existing `value.is_unsigned() && value < delta` clamp-to-zero path for unsigned subtraction is preserved — it produces a valid polymorphic zero, not an overflow sentinel. - No behavior change on the non-overflow path. `datafusion/sqllogictest/test_files/window.slt` Regression coverage for positive and negative overflow, across: - `ASC` + `FOLLOWING` / `ASC` + `PRECEDING` / `DESC` + `PRECEDING` / `DESC` + `FOLLOWING` (each overflow direction occurs on both sort orders depending on which arithmetic branch is taken) - Symmetric `N PRECEDING AND N FOLLOWING` frames where only one side overflows - Signed (`i64`) and unsigned (`u64`) ordering columns - `first_value` and `last_value` both exercised to verify both frame edges - `ROWS` frame regression guard to document that the pre-existing `saturating_sub` / `min(length)` saturation behavior is unchanged.
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Closes apache#22138 . ## Rationale for this change `AVG` used as a window aggregate can return `NaN` (and, for `Decimal` / `Duration`, panic on integer division by zero) when every value in the window frame is NULL. ```sql SELECT i, AVG(v) OVER (ORDER BY i ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING) FROM (VALUES (1,1), (2,2), (3,NULL), (4,NULL)) t(i,v); ``` | i | current output | expected (DuckDB/PgSQL) | |---|----------------|-------------------| | 1 | 1.5 | 1.5 | | 2 | 2.0 | 2.0 | | 3 | **NaN** | **NULL** | | 4 | **NaN** | **NULL** | Root cause: sliding-window execution calls `Accumulator::retract_batch` as rows leave the frame. Once every contributing value has been retracted, `self.count` drops back to `0` but `self.sum` stays `Some(0.0)` (or a tiny floating-point residual). `evaluate()` then computes `sum / 0`, which yields `NaN` on `Float64`, and would panic with integer division by zero on `DecimalAvgAccumulator` and `DurationAvgAccumulator`. The non-sliding aggregation path is unaffected because there `sum` becomes `Some(_)` only after at least one non-NULL value has been added, so `count == 0` implies `sum == None`. ## What changes are included in this PR? `datafusion/functions-aggregate/src/average.rs` — guard all three affected `evaluate()` implementations with an explicit `count == 0 → None` short-circuit: - `AvgAccumulator::evaluate` (Float64) - `DecimalAvgAccumulator::evaluate` (Decimal32/64/128/256) - `DurationAvgAccumulator::evaluate` (Duration*) This matches the idiom already used by sibling retractable accumulators (`variance.rs` uses an explicit `match self.count` before division; `sum.rs` uses a `(self.count != 0).then_some(..)` guard).
Owner
Author
|
run benchmarks baseline:
ref: main
env:
DATAFUSION_EXECUTION_PARQUET_PUSHDOWN_FILTERS: false
DATAFUSION_EXECUTION_PARQUET_REORDER_FILTERS: false
changed:
ref: HEAD
env:
DATAFUSION_EXECUTION_PARQUET_PUSHDOWN_FILTERS: true
DATAFUSION_EXECUTION_PARQUET_REORDER_FILTERS: true |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing HEAD (cae5fe6) to main diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing HEAD (cae5fe6) to main diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing HEAD (cae5fe6) to main diff using: tpch File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
## Which issue does this PR close? Part of apache#22091. ## Rationale for this change Current async udfs in lambdas fail with generic errors ## What changes are included in this PR? Report an explicit error when trying to create a lambda with async functions ## Are these changes tested? One sqllogictest added to assert the friendly error ## Are there any user-facing changes? What failed before still fail but with a better error
## Which issue does this PR close? - Closes apache#22135 ## Rationale for this change `FFI_ExecutionPlan` exposes most of the `ExecutionPlan` trait but does not expose `metrics()`. As a result, `ForeignExecutionPlan::metrics()` falls through to the trait default (`None`), so anything downstream of an FFI boundary loses metrics. The most visible breakage is `EXPLAIN ANALYZE`, which renders empty metric blocks for foreign plans; anything calling `DisplayableExecutionPlan::with_metrics(...)` on a plan tree containing foreign nodes is similarly affected. This PR makes foreign plans behave the same as local plans for metric reporting. Metrics are passed as a snapshot, and all atomic-backed counters/gauges/timers are read into plain integer fields at marshal time. Correct because none of the in-tree consumers (`AnalyzeExec`, `DisplayableExecutionPlan`) poll metrics during streaming. ## What changes are included in this PR? - New module `datafusion/ffi/src/metrics.rs` with FFI-stable mirrors of `MetricsSet`, `Metric`, `MetricValue` (all 16 variants), `Label`, `MetricType`, `MetricCategory`, `PruningMetrics`, `RatioMetrics`, and `RatioMergeStrategy`, plus bidirectional `From` conversions. - `MetricValue::Custom { value: Arc<dyn CustomMetricValue> }` is marshalled as `(name, Display output, as_usize())`. On the consumer side it is reconstructed as a small `FfiCustomMetricValue` shim that preserves `Display` and `as_usize()`. `aggregate` becomes a no-op (snapshots are not mergeable) and `as_any` only downcasts to the shim — this is the documented compromise. - `FFI_ExecutionPlan` gains a new `metrics` function pointer (appended after `repartitioned`). `ForeignExecutionPlan::metrics()` is implemented to call through it. - Two trivial accessors added to `RatioMetrics`: `merge_strategy()` and `display_raw_values()` — needed to marshal these otherwise-private fields. - `chrono` added as a direct dependency of `datafusion-ffi` (used for `Timestamp` ↔ unix-nanos conversion). ## Are these changes tested? Yes. New tests, all passing: - 7 unit tests in `datafusion/ffi/src/metrics.rs` round-trip every `MetricValue` variant individually, plus a full `Metric` (value + labels + partition + type + category) and a `MetricsSet`. - `test_ffi_execution_plan_metrics_round_trip` in `datafusion/ffi/src/execution_plan.rs` exercises the full FFI path: builds an `ExecutionPlan` with a `MetricsSet`, wraps it in `FFI_ExecutionPlan`, retrieves metrics via `ForeignExecutionPlan::metrics()` through `mock_foreign_marker_id`, and asserts the aggregated value matches. - `EmptyExec` test helper extended with `with_metrics(MetricsSet)`. Existing test suites still pass: `cargo test -p datafusion-ffi --all-features` and `cargo test -p datafusion-ffi --features integration-tests`. ## Are there any user-facing changes? Yes — this PR adds public API and makes a binary-incompatible change to `FFI_ExecutionPlan`. Please add the `api change` label. - **New public types** in `datafusion_ffi::metrics`: `FFI_MetricsSet`, `FFI_Metric`, `FFI_MetricValue`, `FFI_Label`, `FFI_MetricType`, `FFI_MetricCategory`, `FFI_PruningMetrics`, `FFI_RatioMetrics`, `FFI_RatioMergeStrategy`, and `FfiCustomMetricValue`. - **ABI break for `FFI_ExecutionPlan`**: a new `metrics` function pointer field is appended. Producers and consumers must be rebuilt together, as is already enforced by the major-version check via `datafusion_ffi::version()`. - **New public accessors** on `RatioMetrics`: `merge_strategy()` and `display_raw_values()`. Non-breaking additions. - **`MetricValue::Custom` across FFI is lossy by design**: the underlying `dyn CustomMetricValue` is not preserved; only its `Display` output and `as_usize()` snapshot survive. Documented on `FfiCustomMetricValue`.
…22029) ## Which issue does this PR close? - Closes apache#21997 (potentially). ## Rationale for this change This PR adds two new APIs to `GenericStringArrayBuilder` and `StringViewArrayBuilder`: 1. `append_with` appends a row whose bytes are produced by invoking a closure that is passed a `StringWriter` 2. `append_byte_map` appends a row whose bytes are produced by mapping each byte of the input with a byte-to-byte map closure. For `StringViewArrayBuilder`, `StringWriter` is an append-only string writer that switches between writing to a new inline view (for short strings) or to the in-progress data block automatically. For `GenericStringArrayBuilder`, `StringWriter` just appends to the value buffer directly. (We need two new APIs because `append_byte_map` vectorizes a lot better than `append_with`, so callers that fit the byte-to-byte map pattern should prefer it.) Both of these new APIs allow string UDFs to avoid creating an intermediate data copy in many cases. To illustrate this, this PR adopts the new APIs in `replace`. Benchmarks (Arm64): Group 1: ASCII single-byte fast path (StringArray) - size=1024 str_len=32 nulls=0.0 : 16.27 µs -> 12.83 µs (−21.1%) - size=1024 str_len=32 nulls=0.2 : 14.23 µs -> 12.10 µs (−15.0%) - size=1024 str_len=128 nulls=0.0 : 11.28 µs -> 8.21 µs (−27.3%) - size=1024 str_len=128 nulls=0.2 : 10.37 µs -> 7.79 µs (−24.9%) - size=4096 str_len=32 nulls=0.0 : 62.48 µs -> 49.50 µs (−20.8%) - size=4096 str_len=32 nulls=0.2 : 55.74 µs -> 46.66 µs (−16.3%) - size=4096 str_len=128 nulls=0.0 : 42.26 µs -> 29.06 µs (−31.2%) - size=4096 str_len=128 nulls=0.2 : 39.17 µs -> 28.52 µs (−27.2%) Group 2: Multi-byte StringArray — general writer path - size=1024 str_len=32 nulls=0.0 : 23.58 µs -> 21.75 µs (−7.8%) - size=1024 str_len=32 nulls=0.2 : 18.92 µs -> 17.41 µs (−8.0%) - size=1024 str_len=128 nulls=0.0 : 37.56 µs -> 35.33 µs (−5.9%) - size=1024 str_len=128 nulls=0.2 : 29.62 µs -> 28.71 µs (−3.1%) - size=4096 str_len=32 nulls=0.0 : 97.15 µs -> 88.92 µs (−8.5%) - size=4096 str_len=32 nulls=0.2 : 77.03 µs -> 71.43 µs (−7.3%) - size=4096 str_len=128 nulls=0.0 : 173.66 µs -> 163.68 µs (−5.7%) - size=4096 str_len=128 nulls=0.2 : 134.98 µs -> 128.56 µs (−4.8%) Group 3: Multi-byte StringViewArray — general writer path - size=1024 str_len=32 nulls=0.0 : 24.46 µs -> 22.18 µs (−9.3%) - size=1024 str_len=32 nulls=0.2 : 20.04 µs -> 17.71 µs (−11.7%) - size=1024 str_len=128 nulls=0.0 : 36.43 µs -> 35.79 µs (−1.8%) - size=1024 str_len=128 nulls=0.2 : 29.73 µs -> 28.70 µs (−3.5%) - size=4096 str_len=32 nulls=0.0 : 99.07 µs -> 89.68 µs (−9.5%) - size=4096 str_len=32 nulls=0.2 : 84.38 µs -> 72.46 µs (−14.1%) - size=4096 str_len=128 nulls=0.0 : 169.27 µs -> 164.80 µs (−2.6%, n.s.) - size=4096 str_len=128 nulls=0.2 : 133.79 µs -> 130.20 µs (−2.7%, n.s.) Group 4: Empty-from StringArray - size=1024 str_len=32 : 87.75 µs -> 50.64 µs (−42.3%) - size=1024 str_len=128 : 313.00 µs -> 187.77 µs (−40.0%) Group 5: Empty-from StringViewArray - size=1024 str_len=32 : 87.01 µs -> 50.10 µs (−42.4%) - size=1024 str_len=128 : 313.99 µs -> 190.17 µs (−39.4%) ## What changes are included in this PR? * Add `append_byte_map` and `append_with` to both of the bulk-NULL string builders * Add unit tests * Adopt the new APIs in `replace` ## Are these changes tested? Yes; new tests added. ## Are there any user-facing changes? No.
## Which issue does this PR close? - Closes #. ## Rationale for this change `rand()` is a common alias for `random()` in SQL engines. Supporting it improves compatibility and lets users write `rand()` as an equivalent zero-argument volatile random function. ## What changes are included in this PR? - Adds `rand` as an alias for the existing `random()` scalar function. - Adds a sqllogictest case verifying that `rand()` resolves successfully and returns a `Float64` value in the expected `[0, 1)` range. ## Are these changes tested? Yes. - `cargo fmt --all` - `cargo test --package datafusion-functions random --lib` - `cargo test --features backtrace,parquet_encryption --profile ci --package datafusion-sqllogictest --test sqllogictests -- functions.slt` ## Are there any user-facing changes? Yes. Users can now call `rand()` as an alias for `random()`.
Bumps [runs-on/action](https://github.com/runs-on/action) from 2.1.0 to 2.1.2. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/runs-on/action/releases">runs-on/action's releases</a>.</em></p> <blockquote> <h2>v2.1.2</h2> <p><strong>Full Changelog</strong>: <a href="https://github.com/runs-on/action/compare/v2.1.1...v2.1.2">https://github.com/runs-on/action/compare/v2.1.1...v2.1.2</a></p> <h2>v2.1.1</h2> <h2>What's Changed</h2> <ul> <li>Upgrade go, deps, and sanitize values better by <a href="https://github.com/crohr"><code>@crohr</code></a> in <a href="https://redirect.github.com/runs-on/action/pull/30">runs-on/action#30</a></li> <li>Propagate actions runtime token by <a href="https://github.com/crohr"><code>@crohr</code></a> in <a href="https://redirect.github.com/runs-on/action/pull/34">runs-on/action#34</a></li> </ul> <p><strong>Full Changelog</strong>: <a href="https://github.com/runs-on/action/compare/v2.1.0...v2.1.1">https://github.com/runs-on/action/compare/v2.1.0...v2.1.1</a></p> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/runs-on/action/commit/d141ef83eb66d096ce8afc767e09115a65c63b60"><code>d141ef8</code></a> dist: rebuild binaries for v2.1.2</li> <li><a href="https://github.com/runs-on/action/commit/c5df5533f2cf2dec19ef109dcd2a6d28fae8928f"><code>c5df553</code></a> Add manual release workflow with gpg signing and checksums</li> <li><a href="https://github.com/runs-on/action/commit/e46a3c6d62a5df0b0e0f8b5fdc50f04c5ecf147f"><code>e46a3c6</code></a> dist: rebuild binaries</li> <li><a href="https://github.com/runs-on/action/commit/88629fc77cb7a6a251ccdf8290a55cd08928794e"><code>88629fc</code></a> Send runtime token to Magic Cache config</li> <li><a href="https://github.com/runs-on/action/commit/6e9cb2b901c5953075dbcd45edf77b0192c8e6b1"><code>6e9cb2b</code></a> Update actions</li> <li><a href="https://github.com/runs-on/action/commit/408de89233b56a743b98d68195bb1923d60543aa"><code>408de89</code></a> dist: rebuild binaries</li> <li><a href="https://github.com/runs-on/action/commit/e8a2e6d65ae450e212faf12b84577c012cf764ab"><code>e8a2e6d</code></a> Remove dead code: unused MetricSummary fields and calculateMin/calculateMax f...</li> <li><a href="https://github.com/runs-on/action/commit/3a86586805f7eeb1ef42293156a97c326ff040fd"><code>3a86586</code></a> dist: rebuild binaries</li> <li><a href="https://github.com/runs-on/action/commit/61a7be1daf74b5271d1238f1d4158fa3a5545ae6"><code>61a7be1</code></a> build: upgrade to go 1.26</li> <li>See full diff in <a href="https://github.com/runs-on/action/compare/742bf56072eb4845a0f94b3394673e4903c90ff0...d141ef83eb66d096ce8afc767e09115a65c63b60">compare view</a></li> </ul> </details> <br /> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
… bits (apache#22158) ## Which issue does this PR close? N/A ## Rationale for this change 1. instead of counting set bits to check if there is at least 1 set bits, we can use the existing helpers on `BooleanArray` that check if there is at least 1 set bit 2. Avoid unnecessary `BooleanBuffer` bitwise operations and reuse mask ## What changes are included in this PR? reused mask, and use helper to check if at least one false ## Are these changes tested? Existing tests ## Are there any user-facing changes? No ------ Cc @gstvg, @comphead
… hashing work during physical planning. (apache#22175) ## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Closes apache#22173. ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> Explained in issue ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> 1. Removed derived Hash & Eq 2. implemented manual Hash & Eq 3. manual impl excludes redudant & recursive hash `eval_method` field ## Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> A unit test was added ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> Pathologically nested CASE/WHEN queries will plan significantly faster. <!-- If there are any breaking changes to public APIs, please add the `api change` label. --> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Closes apache#22121 ## Rationale for this change `array_replace.rs` already covers `array_replace`, `array_replace_n`, and `array_replace_all` through the public UDF path, and includes the relevant int64 benchmark sizes plus broader nested, string, boolean, and fixed-size-binary cases. Keeping both benchmark targets would duplicate maintenance and make ownership unclear. <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> ## What changes are included in this PR? This removes the stale `array_expression` benchmark target. The dedicated `array_replace` benchmark remains the single benchmark owner for array replacement performance. <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> ## Are these changes tested? Yes, locally <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> ## Are there any user-facing changes? No <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please add the `api change` label. -->
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Closes apache#22113 . ## Rationale for this change `RANGE` window frames with a `DECIMAL` `ORDER BY` column crash at runtime: ```sql SELECT COUNT(*) OVER ( PARTITION BY 1 ORDER BY cast(1 as decimal(10, 0)) DESC RANGE BETWEEN CURRENT ROW AND 1 FOLLOWING ) FROM (SELECT 1); -- Internal error: Uncomparable values: Decimal128(Some(0),11,0), Decimal128(Some(1),10,0). ``` Frame-bound arithmetic (`value ± delta`) widens the decimal result precision by 1 (`Decimal(10,0) ± Decimal(10,0) → Decimal(11,0)`). `search_in_slice` then compares the widened target against the original `ORDER BY` column, and `ScalarValue::partial_cmp` rejects decimals whose precision differs — even when the scale matches and the underlying integer representation is directly comparable. That precision-equality gate is also inconsistent with SQL semantics: `DEC(10,0) 1` and `DEC(20,0) 1` represent the same number and should compare equal. ## What changes are included in this PR? **`datafusion/common/src/scalar/mod.rs`** - `ScalarValue::partial_cmp` for `Decimal32` / `Decimal64` / `Decimal128` / `Decimal256`: compare underlying values whenever scales match, regardless of declared precision. Different scales still return `None` (rescaling would be required). **`datafusion/sqllogictest/test_files/window.slt`** - Regression block covering the reporter query verbatim plus `ASC`/`DESC` × `PRECEDING`/`FOLLOWING`, symmetric `N PRECEDING AND N FOLLOWING`, and a non-zero-scale `DECIMAL(10,2)` case over multi-row partitions.
…2101) ## Which issue does this PR close? Part of apache#21172 ## Rationale for this change This has been pulled out from main lambda PR to reduce it's size. Currently, the `LambdaVariable` field is an `Option<FieldRef>` but must be set manually for `expr_api` users (see below, passing `None` would result in planning/execution errors). This is similar to `Expr::Placeholder` field and its helper `Expr::infer_placeholder_types` https://github.com/apache/datafusion/blob/7f2f78d48b6d3d6aee2ce2fd29910bb4c11b1012/datafusion/functions-nested/src/array_transform.rs#L328-L341 Now it can be omitted and later resolved by the new helpers: https://github.com/apache/datafusion/blob/7c266c10e806cf87edfe5a2eccccccf639fdb2fe/datafusion/functions-nested/src/array_transform.rs#L326-L330 ## What changes are included in this PR? This PR adds the method `resolve_lambda_variables` to `Expr` and `LogicalPlan` to automatically resolve all variables of the given expr or logical plan. Nothing changes for sql planning ## Are these changes tested? There's a unit test which should cover all edge cases. Is not trivial to test via sqllogictests because sql planning produces already resolved lambda variables. ## Are there any user-facing changes? New methods added only
…21637) ## Which issue does this PR close? - Closes apache#19028. ## Rationale for this change When DataFusion evaluates a Parquet scan with filter pushdown, it uses row group statistics to determine which row groups contain matching rows. The `RowGroupAccessPlanFilter` already tracks which row groups are "fully matched" — where statistics prove that **all** rows satisfy the predicate (via `is_fully_matched`). However, this information was not propagated downstream. Even for fully matched row groups: 1. **Page index pruning** still evaluated page-level statistics (wasted work since no pages can be pruned) 2. **RowFilter evaluation** still decoded filter columns and evaluated the predicate for every row (wasted work since every row passes) This is especially costly when filter columns are expensive to decode (e.g., large strings) or when predicates are complex. Common real-world examples include time-range filters where entire row groups fall within the range, or `WHERE status != 'DELETED'` on data with no deleted rows. ## What changes are included in this PR? ### DataFusion changes (this PR) 1. **`row_group_filter.rs`**: `RowGroupAccessPlanFilter::build()` now returns `(ParquetAccessPlan, Vec<usize>)` — the access plan plus the indices of fully matched row groups. 2. **`page_filter.rs`**: `prune_plan_with_page_index()` accepts a `fully_matched_row_groups` parameter and skips page-level pruning for those row groups. 3. **`opener.rs`**: Wires fully matched row groups through the pipeline — passes them to page pruning and to the `ParquetPushDecoderBuilder` via `with_fully_matched_row_groups()`. ### Arrow-rs dependency (apache/arrow-rs#9694) The new `ArrowReaderBuilder::with_fully_matched_row_groups()` API in arrow-rs allows skipping `RowFilter` evaluation during Parquet decoding for specified row groups. This PR uses `[patch.crates-io]` pointing to the arrow-rs fork branch until that PR is merged and released. ### Benchmark Includes a criterion benchmark (`parquet_fully_matched_filter`) using `ParquetPushDecoder` directly — the same code path DataFusion's async opener uses. Dataset: 20 row groups × 50K rows, with a 1KB string payload column and predicate `x < 200` (all row groups fully matched). | Scenario | Time | vs. baseline | |----------|------|-------------| | Filter pushdown, no skip | ~43 ms | baseline | | **Filter pushdown, with skip** | **~20 ms** | **~2.2x faster** | | No pushdown at all | ~24 ms | — | ## Are these changes tested? - All 82 existing non-submodule `datafusion-datasource-parquet` tests pass (16 failures are pre-existing, caused by missing `parquet-testing` submodule) - The benchmark verifies correctness by asserting the expected row count - Clippy and fmt pass ## Are there any user-facing changes? No user-facing API changes. This is a transparent performance optimization — queries that previously worked will now be faster when row group statistics prove all rows match the predicate. ~~**Note:** This PR depends on apache/arrow-rs#9694. The `[patch.crates-io]` in `Cargo.toml` will be removed once that arrow-rs change is released.~~ all logic is on df side now --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
## Which issue does this PR close? - Closes apache#22184. ## Rationale for this change The SQL planner should return an error for unsupported user SQL rather than panic. Unknown compound identifiers with six or more parts could reach an unchecked `unwrap()` after failing schema lookup. ## What changes are included in this PR? Updates compound identifier handling so unmatched identifiers with five or more parts return the existing `not_impl_err!` path instead of falling through to `form_identifier`. Adds a regression test for `SELECT a.b.c.d.e.f` to verify planning returns an unsupported compound identifier error. ## Are these changes tested? Yes: - `cargo fmt --all` - `cargo test -p datafusion-sql --test sql_integration select_unknown_deep_compound_identifier_returns_error` - `cargo clippy --all-targets --all-features -- -D warnings` ## Are there any user-facing changes? No API changes. Invalid deeply nested compound identifiers now return a planner error instead of panicking.
## Which issue does this PR close? - Closes: apache#22178 ## Rationale for this change Update the pinned Rust toolchain to 1.95.0 and keep the workspace passing the required formatting and clippy checks with the new toolchain. ## What changes are included in this PR? - Bump rust-toolchain.toml from 1.94.0 to 1.95.0. - Update the contributor docs rust-analyzer example to reference 1.95.0. - Apply the minimal clippy and compile fixes needed under Rust 1.95.0. ## Are these changes tested? - cargo fmt --all - cargo clippy --all-targets --all-features -- -D warnings ## Are there any user-facing changes? No user-facing runtime changes. This updates the project toolchain and related developer-facing docs.
## Which issue does this PR close? No issue reported ## Rationale for this change `Statistics::with_fetch` unconditionally returned `Precision::Exact(0)` when the input had `nr <= skip`, even when the input was `Inexact(nr)` — promoting an estimated upper bound into an exact zero. The exactness flag then misleads downstream consumers (notably `AggregateStatistics` via `Count::value_from_stats`) into trusting a derived "0" and folding the count subtree to a literal. Concrete user-visible symptom reported on TPC-H Q22: ```rust let df = ctx.sql(q22_sql).await?; df.clone().show().await?; // prints 7 rows (correct) df.count().await?; // returns 0 (wrong) ``` `EXPLAIN` for the count plan shows the outer count aggregate collapsed to `ProjectionExec([lit(0)]) -> PlaceholderRowExec`. After PR apache#21240 left uncorrelated scalar subqueries in the filter rather than rewriting them to joins, `FilterExec` can't use interval analysis on `ScalarSubqueryExpr`, falls back to the 20% default selectivity, and produces a small `Inexact` row estimate. A `LeftAnti` join whose estimated semi-overlap covers the outer estimate then yields `Inexact(0)`. That zero propagates through grouped aggregates whose `estimate_num_rows` returns the child stats unchanged when `value == 0`. The pre-existing `with_fetch` bug on a downstream `SortExec` finally promotes it to `Exact(0)`, which `AggregateStatistics` trusts. The root cause is the precision promotion in `with_fetch`. The PR fixes that; the surrounding plan-shape changes after apache#21240 just made it reachable. ## What changes are included in this PR? - `Statistics::with_fetch`: when `nr <= skip`, preserve the exactness of the input via `check_num_rows(Some(0), self.num_rows.is_exact().unwrap())` instead of always returning `Exact(0)`. - `datafusion-common`: new unit test `test_with_fetch_skip_all_rows_inexact` pinning the new behaviour. - `datafusion-physical-plan`: update the existing `test_row_number_statistics_for_global_limit` expectation that encoded the old (incorrect) promotion to expect `Inexact(0)` now. - `datafusion/sqllogictest/test_files/subquery.slt`: SLT regression test reproducing the user-visible `count(*)` symptom over a query that contains a scalar subquery, `not exists`, and a group-by on a derived column, backed by parquet sources so the data sources report Exact statistics. ## Are these changes tested? Yes: - Unit test in `datafusion-common` for the precision-preserving behaviour. - Updated unit test in `datafusion-physical-plan/limit.rs`. - SLT regression test in `subquery.slt` that fails without the fix (`count(*)` returns `0` instead of `2`) and passes with it. ## Are there any user-facing changes? Only the bug fix. No public API changes. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
adriangb
force-pushed
the
exp/r6-pruningpredicate-rates
branch
from
7fafd34 to
b8dd5b6
Compare
github-actions
Bot
added
execution
functions
development-process
sql
logical-expr
ffi
catalog
optimizer
spark
substrait
labels
adriangb
force-pushed
the
exp/r6-pruningpredicate-rates
branch
from
b8dd5b6 to
5b0c491
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
14 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.