SNOW-1787415 Fix dataframe ast to register udtf for replay execution using client created udtf in same client session #2620
+1,106
−230
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sfc-gh-evandenberg
requested review from
sfc-gh-yixie,
sfc-gh-yuwang and
sfc-gh-jrose
and removed request for
a team
sfc-gh-evandenberg
force-pushed
the
evandenberg-SNOW-1787415-fix-functions-register-udtf-same-session
branch
from
34f09cd
to
99bacfb
Compare
| @@ -953,6 +953,7 @@ def build_proto_from_callable( | |||
| expr_builder: proto.SpCallable, | |||
| func: Union[Callable, Tuple[str, str]], | |||
| ast_batch: Optional[AstBatch] = None, | |||
There was a problem hiding this comment.
nit: Could do _registered_object_name here as well.
| @@ -1166,7 +1170,8 @@ def build_udtf( | |||
| comment: Optional[str] = None, | |||
| statement_params: Optional[Dict[str, str]] = None, | |||
| is_permanent: bool = False, | |||
| session=None, | |||
| session: "snowflake.snowpark.session.Session" = None, | |||
| udtf_name: Optional[Union[str, Iterable[str]]] = None, | |||
There was a problem hiding this comment.
can we rename to not clash with name? Simply doing _registered_object_name should do.
| @@ -100,6 +100,7 @@ | |||
| "_from_pandas_udf_function", | |||
| "input_names", # for pandas_udtf | |||
| "max_batch_size", # for pandas_udtf | |||
| "_registered_object_name", # db object name if already registered | |||
There was a problem hiding this comment.
nit: remove if already registered, replace with something like object name within Snowflake (post-registration) or so.
There was a problem hiding this comment.
If it's None I assume the object has not been registered, if not none then it should be a valid object name?
| stmt = _ast_stmt | ||
| ast = None | ||
|
|
||
| # Note it's intentional the column expressions are AST serializerd earlier (ast_cols) to ensure any |
There was a problem hiding this comment.
typo: serializerd -> serialized
| statement_params=statement_params, | ||
| is_permanent=is_permanent, | ||
| session=self._session, | ||
| udtf_name=udtf_name, |
There was a problem hiding this comment.
can we rename udtf_name?
sfc-gh-lspiegelberg
approved these changes
Nov 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add Udtf registration that reuses the client registered UDTF. Doesn't need all the previous parameters as they were only required for original creation of udtf.
Fix Full AST Validation to use same session for replay execution of unparser AST, thereby allowing reuse of client defined UDTFs.