fix(ir): merge window frames for bound analytic window functions with a subsequent over call #7790
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kszucs
force-pushed
the
percent-rank-bug
branch
2 times, most recently
from
2b283fd to
2c02b3b
Compare
kszucs
commented
Dec 17, 2023
| @@ -48,10 +50,24 @@ def test_value_over_api(alltypes): | |||
| w1 = ibis.window(rows=(0, 1), group_by=t.g, order_by=[t.f, t.h]) | |||
| w2 = ibis.window(range=(-1, 1), group_by=[t.g, t.a], order_by=[t.f]) | |||
|
|
|||
| expr = t.f.cumsum().over(rows=(0, 1), group_by=t.g, order_by=[t.f, t.h]) | |||
There was a problem hiding this comment.
I think we have been misinterpreting this scenario because we used the cumulative_window() frame constructed in the cumsum() call, but we also provided explicit rows window boundaries. I changed the behaviour to raise due to window frame boundary conflict.
kszucs
force-pushed
the
percent-rank-bug
branch
5 times, most recently
from
38e3239 to
3d129f9
Compare
cpcloud
requested changes
Dec 18, 2023
| @@ -48,10 +50,24 @@ def test_value_over_api(alltypes): | |||
| w1 = ibis.window(rows=(0, 1), group_by=t.g, order_by=[t.f, t.h]) | |||
| w2 = ibis.window(range=(-1, 1), group_by=[t.g, t.a], order_by=[t.f]) | |||
|
|
|||
| expr = t.f.cumsum().over(rows=(0, 1), group_by=t.g, order_by=[t.f, t.h]) | |||
kszucs
force-pushed
the
percent-rank-bug
branch
from
3d129f9 to
50fae67
Compare
kszucs
force-pushed
the
percent-rank-bug
branch
from
50fae67 to
c51e224
Compare
cpcloud
reviewed
Dec 18, 2023
cpcloud
approved these changes
Dec 18, 2023
cpcloud
added
bug
|
Might need to regen some code in the cloud backends, lemme see. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #7631
#7327 removed the argument of the relevant analytic functions and changed the value methods to return with a
WindowFunctionwith a frame ordered by the value. When.over()is called on a window function the window frames were not merged, while we had that functionality it was only used by thetable.group_by().select(reduction | analytic)API calls (theGroupedTablehelper object).This PR changes the
.over()method to merge the window builder provided in the call with the window already existing in the expression if any. It also deduplicates theorder_byandgroup_byexpressions during merge now.