Skip to content

feat(risingwave): support tumble and hop window agg#11970

Open
litaxc wants to merge 4 commits intoibis-project:mainfrom
litaxc:main
Open

feat(risingwave): support tumble and hop window agg#11970
litaxc wants to merge 4 commits intoibis-project:mainfrom
litaxc:main

Conversation

@litaxc
Copy link

@litaxc litaxc commented Mar 13, 2026

Description of changes

Implement the visit_WindowAggregate function as it is done in Flink.

Issues closed

Resolves #11969

@github-actions github-actions bot added the sql Backends that generate SQL label Mar 13, 2026
@deepyaman
Copy link
Contributor

Can you add tests for this similar to ibis/backends/flink/tests/test_window.py and ibis/backends/pyspark/tests/test_window.py? TBH I'm not sure where there aren't also tests for hopping windows in these.

Alternatively, it could also be an option to centralize the window tests in ibis/backends/tests/test_window.py, and to only test some of these options for streaming backends, but I think it's probably easiest to take the first route for now.

@github-actions github-actions bot added tests Issues or PRs related to tests risingwave The RisingWave backend labels Mar 17, 2026
@litaxc
Copy link
Author

litaxc commented Mar 17, 2026

I added tests for the tumble and hop functions.
I increased the window size to one day (it was 30 seconds in both Flink and PySpark's tests) to make the engine actually aggregate something.
Let me know if you prefer the original setting.

Copy link
Contributor

@deepyaman deepyaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, very much aligned to the Flink implementation (which is great). @cpcloud / @gforsyth / @NickCrews could one of you kick off the CI workflow?

t = alltypes
expr = (
t.window_by(t.timestamp_col)
.hop(size=ibis.interval(days=10), slide=ibis.interval(days=10))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe should use a different slide so that it's not essentially a hopping window? :) I'm sure it works, but even reviewing at a glance I just noticed the results are the same.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried using size=20 days, but I couldn't quite understand the resulting window_start/end produced by RisingWave. So I changed back to use the same size and slide to ensure I would't introduce wrong results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risingwave The RisingWave backend sql Backends that generate SQL tests Issues or PRs related to tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(risingwave): support tumble and hop window aggregation

2 participants