Repository navigation
[pull] main from apache:main - #277
Merged
Merged
Conversation
## 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 #123` indicates that this PR will close issue #123. --> - Closes #22820. ## Rationale for this change Sliding `SUM(DISTINCT)` only supports `Int64`, but it was routed through the wider `SUM` type dispatch path. This made unsupported types fail with a less clear accumulator error. <!-- 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 PR adds an explicit `Int64` gate for sliding `SUM(DISTINCT)`. Unsupported types now return a clear feature error that names the operation and type. The existing `Int64` path is unchanged. <!-- 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 <!-- 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 public API change <!-- 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? - Part of #22330. This is the first of the per-method PRs that issue describes. It plumbs `placement` only; the remaining defaulted methods follow separately, so the umbrella issue stays open. ## Rationale for this change `FFI_ScalarUDF` (`datafusion/ffi/src/udf/mod.rs`) carried no function pointer for `placement`, and `ForeignScalarUDF` did not override it, so a producer's override of `ScalarUDFImpl::placement` (default body at `datafusion/expr/src/udf.rs:1028`) was dropped on the consumer side and every foreign UDF fell back to `KeepInPlace`. A UDF loaded over FFI never delivered its leaf-pushdown hint to the optimizer. ## What changes are included in this PR? - New `FFI_ExpressionPlacement` enum bridge in `datafusion/ffi/src/placement.rs`, in the shape of `FFI_Volatility`: `#[repr(u8)]` with `From` impls both ways and a round-trip test over every variant. - A `placement` function pointer on `FFI_ScalarUDF`, populated in the `From<Arc<ScalarUDF>>` constructor, with `placement_fn_wrapper` on the producer side and a forwarding `ForeignScalarUDF::placement` on the consumer side. `placement` is infallible, so the pointer returns the enum directly rather than `FFI_Result`. Adding a field to the `#[repr(C)]` struct changes its layout, so this is an API change and should carry the `api change` label (I can't add it myself). It targets `main` and should not be back-ported to a release branch. `display_name` is also on the issue's list, but it has been deprecated since 50.0.0, so it should be dropped from the gap list rather than plumbed. I have left it and the remaining methods to follow-up PRs. ## Are these changes tested? Yes. - Unit: a round-trip test over all four `ExpressionPlacement` variants, plus a forced-foreign test (`mock_foreign_marker_id`) using a UDF whose `placement` override depends on its arguments. The assertions cover ordered, reordered, and empty argument slices, so argument marshalling is checked, not just the return value. - Integration: `tests/ffi_udf.rs` loads the UDF from the real cdylib and asserts the override survives the boundary, which is the surface a layout change needs. Run with `cargo test -p datafusion-ffi` and `cargo test -p datafusion-ffi --features integration-tests`. ## Are there any user-facing changes? A `placement` override on a `ScalarUDFImpl` is now preserved across the FFI boundary instead of being silently replaced by the default. This is an ABI change to `FFI_ScalarUDF`; consumers must be recompiled against the new layout. --------- Signed-off-by: Amogh Ramesh <ramogh2404@gmail.com>
## Which issue does this PR close? - Closes #22808. ## Rationale for this change Follow-up to #22791, as suggested in review by @2010YOUY01. That PR fixed a double-decrement bug where `EmitLeftUnmatched` did two jobs at once — deciding whether a partition emits unmatched-left rows (which decrements the shared `probe_threads_counter`) and performing the emit. Because the state is re-enterable (a ready batch can be flushed before the state advances to `Done`), the counter could be decremented twice, driving it to zero before all partitions finished probing and emitting spurious NULL-padded rows. #22791 patched this with a `probe_completed_reported` guard flag. This refactor makes "decrement exactly once per probe stream" a structural property of the state graph rather than a runtime guard, so the inner logic is easier to follow and the bug is harder to reintroduce. ## What changes are included in this PR? Restructures the state machine from `FetchingRight → EmitLeftUnmatched` to `FetchingRight → ProbeEnd → EmitLeftUnmatched`: - Adds a dedicated `ProbeEnd` state, entered exactly once per left chunk when the right side is exhausted. It owns the single `report_probe_completed()` call and records whether this stream is the unmatched-left emitter. - Replaces the `probe_completed_reported` guard flag with an `is_unmatched_left_emitter` field that `EmitLeftUnmatched` only reads. - Removes the per-chunk flag reset in the memory-limited path (the decision is recomputed in `ProbeEnd` for each chunk) and reverts the `Arc::clone` workaround #22791 needed in `process_left_unmatched`. - Updates the state-transition doc graph and arm comments. No behavior change is expected. ## Are these changes tested? Yes — covered by existing tests: - All 42 `nested_loop_join` unit tests and the full `datafusion-physical-plan` suite pass. - `joins.slt` sqllogictests pass (including the multi-partition LEFT JOIN regression test added in #22791). - 41 `join_fuzz` tests (`cargo test --features extended_tests`) comparing `NestedLoopJoinExec` against `HashJoinExec` across every join type, filtered and unfiltered, with a multi-partition probe side — the exact scenario class of the original bug — pass. - `cargo fmt` and `cargo clippy --all-targets --all-features -- -D warnings` are clean. ## Are there any user-facing changes? No.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )