-
Notifications
You must be signed in to change notification settings - Fork 110
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-878135 is_permanent behavior fix #989
SNOW-878135 is_permanent behavior fix #989
Conversation
Codecov Report
@@ Coverage Diff @@
## main #989 +/- ##
==========================================
+ Coverage 98.45% 98.46% +0.01%
==========================================
Files 51 51
Lines 9318 9317 -1
Branches 1696 1695 -1
==========================================
Hits 9174 9174
Misses 59 59
+ Partials 85 84 -1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
||
if parallel < 1 or parallel > 99: | ||
raise ValueError( | ||
"Supported values of parallel are from 1 to 99, " f"but got {parallel}" | ||
) | ||
|
||
return stage_location if is_permanent else None |
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 checking and warning about stage_location
here is a good idea, but for the actual fix, should we consider passing down is_permanent
to _do_register_udf
, instead of using stage_location
as a proxy to deduce its value? Will that be less error prone?
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.
Yeah. I think that is the right way to do things here. I'll reformat the code.
Does it make sense to get rid of |
Not to me. If we get rid of it, do you suggest we use |
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.
We probably also want to add similar test to udtf
and udaf
?
CHANGELOG.md
Outdated
@@ -9,6 +9,10 @@ | |||
- `array_flatten` | |||
- Added support for replicating your local Python environment on Snowflake via `Session.replicate_local_environment`. | |||
|
|||
### Behavior Changes | |||
|
|||
- When creating stored procedures, UDFs, UDTFs, UDAFs with parameter `is_permanent=False` will now create temporary objects even when `stage_name` is provided. Earlier `is_permanent=False` with non-None `stage_name` try to create permanent objects at the given stage location. |
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 also need to call out that the default behavior is changing because the default value of is_permanent
is None
. So the behavior of a UDF that only has stage_name
specified will see their UDF becomes temporary.
@@ -790,7 +793,7 @@ def _do_register_sp( | |||
object_name=udf_name, | |||
all_imports=all_imports, | |||
all_packages=all_packages, | |||
is_temporary=stage_location is None, | |||
is_temporary=not is_permanent, |
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 wonder if it makes sense to rename is_temporary
to is_permanent
? This way we only need to do negation once instead of in all four classes.
We probably should have made the public param is_temporary
at the beginning so we don't need this awkward flip lol. But now all is done 🤷
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.
we could rename is_temporary -> is_permanent
. public API already has is_permanent
so we shouldn't change that.
What about something like |
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-878135
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.