adriangb opened a new pull request, #24003:
URL: https://github.com/apache/datafusion/pull/24003
## Which issue does this PR close?
- Part of #23494 (the `try_to_proto` / `try_from_proto` migration).
Precursor for #23497 / #23683 and for the remaining per-plan migrations that
need to encode an output partitioning.
## Rationale for this change
`Partitioning`'s protobuf conversion currently exists in three places:
- inline in `RepartitionExec::try_to_proto`,
- inline in `RepartitionExec::try_from_proto`,
- in `datafusion-proto`'s `serialize_partitioning` /
`parse_protobuf_partitioning`.
Every plan or data source migrated to the hooks that has an output
partitioning has so far copied it again — #23683 is about to add a fourth
copy for `FileScanConfig`. The same is true of the flat
`PhysicalSortExprNode` encoding, which any operator holding an ordering
needs.
There is no reason for this logic to live in the callers: it converts a
`Partitioning` (and the `PhysicalSortExpr`s and `ScalarValue`s inside it),
and
needs nothing from the plan level beyond the ability to encode a child
expression.
## What changes are included in this PR?
Put the single copy next to the types that own it, taking the
expression-level
context (`datafusion-physical-expr` and `-common` already carry the `proto`
feature):
- `PhysicalSortExpr::try_to_proto` / `try_from_proto`
(`physical-expr-common`)
- `Partitioning::try_to_proto` / `try_from_proto` (`physical-expr`)
So plan hooks can reach them, `ExecutionPlanEncodeCtx` /
`ExecutionPlanDecodeCtx`
now back the expression-level contexts (`PhysicalExprEncode` /
`PhysicalExprDecode`) and hand one out via `expr_ctx()`. That bridge is
useful
beyond partitioning: from here on any plan hook can pass its ctx straight to
an
expression-level conversion, which is the shape the rest of the migration
wants.
`RepartitionExec` and `datafusion-proto`'s central serializer both route
through
the type methods now, which also retires `serialize_range_partitioning`,
`serialize_range_split_point`, `parse_protobuf_range_partitioning` and
`parse_protobuf_range_split_point`.
Net: −269 / +261 lines, and the next operator that needs partitioning serde
writes one line instead of sixty.
## Are these changes tested?
Yes — by the existing round-trip suites, which cover all four partitioning
variants (`datafusion-proto`'s `roundtrip_physical_plan` tests exercise the
central serializer, `RepartitionExec`'s own hook tests exercise the plan
path).
Full workspace run: 10075 passed / 0 failed
(`cargo test --profile ci --workspace --lib --tests --features
avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption`),
plus `cargo fmt` and `ci/scripts/rust_clippy.sh` clean.
## Are there any user-facing changes?
The protobuf wire format is unchanged.
Additive API:
- `Partitioning::try_to_proto` / `try_from_proto`,
`PhysicalSortExpr::try_to_proto` /
`try_from_proto` (feature `proto`).
- `ExecutionPlanEncodeCtx::expr_ctx()` /
`ExecutionPlanDecodeCtx::expr_ctx(schema)`.
Two behavior differences, both in error paths:
- Out-of-range partition counts now return an error instead of wrapping
(`as usize`) or panicking (`try_into().unwrap()` in
`parse_protobuf_hash_partitioning`).
- A missing sort-expression child now reports which field is missing
(`PhysicalSortExpr is missing required field 'expr'`) instead of
`Unexpected empty physical expression`.
Four private helpers in `datafusion-proto` are removed
(`serialize_range_partitioning`, `serialize_range_split_point`,
`parse_protobuf_range_partitioning`, `parse_protobuf_range_split_point`); the
public `serialize_partitioning` / `parse_protobuf_partitioning` keep their
signatures and behavior.
--
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]
