adriangb opened a new pull request, #24610:
URL: https://github.com/apache/datafusion/pull/24610
## Which issue does this PR close?
<!-- No dedicated issue; this is follow-up cleanup for the proto hook
migration EPIC. -->
- Part of #23494.
## Rationale for this change
`SortExec::with_dynamic_filter_expr`,
`AggregateExec::with_dynamic_filter_expr` and
`HashJoinExec::with_dynamic_filter_expr` were added in #22011 for one
reason: at the
time `datafusion-proto` drove serialization from outside the crate where
these plans
are defined, so it needed a public setter to put a decoded dynamic filter
back onto a
plan. The matching getters (`dynamic_filter_expr`) were deprecated in #24068
in favour
of `ExecutionPlan::dynamic_expressions_produced`.
Serialization now lives next to each plan (EPIC #23494), so the only caller
left for
each setter is that same plan's `try_from_proto`, in the same file. Nothing
outside
DataFusion uses them either: a GitHub code search for
`with_dynamic_filter_expr`
returns only the three definitions in this repository, and grepping shallow
clones of
datafusion-distributed, datafusion-ballista, datafusion-comet, sail,
delta-rs,
iceberg-rust, sedona-db and openobserve turns up no callers.
Handing external code a way to swap a plan's dynamic filter is also the kind
of API
that is easy to misuse: the filter is wired to the plan's internal state,
which is why
each setter has to re-validate the filter's children against the plan's
schema.
## What changes are included in this PR?
- Marks all three `with_dynamic_filter_expr` methods `#[deprecated(since =
"56.0.0")]`.
- Moves each body to a private `set_dynamic_filter`, called by
`try_from_proto` and by
the tests. The public method is a thin shim that delegates, so there is no
duplicated
logic and no behavior change.
The deprecation note follows the wording already used for
`AsyncFuncExec::async_exprs`,
deprecated for the same reason in #23825.
## Are these changes tested?
Covered by the existing tests, which now exercise the private method:
`test_with_dynamic_filter`, `test_with_dynamic_filter_error_unsupported`,
`test_with_dynamic_filter_error_column_mismatch` (aggregate),
`test_with_dynamic_filter` /
`test_with_dynamic_filter_rejects_invalid_columns` (sort
and hash join), plus the dynamic-filter round-trip tests in
`datafusion-proto`.
`cargo test -p datafusion-physical-plan --features proto --lib` and
`cargo test -p datafusion-proto` pass, and `cargo clippy --all-targets
--workspace
--features avro,integration-tests,extended_tests -- -D warnings` is clean.
## Are there any user-facing changes?
Three public methods are deprecated. Nothing is removed, so no code stops
compiling.
There is no replacement: dynamic filters are created by the plan itself and
restored by
the plan's own `try_from_proto`. If you have a use case for setting one from
outside,
please comment here or open an issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
