SNOW-2099580: fix Snowpark does not return a dataframe if default argument for called sp not given in session.call#3365
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) |
sfc-gh-aalam
left a comment
sfc-gh-aalam
left a comment
There was a problem hiding this comment.
It would also be nice if you could describe briefly why we cannot infer this on client -> for example using describe queries or describe procedure in the PR description
src/snowflake/snowpark/session.py
Outdated
| is_return_table: When set to a non-null value, it signifies whether the return type of sproc_name | ||
| is a table return type. This skips infer check and returns a dataframe with appropriate sql call. |
There was a problem hiding this comment.
Can we say,
When set to True, the return value of this function returns a DataFrame object. This is useful when the given stored procedure's return type is a table.
CHANGELOG.md
Outdated
| - Improved the error message of the XML reader when the specified row tag is not found in the file. | ||
| - Improved query generation for `Dataframe.drop` to use `SELECT * EXCLUDE ()` to exclude the dropped columns. To enable this feature, set `session.conf.set("use_simplified_query_generation", True)`. | ||
| - Added support for `VariantType` to `StructType.from_json` | ||
| - Added support for parameter `is_return_table` in `Session.call`, which control if a dataframe is returned. |
There was a problem hiding this comment.
| - Added support for parameter `is_return_table` in `Session.call`, which control if a dataframe is returned. | |
| - Added support for parameter `is_return_table` in `Session.call`, which can be used to set the return type of the functions a `DataFrame` object. |
src/snowflake/snowpark/session.py
Outdated
| *args: Any, | ||
| statement_params: Optional[Dict[str, Any]] = None, | ||
| log_on_exception: bool = False, | ||
| is_return_table: Optional[bool] = None, |
There was a problem hiding this comment.
do you think return_dataframe is a better kwarg here? From a users perspective, I think it makes more sense.
There was a problem hiding this comment.
I think it is better, do you think we should align it with _call() ?
| log_on_exception: Log warnings if they arise when trying to determine if the stored procedure | ||
| as a table return type. | ||
| return_dataframe: When set to True, the return value of this function is a DataFrame object. | ||
| This is useful when the given stored procedure's return type is a table. |
There was a problem hiding this comment.
let's also add comment about default None behavior
| #### Improvements | ||
|
|
||
| - Added support for reading XML files with namespaces using `rowTag` and `stripNamespaces` options. | ||
| - Added support for parameter `is_return_table` in `Session.call`, which can be used to set the return type of the functions a `DataFrame` object. |
There was a problem hiding this comment.
the call functions has parameter named return_dataframe while here it's called is_return_table
which name we want to use?
sfc-gh-aling
left a comment
sfc-gh-aling
left a comment
There was a problem hiding this comment.
please address my comment before merging
5 participants
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-NNNNNNN
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.