SNOW-1325701: Use scoped temp object in write pandas #2068

sfc-gh-yuwang · 2024-10-10T21:52:18Z

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 #NNNN
Fill out the following pre-review checklist:
- I am adding a new automated test(s) to verify correctness of my new code
- I am adding new logging messages
- I am adding a new telemetry message
- I am modifying authorization mechanisms
- I am adding new credentials
- I am modifying OCSP code
- I am adding a new dependency
Please describe how your code solves the related issue.

Please write a short description of how your code change solves the related issue.
(Optional) PR for stored-proc connector:

sfc-gh-aling · 2024-10-14T18:52:15Z

DESCRIPTION.md

@@ -7,6 +7,8 @@ https://docs.snowflake.com/
 Source code is also available at: https://github.com/snowflakedb/snowflake-connector-python

 # Release Notes
+- v3.13.0(TBD)
+  - Improved `write_pandas` and `BindUploadAgent.upload` to create scoped temporary object instead of temporary object.


why do we need this change in the public connector?
I thought "scoped object" only applied to the concept of stored proc. is this for aligning the change with the sproc connector?

here is some context: https://docs.google.com/document/d/1qfJevIMEZL7O12H5pQK5WZG-a-uwA2K2cmFDusHjdXM/edit?usp=sharing.
I think mostly they want temp object being created as job-scoped object, and they also want to align with sp connector

Is my understanding correct?: for regular / non-SP session, "job" is equivalent to the session itself. So job-scoped objects are de facto session-scoped ones. Therefore this change does not affect behavior of regular sessions.

sfc-gh-aling · 2024-10-14T18:59:05Z

src/snowflake/connector/bind_upload_agent.py

        self,
        cursor: SnowflakeCursor,
        rows: list[bytes],
        stream_buffer_size: int = 1024 * 1024 * 10,
+        use_scoped_temp_object: bool = True,


do we need this as part of the public api/do we want users to control the behavior?

Will change this to private

sfc-gh-aling · 2024-10-14T19:00:21Z

src/snowflake/connector/pandas_tools.py

