SNOW-919476: Do not ignore stage location for using imports when is_permanent is False

[sfc-gh-aalam]

Please answer these questions before submitting your pull requests. Thanks!

1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

   Fixes #SNOW-919476

2. 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 adding new credentials
   • I am adding a new dependency

3. Please describe how your code solves the related issue.

   Please write a short description of how your code change solves the related issue.

[sfc-gh-aalam]

PR for fixing the merge-gate failures is here: #1054

[sfc-gh-sfan]

I'm not sure if this is the desired behavior (e.g. upload the udf zip file into a permanent stage location for a temp UDF). Can we instead separate the concern of upload and imports? For example, we could have an upload stage and an import stage.

[sfc-gh-aalam]

I'm not sure if this is the desired behavior (e.g. upload the udf zip file into a permanent stage location for a temp UDF). Can we instead separate the concern of upload and imports? For example, we could have an upload stage and an import stage.

This is a really good suggestion. I have updated the code to use this logic because I got confused about the context and started looking at stage location only as an upload location.

This is the desired behavior from temp sprocs:

• temp sprocs should not upload packages to permanent stages
• temp sprocs should be able to import pacakges from permanent stages
• temp sprocs should be able to use custom packages using imports
• temp sprocs should be able to import files from permanent stage and also use custom packages from imports

[sfc-gh-sfan]

Can we verify if this could lead to bugs (e.g. over-unwrapping)?

[sfc-gh-aalam]

there are some tests in test_utils_suite.py/test_normalize_stage_location which test for this.

[sfc-gh-aalam]

Added more tests. Overwrapping does not lead to bugs

[sfc-gh-sfan]

This is the desired behavior from temp sprocs:

• temp sprocs should not upload packages to permanent stages
• temp sprocs should be able to import pacakges from permanent stages
• temp sprocs should be able to use custom packages using imports
• temp sprocs should be able to import files from permanent stage and also use custom packages from imports

Let's make sure this is documented and tested :)

[sfc-gh-aalam]

This is the desired behavior from temp sprocs:

• temp sprocs should not upload packages to permanent stages
• temp sprocs should be able to import pacakges from permanent stages
• temp sprocs should be able to use custom packages using imports
• temp sprocs should be able to import files from permanent stage and also use custom packages from imports

Let's make sure this is documented and tested :)

Yep. I'm working on the tests now.

[sfc-gh-stan]

Let's define import_stage first then upload_stage = import_stage if is_permanent else session_stage.

[sfc-gh-stan]

I wonder if there's a better name for upload_stage, because technically we need to upload files to import_stage too. How about something like sproc_stage vs import_stage?
In addition, could you please add a one line documentation for these args to clarify?

[sfc-gh-aalam]

I agree with your point but the stage is used for udfs as well. How about import_only_stage and upload_and_import_stage?

[sfc-gh-sfan]

I don't think we upload files to import stage, but it is the other way around (we import the upload stage). And as Afroz mentioned, the import/upload difference is application to UDF families as well. I cannot come up with a better name than what Afroz suggested.

[sfc-gh-aalam]

done

[sfc-gh-sfan]

Let's remove this comment if we already have tests :p