-
Notifications
You must be signed in to change notification settings - Fork 117
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-1347394: Remove BaseQueryCompiler + error in QC default to pandas #1454
Conversation
7cd720f
to
bfd299f
Compare
src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py
Show resolved
Hide resolved
src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py
Outdated
Show resolved
Hide resolved
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 @sfc-gh-joshi!
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 Jonathan!
if kwargs: | ||
args_str += ", ".join(f"{key}={value}" for key, value in kwargs.items()) | ||
ErrorMessage.not_implemented( | ||
f"Snowpark pandas doesn't yet support {object_type}.{fn_name}({args_str}" |
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.
nit: ...doesn't yet support the method {object_type}.{fn_name}
Do we not want the message to include the specified parameters? Some query
compiler methods directly call DataFrameDefault.register when certain
parameter combinations are encountered, while the method is generally
supported.
…On Tue, Apr 30, 2024 at 17:34 Naresh Kumar ***@***.***> wrote:
***@***.**** approved this pull request.
------------------------------
In
src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py
<#1454 (comment)>
:
> - )
- return_query_compiler.snowpark_pandas_api_calls.append(
- {
- TelemetryField.NAME.value: f"{object_type}.{fn_name}",
- SnowparkPandasTelemetryField.IS_FALLBACK.value: True,
- }
+ # Previously, Snowpark pandas would register a stored procedure that materializes the frame
+ # and performs the native pandas operation. Because this fallback has extremely poor
+ # performance, we now raise NotImplementedError instead.
+ args_str = ", ".join(map(str, args))
+ if args and kwargs:
+ args_str += ", "
+ if kwargs:
+ args_str += ", ".join(f"{key}={value}" for key, value in kwargs.items())
+ ErrorMessage.not_implemented(
+ f"Snowpark pandas doesn't yet support {object_type}.{fn_name}({args_str}"
nit: ...doesn't yet support the method {object_type}.{fn_name}
—
Reply to this email directly, view it on GitHub
<#1454 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BDT7LZVBBULMQ3PNKXN5IMTZAA2C7AVCNFSM6AAAAABG7D2UY6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMZSHA2TOMBVGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Co-authored-by: Naren Krishna <[email protected]>
93f2366
to
2f457ca
Compare
Please answer these questions before submitting your pull requests. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-1347394
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
This PR removes our vendored copy of the
BaseQueryCompiler
class, inheriting the class from upstream Modin instead. Similarly, it removes all the operator registration classes defined insnowflake.snowpark.modin.core.dataframe.algebra.default2pandas
, with one exception. Upstream Modin does not properly render the names ofproperty
objects (modin-project/modin#7233), so we should overrideDataFrameDefault.register
to fix this until this issue is fixed upstream.This PR incidentally removes
Series.dt.week
+Series.dt.weekofyear
, which were already removed in pandas 2.0.