@@ -181,6 +209,7 @@ def write_pandas(
    overwrite: bool = False,
    table_type: Literal["", "temp", "temporary", "transient"] = "",
    use_logical_type: bool | None = None,
+    use_scoped_temp_object: bool = True,


same question, do we need this to be part of public api?

src/snowflake/connector/utils.py

test/integ/pandas/test_pandas_tools.py

src/snowflake/connector/bind_upload_agent.py

sfc-gh-aling

connector is not only used by snowpark, but also by non-snowpark users. I think we should make the job scoped change only for snowpark users.

src/snowflake/connector/pandas_tools.py

sfc-gh-zyao

The change looks good to me. Only 2 nitpicking comments. Thanks for looking into this!

sfc-gh-zyao · 2024-10-16T17:55:11Z

src/snowflake/connector/bind_upload_agent.py

+            cursor.connection._session_parameters.get(
+                _PYTHON_SNOWPARK_USE_SCOPED_TEMP_OBJECTS_STRING, False
+            )
+            if cursor.connection._session_parameters


(not a strong opinion) Will cursor.connection._session_parameters ever be None? iirc we always at least initialized to an empty dict, so we can even simplify do

self._use_scoped_temp_object = cursor.connection._session_parameters.get( _PYTHON_SNOWPARK_USE_SCOPED_TEMP_OBJECTS_STRING, False )

But to err on the side of caution is also a good idea.

I followed the practice in snowpark here, and this approach handled all possible errors too

We hit an issue with this change. The latest 3.12.3 broke our tests.

In our tests, we have a MockConnection class which mocks all the public functions of Connection. This change, however, started using a private class member (connection._session_parameters), which doesn't seem right.

sfc-gh-zyao · 2024-10-16T17:56:51Z

src/snowflake/connector/_utils.py

+
+
+TEMP_OBJECT_NAME_PREFIX = "SNOWPARK_TEMP_"
+ALPHANUMERIC = string.digits + string.ascii_lowercase


Nit: Is there any special reason that we set the character set to be number + lowercase, but then later uppercase the entire name?

same as above, I followed snowpark's practice, I guess because we need a special name format for scoped object

sfc-gh-aling · 2024-10-16T20:34:40Z

src/snowflake/connector/bind_upload_agent.py

+        self._stage_name = (
+            random_name_for_temp_object(TempObjectType.STAGE)
+            if self._use_scoped_temp_object
+            else "SYSTEMBIND"
+        )


actually I just realized that if _use_scoped_temp_object is True, then each time when we instantiate a BindUploadAgent (we do this when binding parameters), a new temp stage created.

is this the same behavior as the sproc connector change?

This is a tricky problem, the original design is that the temp stage name in bind upload agent is a constant. So that there is only one temp stage created by it.
what i can think of is creating a constant stage name following the naming rule, what do you think?

src/snowflake/connector/bind_upload_agent.py

sfc-gh-yuwang added 5 commits October 10, 2024 14:51

add scoped object

Loading
Loading status checks…

931e77d

fix test

Loading
Loading status checks…

c6dd57f

fix test

Loading
Loading status checks…

8613ee4

add binduploadagent

Loading
Loading status checks…

196ed64

fix test

Loading
Loading status checks…

8c13a69

sfc-gh-yuwang marked this pull request as ready for review October 11, 2024 18:25

sfc-gh-yuwang requested a review from sfc-gh-aling October 11, 2024 18:26

fix test

Loading
Loading status checks…

acf25bf

sfc-gh-aling reviewed Oct 14, 2024

View reviewed changes

src/snowflake/connector/utils.py Outdated Show resolved Hide resolved

sfc-gh-aling reviewed Oct 14, 2024

View reviewed changes

test/integ/pandas/test_pandas_tools.py Outdated Show resolved Hide resolved

address comments

Loading
Loading status checks…

ffbe2d7

sfc-gh-yuwang added the NO-CHANGELOG-UPDATES label Oct 14, 2024

sfc-gh-yuwang requested a review from sfc-gh-aling October 14, 2024 23:02

sfc-gh-aling reviewed Oct 15, 2024

View reviewed changes

src/snowflake/connector/bind_upload_agent.py Outdated Show resolved Hide resolved

sfc-gh-aling reviewed Oct 15, 2024

View reviewed changes

src/snowflake/connector/bind_upload_agent.py Outdated Show resolved Hide resolved

sfc-gh-aling reviewed Oct 15, 2024

View reviewed changes

sfc-gh-yuwang added 2 commits October 15, 2024 09:41

default to false

Loading
Loading status checks…

26a519d

add test

Loading
Loading status checks…

d1bb9e6

sfc-gh-yuwang requested a review from sfc-gh-zyao October 15, 2024 18:51

sfc-gh-aling reviewed Oct 16, 2024

View reviewed changes

src/snowflake/connector/pandas_tools.py Outdated Show resolved Hide resolved

sfc-gh-zyao approved these changes Oct 16, 2024

View reviewed changes

do not create new cursor

Loading
Loading status checks…

19c5826

sfc-gh-aling reviewed Oct 16, 2024

View reviewed changes

use constant stage name for bind upload

Loading
Loading status checks…

b0e0427

sfc-gh-aling reviewed Oct 16, 2024

View reviewed changes

src/snowflake/connector/bind_upload_agent.py Show resolved Hide resolved

do not change defaukt name

Loading
Loading status checks…

53284a1

sfc-gh-aling approved these changes Oct 16, 2024

View reviewed changes

sfc-gh-yuwang merged commit a959fc6 into main Oct 17, 2024
93 of 94 checks passed

sfc-gh-yuwang deleted the SNOW-1325701 branch October 17, 2024 16:48

github-actions bot locked and limited conversation to collaborators Oct 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNOW-1325701: Use scoped temp object in write pandas #2068

SNOW-1325701: Use scoped temp object in write pandas #2068

sfc-gh-yuwang commented Oct 10, 2024

sfc-gh-aling Oct 14, 2024 •

edited

Loading

sfc-gh-yuwang Oct 14, 2024

sfc-gh-zyao Oct 16, 2024

sfc-gh-aling Oct 14, 2024

sfc-gh-yuwang Oct 14, 2024

sfc-gh-aling Oct 14, 2024

sfc-gh-aling left a comment •

edited

Loading

sfc-gh-zyao left a comment

sfc-gh-zyao Oct 16, 2024

sfc-gh-yuwang Oct 16, 2024

sfc-gh-smao Oct 25, 2024

sfc-gh-zyao Oct 16, 2024 •

edited

Loading

sfc-gh-yuwang Oct 16, 2024

sfc-gh-aling Oct 16, 2024

sfc-gh-yuwang Oct 16, 2024



		TEMP_OBJECT_NAME_PREFIX = "SNOWPARK_TEMP_"
		ALPHANUMERIC = string.digits + string.ascii_lowercase

SNOW-1325701: Use scoped temp object in write pandas #2068

SNOW-1325701: Use scoped temp object in write pandas #2068

Conversation

sfc-gh-yuwang commented Oct 10, 2024

sfc-gh-aling Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-aling left a comment • edited Loading

Choose a reason for hiding this comment

sfc-gh-zyao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-zyao Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-aling Oct 14, 2024 •

edited

Loading

sfc-gh-aling left a comment •

edited

Loading

sfc-gh-zyao Oct 16, 2024 •

edited

Loading