-
Notifications
You must be signed in to change notification settings - Fork 109
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
NO-SNOW: fix sp-precommit test for large_query_breakdown and cte #2404
Conversation
@decorator | ||
def sql_count_checker( | ||
func, |
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.
updating the implementation because decorator
package is not available in SP
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.
Please see comments regarding failures:
sql_count_checker() got an unexpected keyword argument 'fallback_count'
tests/integ/utils/sql_counter.py
Outdated
@@ -376,17 +374,8 @@ def sql_count_checker( | |||
udf_count=None, | |||
udtf_count=None, | |||
union_count=None, | |||
*args, |
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 failure
"sql_count_checker() got an unexpected keyword argument 'fallback_count'"
happens because the *args, **kwargs are removed here.
Not sure if that was intentional. When I add them back, then the test_sql_counter.py tests pass.
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 for this. I assumed these were args
and kwargs
for the func
we are decorating so I moved them down to wrapper
. I moved it back up now.
@functools.wraps(func) | ||
def wrapper(*args, **kwargs): | ||
sql_counter = SqlCounter( | ||
no_check=no_check, |
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.
It looks like all tests that contain 'fallback_count=#' are disabled from being counted by pytest.mark.xfail:
@pytest.mark.xfail(
reason="SNOW-1336091: Snowpark pandas cannot run in sprocs until modin 0.28.1 is available in conda",
strict=True,
raises=RuntimeError,
)
but when you actually run the tests, they fail with some other error such as NotImplementedError.
I believe a long time ago we would go through a 'fallback' to run the code using pandas within a sproc. However, it looks like this was removed by https://github.com/snowflakedb/snowpark-python/pull/1542/files . I'd recommend to check with @sfc-gh-nkrishna here. Perhaps the disabled tests need to be re-evaluated to validate not implemented error rather than skipped in this way.
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
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-NNNNNNN
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Test fails because it cannot detect fixture
sql_simplifier_enabled
. Reading the value from session to workaround the issue.example: https://ci-dev-133.int.snowflakecomputing.com/job/RT-PC-TestWorker/203222/testReport/junit/(root)/t_python_sp_integ_sp/snowpark_test_large_query_breakdown_sql/