Phoenix500526 commented on PR #23779: URL: https://github.com/apache/datafusion/pull/23779#issuecomment-5076699056
> Reviewed this against the reference implementation in #23496 and against the code it deletes from `datafusion/proto/src/physical_plan/mod.rs` — the port looks faithful field-for-field (encode/decode struct construction, by-name `AggregateMode` match, `asc: !descending` ordering encoding, `(!buf.is_empty()).then_some(buf)` UDAF payload semantics, and the payload → codec / registry → codec lookup order all match). Two things to fix up before merge: > > 1. **Please move the unit tests along with the function.** This PR moves > `split_human_display_alias` from `datafusion-proto` into `aggregates/mod.rs`, but the two > tests covering it — `split_human_display_alias_ignores_mismatched_alias` and > `split_human_display_alias_keeps_malformed_prefix_literal` — are deleted rather than moved. > They cover exactly the tricky cases in that helper (mismatched alias, malformed prefix > falling back to the literal), so they should follow it into `datafusion-physical-plan`. > 2. **Consider keeping the explanatory comments from [[EPIC/reference] Port ExecutionPlan serialization to try_to_proto / try_from_proto hooks (full) #23496](https://github.com/apache/datafusion/pull/23496).** The port renames locals and strips > the reference version's comments. The renames are fine, but a couple of the comments are > load-bearing rationale rather than narration — in particular the note that `AggregateMode` is > matched **by name** because the proto and `datafusion_common` enums are numbered differently, > so a numeric cast would silently corrupt the wire format. That is the kind of thing a future > editor needs to see at the call site. Same for the note that `encode_udaf`/`decode_udaf` > already apply the `(!buf.is_empty()).then_some(buf)` semantics, and that the aggregate's own > physical schema (not the child schema) is what arg/ordering/filter exprs decode against. > > Also note `HUMAN_DISPLAY_ALIAS_PREFIX` / `encode_human_display_alias` now exist in both crates (the deprecated `serialize_physical_aggr_expr` path still needs the `datafusion-proto` copy). That's expected for now, but worth a comment on one of them pointing at the other so the two stay in sync until the deprecation window closes. 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]
