-
Notifications
You must be signed in to change notification settings - Fork 112
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-1791241] Add a session parameter to disable scoped read only temp table #2587
Conversation
self._conn._get_client_side_session_parameter( | ||
_PYTHON_SNOWPARK_ENABLE_SCOPED_TEMP_READ_ONLY_TABLE, False |
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.
If it's for native app, I think it's easier to look up from self._conn._conn._session_parameters['<param_name>']
, similar to what we did for ENABLE_FIX_1375538
, WDYT @sfc-gh-sfan?
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.
oh, i believe that function does look into the _conn._session_parameters, here is the implementation
def _get_client_side_session_parameter(self, name: str, default_value: Any) -> Any:
"""It doesn't go to Snowflake to retrieve the session parameter.
Use this only when you know the Snowflake session parameter is sent to the client when a session/connection is created.
"""
return (
self._conn._session_parameters.get(name, default_value)
if self._conn._session_parameters
else default_value
)
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.
Cool! I think you need to define a server side parameter too, similar to PYTHON_SNOWPARK_ENABLE_THREAD_SAFE_SESSION
above: https://sourcegraph.c2.us-central1.gcp-dev.app.snowflake.com/github.com/snowflakedb/snowflake/-/blob/GlobalServices/src/main/java/com/snowflake/core/parameters/SnowparkEcosystemParameters.java?L774:42
And default it to false.
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.
@sfc-gh-deliu i will follow up with you on this, i though you mentioned that i don't need a server side parameter, and you can set this up in the session automatically
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.
@sfc-gh-deliu i double checked with @sfc-gh-sfan , we do not need a definition for this parameter at server side. but he suggest we have a parameter control for turning this on for native app. I think we can do that when doing the server side change
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 think we should have a changelog entry for this.
@sfc-gh-helmeleegy added a changelog as part of fix. |
Let's wait for the response of Rich Murnane to our question here - to be able to assess the impact - before we go ahead and merge this. |
@sfc-gh-helmeleegy I don't think we need to wait for the native app response, our impact on notebook is much bigger than the native app at this moment, the native app user can stay with 1.24.0 if they would like, or turn on the flag if the user would like to. |
SGTM. I will approve. The native app response when it comes (hopefully soon) can help us decide if there are users we need to communicate with to turn off the flag. |
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.
CHANGELOG.md
Outdated
@@ -53,6 +53,7 @@ | |||
- Treat an empty `subset` (e.g. `[]`) as if it specified all columns instead of no columns. | |||
- Raise a `TypeError` for a scalar `subset` instead of filtering on just that column. | |||
- Raise a `ValueError` for a `subset` of type `pandas.Index` instead of filtering on the columns in the index. | |||
- Disable creation of scoped read only table to mitigate dynamic pivot unfound table issue in notebook env. |
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 would suggest we describe the kind of error this would bubble earlier. For example
- Disable creation of scoped read only table to mitigate dynamic pivot unfound table issue in notebook env. | |
- Disable creation of scoped read only table to mitigate `TableNotFoundError` when using dynamic pivot in notebook environment. |
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.
udpated
fa87774
to
9f5e9d3
Compare
37e338f
to
cae1966
Compare
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
SNOW-1791241
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
scoped read only table doesn't work with dynamic pivot in notebook today, investigation needed.
We added a flag to turns the scoped temp read only table off for snowpark padnas.
Follow up: https://snowflakecomputing.atlassian.net/browse/SNOW-1793002 enable the flag at server side for native app