You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "cool9850311 (via GitHub)" <gi...@apache.org> on 2026/10/18 12:59:51 UTC
[PR] fix: forward `create_sliding_accumulator` to the inner UDAF when aliased [datafusion]
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: forward `create_sliding_accumulator` to the inner UDAF when aliased [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #26480:
URL: https://github.com/apache/datafusion/pull/26480#issuecomment-6250636044
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/26480?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `62.90323%` with `23 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.89%. Comparing base ([`3d9037c`](https://app.codecov.io/gh/apache/datafusion/commit/3d9037cb0fdc4965cd4fd5ea1f5e6f5ff7927856?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`519799e`](https://app.codecov.io/gh/apache/datafusion/commit/519799e0641446307a4239e7fc3aaae5663a9327?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/26480?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/expr/src/udaf.rs](https://app.codecov.io/gh/apache/datafusion/pull/26480?src=pr&el=tree&filepath=datafusion%2Fexpr%2Fsrc%2Fudaf.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9leHByL3NyYy91ZGFmLnJz) | 62.90% | [23 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/26480?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #26480 +/- ##
==========================================
- Coverage 82.89% 82.89% -0.01%
==========================================
Files 1154 1154
Lines 457961 458022 +61
Branches 457961 458022 +61
==========================================
+ Hits 379637 379664 +27
- Misses 55285 55314 +29
- Partials 23039 23044 +5
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/26480?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: forward `create_sliding_accumulator` to the inner UDAF when aliased [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #26480:
URL: https://github.com/apache/datafusion/pull/26480
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org