-
Notifications
You must be signed in to change notification settings - Fork 110
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-1566362] Add sql counter check for snowpark python test_cte #2370
Conversation
snowpark pandas daily job started https://ci-dev-142.int.snowflakecomputing.com/job/SnowparkPythonSnowPandasDailyRegressRunner/874/ |
daily job started https://github.com/snowflakedb/snowpark-python/actions/runs/11115145384 |
tests/integ/utils/sql_counter.py
Outdated
sql_count_records: dict[str, dict[str, list[dict[str, Optional[Scalar]]]]] | ||
sql_count_records: Dict[str, Dict[str, List[Dict[str, Optional[Scalar]]]]] |
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.
im not sure if this change is actually needed.
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.
yes, that is needed, otherwise it will give an error "type" is not subscritble, it is because the python version restriction, snowpark pandas requires python 3.9 +, so have no problem with using dict, or list, but snowpark python still allows 3.8
@@ -100,6 +104,12 @@ def count_number_of_ctes(query): | |||
lambda x: x.to_df("a1", "b1").alias("L"), | |||
], | |||
) | |||
@sql_count_checker( |
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.
how about also adding describe_count here and other tests?
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.
the describe count check is embeded in check_result, which is used in this test point already.
there is also a limitation that the sql_counter_check annotation currently seems not working with describe_count yet, a ticket has been created here to https://snowflakecomputing.atlassian.net/browse/SNOW-1704919 to track the issue
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.
LGTM
0e89c42
to
c136db4
Compare
c136db4
to
8c7dd0d
Compare
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
SNOW-1566362
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.