-
Notifications
You must be signed in to change notification settings - Fork 132
SNOW-2081762: Handle no session during Sproc, UDxF registration #3331
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
base: main
Are you sure you want to change the base?
SNOW-2081762: Handle no session during Sproc, UDxF registration #3331
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
if self._session is not None: | ||
stmt = self._session._ast_batch.bind() | ||
ast = with_src_position(stmt.expr.stored_procedure, stmt) | ||
ast_id = stmt.uid | ||
else: | ||
ast = proto.StoredProcedure() |
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.
Is information being lost when the session is not initialized?
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.
The source information from where session.{sproc,udaf,udf,udtf}.register
is called will be lost.
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.
Why can't we still capture the source information when session is None
? source read only depends on reading python call stack and frames right?
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.
Eventually we will be able to once Snowpark object own their AST statements, but for now because Bind
statements can only be created in association with a Session
instance that owns an AstBatch
instance, we will lose the call stack and frames from this point in the code.
23c8d05
to
8dc25ec
Compare
session = ( | ||
self._session or snowflake.snowpark.session._get_active_session() | ||
) |
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.
is it possible session is still None here?
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.
As far as I can tell, if you try to call a UDxF without an active session, it means it will not have been registered in the first place.
This is likely moving the eventual failure slightly earlier in the pipeline, where if the session is still None here, _get_active_session()
will raise an error.
@@ -87,6 +88,7 @@ def __init__( | |||
packages: Optional[List[Union[str, ModuleType]]] = None, | |||
_ast: Optional[proto.Udf] = None, | |||
_ast_id: Optional[int] = None, | |||
_session: Optional["snowflake.snowpark.session.Session"] = 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 may overlook but I didn't see any caller passing the _session
parameter?
when is this parameter used?
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.
Thank you for catching this! Missed passing in the _session
parameter at the call sites in UDFRegistration
methods in this file.
8dc25ec
to
453ef2f
Compare
69506f3
to
0083cef
Compare
0083cef
to
8e02221
Compare
8e02221
to
f30c9bc
Compare
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-2081762
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Allow
Sproc/UDxF
creation/registration without a live session. An active session is assumed to be required at the call site of the resultantSproc/UDxF
.