adriangb commented on PR #23779:
URL: https://github.com/apache/datafusion/pull/23779#issuecomment-5074994074
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 #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.
--
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]
