Phoenix500526 commented on PR #23752: URL: https://github.com/apache/datafusion/pull/23752#issuecomment-5139181473
> 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 [refactor(proto): put Partitioning / sort-expression serde on the types #24003](https://github.com/apache/datafusion/pull/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 [Proto: migrate file sink serialization #23781](https://github.com/apache/datafusion/pull/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. 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]
