Phoenix500526 commented on PR #23781:
URL: https://github.com/apache/datafusion/pull/23781#issuecomment-5140678524
> Thanks @Phoenix500526 — reviewing the commits on top of #23752 as you
asked. The migration itself is faithful: each sink's encode matches the arm it
replaces field-for-field (including `sink_schema` and decoding `sort_order`
against the _input_ schema), the central `DataSinkExec` downcast block is
removed, the decode arms are repointed, and the old helpers are kept as
deprecated delegates. That is the shape #23494 asks for.
>
> Four things before this comes out of draft.
>
> **Rebase.** Main has moved: #24006 merged and this now conflicts in
`datafusion/datasource/Cargo.toml`,
`datafusion/proto/src/physical_plan/mod.rs`, `from_proto.rs` and
`roundtrip_physical_plan.rs`. Most of the resolution is deletion —
`datafusion-datasource` already carries a `proto` feature, and
`PartitionedFile` / `FileGroup` <-> proto already live in
`datafusion_datasource::proto`.
>
> **Use `expect_plan_variant!` instead of hand-rolling the variant match.**
Each `try_from_proto` currently opens with
>
> ```rust
> let sink_node = match &node.physical_plan_type {
> Some(protobuf::physical_plan_node::PhysicalPlanType::CsvSink(sink)) =>
sink.as_ref(),
> _ => return datafusion_common::internal_err!("PhysicalPlanNode is not
a CsvSink"),
> };
> ```
>
> which is what the macro exists for. It is `#[macro_export]`ed from
`datafusion-physical-plan`, so the format crates can use it as
`datafusion_physical_plan::expect_plan_variant!`, and every plan migrated under
this epic on main already does. It also produces the same error string, so
nothing changes behaviorally.
>
> **The `TryFromProto` impls for the sink messages are now duplicated
logic.** `impl TryFromProto<&CsvSink> for protobuf::CsvSink` (and the
Json/Parquet siblings, both directions) still carry their original bodies in
`datafusion-proto`, while this PR adds a second copy of the same field mapping
inside `CsvSink::try_to_proto`. That is the drift risk the epic is trying to
remove, and it is the one thing here that is not yet single-sourced.
>
> Since these types live in the format crates, the conversions can move next
to them as standard `TryFrom` impls — see #24006 for the pattern and #24019 for
why. Concretely, give each sink an inherent conversion for its _message_:
>
> ```rust
> impl TryFrom<&CsvSink> for protobuf::CsvSink { ... }
> impl TryFrom<&protobuf::CsvSink> for CsvSink { ... }
> ```
>
> then have `CsvSink::try_to_proto` / `try_from_proto` use them for the
`sink` field, and reduce `datafusion-proto`'s `TryFromProto` impls to
delegates. As a bonus this restores the `TryFrom<&CsvSink>` /
`TryFrom<&protobuf::CsvSink>` impls that shipped in 54.1.0 and were dropped on
main (#24019), so it is a regression fix rather than new API.
>
> **Consider merging this into #23752.** The two are one logical change —
#23752 adds a hook with no built-in implementor, and until this PR lands every
built-in sink encodes its input subtree twice (the hook encodes it, the sink
returns `Ok(None)`, and the central chain encodes it again). Merged together
they are also easier to review as a single before/after, and it removes the
need for `parse_sink_sort_order` to be public API in #23752 with no caller.
Entirely your call — the stack is reviewable as-is if you would rather keep it
split.
>
> One small thing: the non-`parquet` arm changed its panic message from
"Trying to use ParquetSink without `parquet` feature enabled" to "Unable to
process a Parquet PhysicalPlan when `parquet` feature is not enabled".
Harmless, but if it was not deliberate it is worth keeping the original so the
string stays greppable.
Done
--
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]
