cool9850311 opened a new pull request, #26480:
URL: https://github.com/apache/datafusion/pull/26480
## Which issue does this PR close?
- Closes #26466.
## Rationale for this change
Giving an aggregate function an extra name with `AggregateUDF::with_aliases`
broke that function for every window frame that does not start at `UNBOUNDED
PRECEDING`. After registering an aliased `sum`, this query
```sql
SELECT my_sum(i) OVER (ORDER BY i ROWS BETWEEN 1 PRECEDING AND CURRENT ROW)
FROM t
```
failed with
```
NotImplemented("Aggregate can not be used as a sliding accumulator because
`retract_batch` is not implemented: sum(t.i) ORDER BY [t.i ASC NULLS LAST] ROWS
BETWEEN 1 PRECEDING AND CURRENT ROW")
```
instead of returning `1, 3, 5, 7`. Aliased `min` and `max` failed the same
way.
The original name was affected too. `AliasedAggregateUDFImpl::name()`
forwards to the inner function, so registering an aliased `sum` replaces the
built-in under the name `sum` as well, and a plain `sum(i)` over a sliding
frame then failed in that session even though the user never touched the alias.
## What changes are included in this PR?
One line in `AliasedAggregateUDFImpl::create_sliding_accumulator`. It
delegated to the inner function's `accumulator` rather than to the inner
function's `create_sliding_accumulator`, so an aliased function was handed the
plain accumulator, which does not implement `retract_batch`.
`sum`, `min`, `max` and `count` all override `create_sliding_accumulator`,
so forwarding to it is enough to restore the correct behaviour. The `DISTINCT`
paths come along for free, because each function decides on
`AccumulatorArgs::is_distinct` internally; the wrapper only has to ask the
right method.
Since #12098 set out to forward every method on this wrapper to the inner
function, this looks like a slip rather than a deliberate choice. The
`#[warn(clippy::missing_trait_methods)]` attribute on the impl cannot catch it,
because the method does exist and only its body is wrong. It is also easy to
miss by eye: the wrong body is exactly the trait's own default implementation
of `create_sliding_accumulator`.
## What is the testing strategy for this PR?
`datafusion/expr/src/udaf.rs` adds
`test_aliased_udaf_forwards_create_sliding_accumulator`, a unit test built on a
UDAF double whose two accumulator methods are deliberately distinguishable: the
plain one returns an error, the sliding one succeeds. Asserting both directions
pins the delegation itself, independent of how any real aggregate implements
its accumulators, so the test keeps its meaning even if `sum`'s sliding
accumulator is rewritten.
`datafusion/core/tests/user_defined/user_defined_aggregates.rs` adds three
end-to-end tests, placed next to the existing
`test_user_defined_functions_with_alias`:
- aliased `sum`, `min` and `max` over `ROWS BETWEEN 1 PRECEDING AND CURRENT
ROW`, asserting the values from the issue;
- the same query under the original `sum` name, covering the
registry-overwrite case described above;
- an `UNBOUNDED PRECEDING` control, which takes the plain-accumulator path
and passes both before and after this change.
**Why these are not `.slt` tests**, even though the queries themselves are
plain SQL: reproducing the bug needs `with_aliases` plus `register_udaf`, and
`datafusion/sqllogictest` has no way to register a user-defined aggregate, so
an aliased UDAF cannot be constructed from SQL at all. That puts this change at
the second tier of the testing guide, end-to-end tests through the public API,
rather than the first.
Each new test was checked to fail without the one-line change, with the
error message quoted above, and the control test to pass either way.
## Are there any user-facing changes?
Yes, as a bug fix. Aliased `sum`, `min` and `max` now work in window frames
that do not start at `UNBOUNDED PRECEDING`, and a session that registers an
aliased aggregate no longer breaks the built-in function of the same name in
those frames. No public API signature changes, so no `api change` label:
`AliasedAggregateUDFImpl` is private, and `create_sliding_accumulator` was
already a defaulted method on `AggregateUDFImpl`.
## AI usage
This PR was prepared with Claude Code.
Two things worth flagging rather than burying:
- The `UNBOUNDED PRECEDING` control test passes with or without the fix by
design. It is there to show that the change does not affect the
plain-accumulator path. Happy to drop it if reviewers consider it noise.
- While checking whether this was a systemic problem, I looked at the
sibling wrappers. `AliasedWindowUDFImpl` and the FFI UDAF wrapper both forward
correctly, but `AliasedScalarUDFImpl::with_updated_config` returns `None`
instead of forwarding to the inner function, which looks like the same class of
missing delegation. I left it out of this PR because a correct fix has to
re-wrap the returned `ScalarUDF` so the aliases are not lost, which is a
separate decision. Happy to open an issue for it if that would be useful.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01Fyw8WvayuoabyXS57kvCma
--
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]
