-
Notifications
You must be signed in to change notification settings - Fork 106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SNOW-982770: [Local Testing] Add support for rank related features. #1890
SNOW-982770: [Local Testing] Add support for rank related features. #1890
Conversation
34a49e2
to
aaacd32
Compare
def test_cume_dist(session): | ||
Utils.check_answer( | ||
TestData.xyz(session).select( | ||
cume_dist().over(Window.partition_by(col("X")).order_by(col("Y"))) | ||
"X", "Y", cume_dist().over(Window.partition_by(col("X")).order_by(col("Y"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see some changes in the test, like adding "X", "Y" in the select statement, change sort from False
to True
in the check_answer.
what's the purpose of the change here? is the previous test not working in the implementation or it's for adding more test coverage here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've enabled the sort because our current logic in _plan.py
does not result in the same ordering as the server side when sorting and partitioning windows. Fixing that would be a lot more scope than I think this PR should include. I'm happy to open a ticket to track that work in the future.
With sorting enabled the "X" and "Y" columns are required to avoid ambiguous results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, now I understand why cols X and Y are introduced.
btw does the server guarantee the same ordering or can return different orders. wondering if we should mention this in the limitation or known gap section of local testing framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the server order is consistent, not sure if it has a guarentee.
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-982770
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
This PR does a few things to add support for rank related features to local testing.