Skip to content
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

Merged
merged 8 commits into from
Aug 8, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,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.
Copy link
Collaborator

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.


## 1.6.1 (2023-08-02)

### New Features
Expand Down
9 changes: 8 additions & 1 deletion src/snowflake/snowpark/_internal/udf_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ def check_register_args(
is_permanent: bool = False,
stage_location: Optional[str] = None,
parallel: int = 4,
):
) -> Optional[str]:
if is_permanent:
if not name:
raise ValueError(
Expand All @@ -341,12 +341,19 @@ def check_register_args(
raise ValueError(
f"stage_location must be specified for permanent {get_error_message_abbr(object_type)}"
)
else:
if stage_location:
logger.warn(
"is_permanent is False therefore stage_location will be ignored"
)

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
Copy link
Collaborator

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?

Copy link
Contributor Author

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.



def check_execute_as_arg(execute_as: typing.Literal["caller", "owner"]):
if (
Expand Down
4 changes: 2 additions & 2 deletions src/snowflake/snowpark/stored_procedure.py
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ def register(
)

check_execute_as_arg(execute_as)
check_register_args(
stage_location = check_register_args(
TempObjectType.PROCEDURE, name, is_permanent, stage_location, parallel
)

Expand Down Expand Up @@ -642,7 +642,7 @@ def register_from_file(
- :meth:`register`
"""
file_path = process_file_path(file_path)
check_register_args(
stage_location = check_register_args(
TempObjectType.PROCEDURE, name, is_permanent, stage_location, parallel
)
check_execute_as_arg(execute_as)
Expand Down
4 changes: 2 additions & 2 deletions src/snowflake/snowpark/udaf.py
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,7 @@ def register(
f"Invalid handler: expecting a class type, but get {type(handler)}"
)

check_register_args(
stage_location = check_register_args(
TempObjectType.AGGREGATE_FUNCTION,
name,
is_permanent,
Expand Down Expand Up @@ -529,7 +529,7 @@ def register_from_file(
- :meth:`register`
"""
file_path = process_file_path(file_path)
check_register_args(
stage_location = check_register_args(
TempObjectType.AGGREGATE_FUNCTION,
name,
is_permanent,
Expand Down
4 changes: 2 additions & 2 deletions src/snowflake/snowpark/udf.py
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,7 @@ def register(
f"(__call__ is not defined): {type(func)}"
)

check_register_args(
stage_location = check_register_args(
TempObjectType.FUNCTION, name, is_permanent, stage_location, parallel
)

Expand Down Expand Up @@ -720,7 +720,7 @@ def register_from_file(
- :meth:`register`
"""
file_path = process_file_path(file_path)
check_register_args(
stage_location = check_register_args(
TempObjectType.FUNCTION, name, is_permanent, stage_location, parallel
)

Expand Down
4 changes: 2 additions & 2 deletions src/snowflake/snowpark/udtf.py
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ def register(
f"(__call__ is not defined): {type(handler)}"
)

check_register_args(
stage_location = check_register_args(
TempObjectType.TABLE_FUNCTION, name, is_permanent, stage_location, parallel
)

Expand Down Expand Up @@ -614,7 +614,7 @@ def register_from_file(
- :meth:`register`
"""
file_path = process_file_path(file_path)
check_register_args(
stage_location = check_register_args(
TempObjectType.TABLE_FUNCTION, name, is_permanent, stage_location, parallel
)

Expand Down
35 changes: 35 additions & 0 deletions tests/integ/test_stored_procedure.py
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ def test_call_named_stored_procedure(session, temp_schema, db_parameters):
stage_location=unwrap_stage_location_single_quote(
tmp_stage_name_in_temp_schema
),
is_permanent=True,
)
assert new_session.call(full_sp_name, 13, 19) == 13 + 19
# oen result in the temp schema
Expand Down Expand Up @@ -575,6 +576,40 @@ def test_permanent_sp(session, db_parameters):
Utils.drop_stage(session, stage_name)


@pytest.mark.skipif(IS_IN_STORED_PROC, reason="Cannot create session in SP")
def test_permanent_sp_negative(session, db_parameters, caplog):
stage_name = Utils.random_stage_name()
sp_name = Utils.random_name_for_temp_object(TempObjectType.PROCEDURE)
with Session.builder.configs(db_parameters).create() as new_session:
new_session.sql_simplifier_enabled = session.sql_simplifier_enabled
new_session.add_packages("snowflake-snowpark-python")
try:
with caplog.at_level(logging.WARN):
sproc(
lambda session_, x, y: session_.sql(f"SELECT {x} + {y}").collect()[
0
][0],
return_type=IntegerType(),
input_types=[IntegerType(), IntegerType()],
name=sp_name,
is_permanent=False,
stage_location=stage_name,
session=new_session,
)
assert (
"is_permanent is False therefore stage_location will be ignored"
in caplog.text
)

with pytest.raises(
SnowparkSQLException, match=f"Unknown function {sp_name}"
):
session.call(sp_name, 1, 2)
assert new_session.call(sp_name, 8, 9) == 17
finally:
new_session._run_query(f"drop function if exists {sp_name}(int, int)")


@pytest.mark.skipif(not is_pandas_available, reason="Requires pandas")
def test_sp_negative(session):
def f(_, x):
Expand Down
35 changes: 35 additions & 0 deletions tests/integ/test_udf.py
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ def test_call_named_udf(session, temp_schema, db_parameters):
stage_location=unwrap_stage_location_single_quote(
tmp_stage_name_in_temp_schema
),
is_permanent=True,
)
Utils.check_answer(
new_session.sql(f"select {full_udf_name}(13, 19)").collect(), [Row(13 + 19)]
Expand Down Expand Up @@ -967,6 +968,40 @@ def test_permanent_udf(session, db_parameters):
Utils.drop_stage(session, stage_name)


@pytest.mark.skipif(IS_IN_STORED_PROC, reason="Cannot create session in SP")
def test_permanent_udf_negative(session, db_parameters, caplog):
stage_name = Utils.random_stage_name()
udf_name = Utils.random_name_for_temp_object(TempObjectType.FUNCTION)
with Session.builder.configs(db_parameters).create() as new_session:
new_session.sql_simplifier_enabled = session.sql_simplifier_enabled
try:
with caplog.at_level(logging.WARN):
udf(
lambda x, y: x + y,
return_type=IntegerType(),
input_types=[IntegerType(), IntegerType()],
name=udf_name,
is_permanent=False,
stage_location=stage_name,
session=new_session,
)
assert (
"is_permanent is False therefore stage_location will be ignored"
in caplog.text
)

with pytest.raises(
SnowparkSQLException, match=f"Unknown function {udf_name}"
):
session.sql(f"select {udf_name}(8, 9)").collect()

Utils.check_answer(
new_session.sql(f"select {udf_name}(8, 9)").collect(), [Row(17)]
)
finally:
new_session._run_query(f"drop function if exists {udf_name}(int, int)")


def test_udf_negative(session):
def f(x):
return x
Expand Down
Loading