adriangb commented on PR #23752:
URL: https://github.com/apache/datafusion/pull/23752#issuecomment-5135719887
Thanks for this @Phoenix500526 — the `FileSinkConfig` move is faithful
field-for-field, and the `DataSinkExec` delegation test is exactly the right
proof that the hook is the live path. A few things to work through, plus a
rebase that main has forced.
**Rebase first.** #24006 merged and touches the same two places this PR
does, so it now conflicts in `datafusion/datasource/Cargo.toml` and
`datafusion/proto/src/physical_plan/from_proto.rs`. The good news is that the
rebase deletes code rather than adding it:
- `datafusion-datasource` already has a `proto` feature on main; drop the
one this PR adds.
- `PartitionedFile` / `FileGroup` <-> proto now live in
`datafusion_datasource::proto` on main, so `partitioned_file_to_proto` /
`partitioned_file_from_proto` in `file_sink_config/proto.rs` can go entirely —
that is the duplication @kumarUjjawal flagged, now resolved upstream.
- If #24003 lands first, `parse_sink_sort_order` collapses into
`sort_exprs_try_from_proto` plus the `LexRequirement::new(...)` wrap.
**Please use `TryFrom` rather than `to_proto` / `from_proto` inherent
methods.** #24006 established this for the file-scan leaf types, and #24019
explains why: `TryFromProto` only exists because `datafusion-proto` owns
neither side of the conversion, and once the impl lives in
`datafusion-datasource` that constraint is gone. So:
```rust
impl TryFrom<&FileSinkConfig> for protobuf::FileSinkConfig { ... }
impl TryFrom<&protobuf::FileSinkConfig> for FileSinkConfig { ... }
```
with `datafusion-proto`'s existing `TryFromProto` impls reduced to one-line
delegates, as this PR already does. This also restores the
`TryFrom<&FileSinkConfig>` impl that shipped in 54.1.0 and was dropped on main
(#24019), so it is a fix rather than a new API. The `try_to_proto(ctx)` naming
stays for conversions that need an encode/decode context; `FileSinkConfig` does
not need one.
**`parse_sink_sort_order` is unused in this PR.** Its only caller is #23781.
As it stands this PR adds a public free function to `datafusion-datasource`
that nothing calls. Either move it to #23781 or make it `pub(crate)` until it
has a caller.
**The hook signature is worth reconsidering before three sinks depend on
it.** `DataSink::try_to_proto` takes pre-encoded proto values:
```rust
fn try_to_proto(&self, input: PhysicalPlanNode, sort_order:
Option<PhysicalSortExprNodeCollection>, sink_schema: &Schema)
```
Compare `FileSource::try_to_proto(&self, base: &FileScanConfig, ctx:
&ExecutionPlanEncodeCtx)` in #23683, and `ExecutionPlan::try_to_proto(&self,
ctx)` on main. Two consequences of the current shape:
1. A sink can never encode an expression of its own without a breaking
signature change.
2. `DataSinkExec::try_to_proto` encodes the whole input subtree eagerly and
discards it whenever the sink returns `Ok(None)` — which, until #23781 lands,
is every built-in sink. On main those encode their input exactly once; with
this PR alone they encode it twice.
Passing the ctx instead fixes both:
```rust
fn try_to_proto(&self, exec: &DataSinkExec, ctx:
&ExecutionPlanEncodeCtx<'_>) -> Result<Option<PhysicalPlanNode>>
```
with a shared helper for the sort-order encoding so the three sinks do not
repeat it. If you would rather not restructure, merging this together with
#23781 also closes the double-encode window.
**`sink_schema` is not the sink's schema.** It is `DataSinkExec::schema()`,
i.e. the single-column count schema, and decode ignores the field and uses
`input.schema()` instead. That matches main byte-for-byte and should stay, but
the parameter name asserts something false — worth renaming or documenting
inline, since the next reader will reasonably assume it is `DataSink::schema()`.
**Make the encode side of `insert_op` a by-name match too.** Decode was
converted to an exhaustive by-name match (good — that was the review fix), but
encode still does `insert_op: self.insert_op as i32`. The numbering happens to
agree today (`Append`/`Overwrite`/`Replace` = 0/1/2 on both sides), so there is
no bug, but a numeric cast across two independently-numbered enums is exactly
the hazard #23494 calls out, and the encode side is the one that silently
corrupts the wire format if they ever diverge.
**One test no longer asserts anything.** In
`file_sink_config_conversion_preserves_compatibility_api`:
```rust
let encoded = config.to_proto()?;
let compatibility_encoded =
protobuf::FileSinkConfig::try_from_proto(&config)?;
assert_eq!(encoded, compatibility_encoded);
```
Since `TryFromProto` was reduced to a delegate in this same PR, both sides
are now the same function, so this compares a value with itself. Either assert
field-level fidelity against the original `config` after a decode, or drop the
test — the round-trip assertions below it are the ones carrying weight.
**Behavior change worth stating in the description.** Decode now errors on
unknown `InsertOp` / `FileOutputMode` values where main silently fell back to
the zero variant. That is the right change, but it means a payload from a newer
producer that previously decoded as `Append` now fails, so it belongs under
"Are there any user-facing changes?" rather than only under "wire format
preserved".
Finally, could you resolve the review threads you have already addressed?
Two are marked done but still open, which makes it hard to see what is left.
--
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]
