SNOW-2057867 refactor and fixes to make pandas write work for Python … #2304
Conversation
…sprocs - use _upload to implement write_pandas - correctly use scoped keyword and temp naming pattern for write_pandas - fix a minor bug where file_transfer_agent returns meta.dst_file_name as file name in download (dst_file_name may contain sub-directories, whereas name will be the base file name without any sub-directory prefixes. We should keep it consistent with upload, where we use meta.name as file name ### Tests - existing test_pandas_tools.py to make sure the change does not break existing write_pandas logic
sfc-gh-zyao
requested review from
sfc-gh-sfan,
sfc-gh-aling and
sfc-gh-mmishchenko
| @@ -818,7 +818,7 @@ def result(self) -> dict[str, Any]: | |||
|
|
|||
| rowset.append( | |||
| [ | |||
| meta.dst_file_name, | |||
| meta.name, | |||
There was a problem hiding this comment.
This is the returned result to user, right? Would this imply a behavior change?
There was a problem hiding this comment.
Yes it is the first item returned result, and yeah, I think I should do it in a separate PR and go through behavior change process.
Currently behavior is that
@stage_foo/test.txtwill have the resulttest.txt@stage_foo/dir_x/test.txtwill have the resulttest.txt@stage_foo/dir_x/subdir_y/test.txtwill have the resultsubdir_y/test.txt
With this change, the first 2 scenarios will remain the same, but the 3rd scenario will be test.txt.
Let me separate this change out to a standalone PR. Is there a standard process for me to follow for such behavior change(s)?
There was a problem hiding this comment.
As discussed, let's follow up with the connector team to see if this is the intended behavior. It seems like your change is more desired.
There was a problem hiding this comment.
Yes, let me start the conversation with connector team, and do follow-up changes based on the outcome of discussion.
…w a behavior change process for that
…or-sprocs' into zyao-SNOW-2057867-refactor-and-fix-write-pandas-to-make-it-work-for-sprocs
sfc-gh-zyao
added
NO-CHANGELOG-UPDATES
…or-sprocs' into zyao-SNOW-2057867-refactor-and-fix-write-pandas-to-make-it-work-for-sprocs
…or-sprocs' into zyao-SNOW-2057867-refactor-and-fix-write-pandas-to-make-it-work-for-sprocs
…or-sprocs' into zyao-SNOW-2057867-refactor-and-fix-write-pandas-to-make-it-work-for-sprocs
…or-sprocs' into zyao-SNOW-2057867-refactor-and-fix-write-pandas-to-make-it-work-for-sprocs
…or-sprocs' into zyao-SNOW-2057867-refactor-and-fix-write-pandas-to-make-it-work-for-sprocs
- this has no effect for regular / non-sproc use cases because it is the default option - sproc require it to be explicitly present, so we need it here (we will have a future server side change to make it optional as well for sproc)
…r regular temp object - the naming pattern applies to both regular temp and scoped temp, but here we only use that naming pattern for scoped temp, this is incorrect - it works fine for non-sproc (for now), but it will break for sproc - this change of naming pattern has no effect on customers because these are creating intermediate results, which is not consumed directly by customers
|
Hi @sfc-gh-sfan commit 30ce232 and e841eb4 are the ones that I mentioned earlier, could you please take a look and let me know if it looks good to you? |
LGTM |
…sprocs
Note that this is based on top of an in-review change #2303
Tests
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:
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: