[branch-54] fix: Avoid panicing when stats are not available for a file group split (backport #23277)#23339
Closed
mkleen wants to merge 20 commits into
Closed
[branch-54] fix: Avoid panicing when stats are not available for a file group split (backport #23277)#23339mkleen wants to merge 20 commits into
mkleen wants to merge 20 commits into
Conversation
…20337)" (apache#22437) (apache#22445) - Backports apache#22437 from @alamb to the branch-54 line This PR cherry-picks the revert of `ExecutionPlan::apply_expressions()` (apache#20337) onto `branch-54` so that DataFusion 54.0 does not ship the new public API.
…apache#22443) ## 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. --> Backport apache#22404 - Closes #. ## 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. --> ## 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. --> ## 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)? --> ## Are there any user-facing changes? <!-- 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. -->
…ter stats-based file reorder (apache#22501) ## Which issue does this PR close? Cherry-pick of apache#22493 onto `branch-54`. ## Rationale for this change `branch-54` includes apache#21956 (`feat: globally reorder files and row groups by statistics for TopK queries`), which introduced a regression: for plain-column, multi-file scans where the on-disk file order does not match the declared sort order, `SortExec` was no longer eliminated even when stats-based reorder produced non-overlapping file groups whose declared ordering re-validated. apache#22493 restores the pre-apache#21956 sort-elimination behaviour by re-validating `output_ordering` after `rebuild_with_source` reorders files, and (per @adriangb's correctness follow-up) restoring the original hint-free `file_source` on the Inexact→Exact upgrade so leftover `reverse_row_groups` / `sort_order_for_reorder` hints don't mis-order row groups within a single file once the `SortExec` safety net is gone. ## What changes are included in this PR? Straight cherry-pick of merge commit `94c58d086`. Includes: - `FileScanConfig::try_pushdown_sort` Inexact arm: re-validate, upgrade to Exact (with file_source restore), guard with NULL safety + early-return - `rebuild_with_source`: `match (all_non_overlapping, is_exact)` decision table for keep_ordering - SLT updates restoring `SortExec` elimination expectations + Tests 5b/5c/8b for the NULL-safety and same-min row-group edge cases ## Are these changes tested? Cherry-picked cleanly (auto-merge in `sort_pushdown.rs`). `cargo build -p datafusion-datasource` — passes. `cargo test -p datafusion-sqllogictest --test sqllogictests -- sort_pushdown` — passes. ## Are there any user-facing changes? Same as apache#22493: plain-column wrong-order-files cases regain SortExec elimination when files happen to be non-overlapping by statistics. No new API. Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… container types (apache#21934) (apache#22446) - Backports apache#21934 from @bert-beyondloops to the branch-54 line This PR cherry-picks the fix for `ScalarValue::compact` to compact view buffers for all container types onto `branch-54`. Co-authored-by: Bert Vermeiren <103956021+bert-beyondloops@users.noreply.github.com> Co-authored-by: Bert Vermeiren <bert.vermeiren@datadobi.com> Co-authored-by: Dmitrii Blaginin <dmitrii@blaginin.me>
## Which issue does this PR close? - Refs apache#22557. - Companion PR to apache#22559, targeting `branch-54`. ## Rationale for this change DataFusion 54 changed `ExecutionPlan` downcasting to use the `Any` supertrait directly. That removes `ExecutionPlan::as_any`, which had also served as a customization point for wrapper nodes: wrappers could identify as themselves internally while exposing the wrapped plan type to normal downcast-based inspection. This PR adds an explicit `ExecutionPlan::downcast_delegate()` hook for wrapper nodes that want their public `ExecutionPlan` downcast identity to be delegated to another plan. The proposed behavior intentionally preserves the old `as_any` override semantics: when a node opts into downcast delegation, intermediate delegating wrappers are invisible to `dyn ExecutionPlan::is::<T>()` and `downcast_ref::<T>()`. ## What changes are included in this PR? - Adds `ExecutionPlan::downcast_delegate()` with a default implementation returning `None`. - Updates `dyn ExecutionPlan::is::<T>()` and `downcast_ref::<T>()` to delegate to `downcast_delegate()` when present, otherwise use the current concrete plan type. - Documents that `downcast_delegate()` is only for type introspection and is independent from `children()` / plan traversal. - Adds tests for direct and nested downcast-delegating wrappers, including that intermediate delegating wrappers remain invisible to normal downcast-based inspection. ## Are these changes tested? Yes. - `cargo test -p datafusion-physical-plan execution_plan_downcast` - `cargo test -p datafusion-physical-plan --lib` - `cargo fmt --all -- --check` - `git diff --check` ## Are there any user-facing changes? Yes. This adds a new public `ExecutionPlan` trait method with a default implementation, and it changes `ExecutionPlan` downcast helpers to honor wrappers that explicitly opt into delegating public downcast identity.
) (apache#22634) - Part of apache#21080 This PR: - Backports apache#22571 from @kumarUjjawal to the `branch-54` line Co-authored-by: Kumar Ujjawal <ujjawalpathak6@gmail.com>
…erUDF struct (apache#22593) (apache#22635) - Part of apache#21080 This PR: - Backports apache#22593 from @LiaCastaneda to the `branch-54` line ## Note on conflict resolution The cherry-pick had conflicts in two test areas: - `datafusion/functions-nested/src/array_any_match.rs` — only the test-module `use` line conflicted; `branch-54`'s test does not reference the renamed symbols, so the existing import was kept (the production `HigherOrderUDF` -> `HigherOrderUDFImpl` rename applied cleanly). - `datafusion/substrait/tests/cases/roundtrip_logical_plan.rs` — the upstream PR modified the `roundtrip_array_transform_higher_order_function` test and `ArrayTransform` helper, but that test was added to `main` after `branch-54` was cut and does not exist on `branch-54`. It is out of scope for this refactor backport, so the `branch-54` file was left unchanged. Co-authored-by: Lía Adriana <lia.castaneda@datadoghq.com>
…ryToJoin` (#… (apache#22693) This is PR packports apache#22316 from @neilconway to branch-54 Co-authored-by: Neil Conway <neil.conway@gmail.com>
…pache#22530) (apache#22690) ## 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 #. ## Rationale for this change This PR backports apache#22530 ## 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. --> ## 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)? --> ## Are there any user-facing changes? <!-- 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. -->
…Pushdown (apache#22525) (apache#22631) - Part of apache#21080 This PR: - Backports apache#22525 from @kumarUjjawal to the `branch-54` line Co-authored-by: Kumar Ujjawal <ujjawalpathak6@gmail.com>
…flag (backport apache#22632) (apache#22648) ## Which issue does this PR close? - Backport of apache#22632 to `branch-54`. ## Rationale for this change Content-defined chunking (CDC) write options were added in apache#21110 and are slated for the 54.0.0 release. This backports the refactor in apache#22632 so the config/proto surface ships in its final form, before the release goes out. The CDC options previously worked as `use_content_defined_chunking: Option<CdcOptions>` with a `ConfigField` impl that accepted a bare `use_content_defined_chunking = true|false` and otherwise enabled CDC implicitly when any sub-field was set. This has a few problems: - **Naming diverges from parquet-rs.** `WriterProperties` exposes `content_defined_chunking()` / `set_content_defined_chunking(Option<CdcOptions>)` with no `use_` prefix. - **Implicit / order-dependent on the SQL side.** Format options in `COPY ... OPTIONS` / `CREATE EXTERNAL TABLE ... OPTIONS` are applied from a `HashMap` (non-deterministic order). With the old bare-boolean form, mixing `... = false` with a sub-field could resolve to enabled or disabled depending on iteration order. - **Extra machinery.** Supporting the bare boolean required hand-written `ConfigField` impls and a `#[expect(clippy::should_implement_trait)]` workaround, plus a zero-sentinel fallback in the proto mapping. Since CDC is unreleased, the config/proto surface can still be changed freely. ## What changes are included in this PR? - Rename the `ParquetOptions` field `use_content_defined_chunking` -> `content_defined_chunking` (matches parquet-rs). - Make `CdcOptions` a plain `config_namespace!` with an explicit `enabled: bool` field alongside the chunking parameters; the field is a bare `CdcOptions` (no longer `Option<CdcOptions>`). CDC is on iff `content_defined_chunking.enabled` is true. Setting a parameter no longer implicitly enables CDC, and the result is independent of key order. - Add `CdcOptions::enabled()` / `CdcOptions::disabled()` shorthand constructors. - Drop the `ConfigField` impls and the `should_implement_trait` workaround — all generated by the macro now. - Add an `enabled` field to the proto `CdcOptions` message so the proto <-> config mapping is a plain field copy in both directions. - Update unit tests, regenerate config docs + the `information_schema` snapshot, and add `parquet_cdc_config.slt` documenting the resolution behavior. ## Are these changes tested? Yes — `datafusion-common` config + writer unit tests, `datafusion-proto-common` proto round-trip tests, `datafusion/core` parquet integration tests, and sqllogictest (`parquet_cdc.slt` + new `parquet_cdc_config.slt`). Cherry-pick applied cleanly onto `branch-54`; affected crates build and the CDC unit tests pass. ## Are there any user-facing changes? Yes, but only to the unreleased CDC options: - Config key `datafusion.execution.parquet.use_content_defined_chunking` -> `datafusion.execution.parquet.content_defined_chunking.enabled` (plus `.min_chunk_size` / `.max_chunk_size` / `.norm_level`). - The bare-boolean form is removed; enable/disable via `content_defined_chunking.enabled = true|false`. No released API is affected. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
## 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 #. ## 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. --> ## 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. --> ## 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)? --> ## Are there any user-facing changes? <!-- 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. --> --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
…trip (backport apache#22104) (apache#22785) ## Which issue does this PR close? - Backport of apache#22104 to `branch-54` (for 54.1.0, tracked in apache#22547). This PR: - Backports apache#22104 to the `branch-54` line so the `null_aware` proto round-trip fix ships in 54.1.0, as requested in apache#22065 (comment) Clean cherry-pick; `datafusion-proto` builds and both round-trip regression tests pass on `branch-54`.
…ark's width_bucket to i64) (apache#23087) backport apache#22811 to `branch-54` in anticipation of 54.1.0 release. DataFusion Comet encountered this issue while trying to upgrade to DF 54.0.0, so it would ease that upgrade for Comet if this were backported. Co-authored-by: Eduardo Aguilar <contact@eduaguilar.com>
…en row-group statistics already prove it cannot prune) (apache#23088) backport apache#22857 to `branch-54` in anticipation of 54.1.0 release. DataFusion Comet encountered this issue while trying to upgrade to DF 54.0.0, so it would ease that upgrade for Comet if this were backported. This one had conflicts on cherry-pick, so I could use a sanity check on the review by comparing to the original PR. --------- Co-authored-by: Ratul Dawar <ratuldawar11@gmail.com>
…th NULLs (apache#23196) ## 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 #. ## Rationale for this change Backport `array_compact` handle edge case with NULLs. The issue causing correctness issues and has no breaking API, therefore should fall into backport criteria <!-- 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? <!-- 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? <!-- 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? <!-- 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. -->
… config (apache#23296) ## Which issue does this PR close? - Backport of apache#23294 to `branch-54`. - Closes apache#23293 for the 54 release line. ## Rationale for this change `FileStream` sibling work-stealing (`WorkSource::Shared`) seeds one shared work queue from every file group and lets whichever output partition goes idle first steal the next unopened file (or byte-range morsel). This assumes all output partitions of a scan are polled concurrently in one process. Executors that run each output partition as an isolated task in a separate process — Ballista and datafusion-distributed — never poll the sibling partitions. The single polled partition drains the whole shared queue and reads files belonging to other partitions, so every isolated task reads the entire input and the scan output is inflated by the partition count. This is a correctness bug for those executors, not just a performance one. The existing escape hatches (`preserve_order`, `partitioned_by_file_group`) are plan-level flags on `FileScanConfig`, not something a distributed executor can set centrally through the session config, and a plain repartitioned scan does not set `partitioned_by_file_group`. There is no session-level off switch, unlike `datafusion.optimizer.enable_dynamic_filter_pushdown`, which exists precisely so consumers that cannot support runtime cross-partition state can disable it. ## What changes are included in this PR? This is a backport of apache#23294. The changes are identical in intent; two conflicts were resolved for `branch-54`, which does not yet carry the `output_partitioning` field on `FileScanConfig` or the `enable_migration_aggregate` config that exist on `main`: - Add `datafusion.execution.enable_file_stream_work_stealing` (default `true`). When `false`, `FileScanConfig::create_sibling_state` returns `None`, so each partition falls back to `WorkSource::Local` and reads only its own file group. - Thread `&ConfigOptions` into `DataSource::create_sibling_state` so the flag is read from the session config at `execute` time. As a session config value it round-trips through `datafusion-proto` with no proto schema change. - Regenerate `configs.md` and add the setting to `information_schema.slt`. - Add the regression test `morsel_disabled_work_stealing_keeps_files_local` using the file's standard morsel snapshot harness. ## Are these changes tested? Yes. The `file_stream` test suite (including the new `morsel_disabled_work_stealing_keeps_files_local`) passes, and `cargo clippy` is clean. The existing sibling work-stealing tests continue to pass with the default. ## Are there any user-facing changes? A new session config, `datafusion.execution.enable_file_stream_work_stealing` (default `true`), so existing behavior is unchanged. `DataSource::create_sibling_state` gains a `&ConfigOptions` parameter (an API change for anyone implementing the `DataSource` trait directly).
github-actions
Bot
added
documentation
github-actions
Bot
added
execution
mkleen
changed the title
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
10 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.
Which issue does this PR close?
branch-54(for 54.1.0, tracked in Release DataFusion54.1.0(minor/patch) Release #22547).Clean cherry-pick, all tests pass.