Skip to content

SNOW-2098847: Do not use "scoped temporary" stage for XML reader in stored procedure #3354

Merged
sfc-gh-jdu merged 1 commit intomainfrom
jdu-SNOW-2098847-fix-stage-xml
May 14, 2025
Merged

SNOW-2098847: Do not use "scoped temporary" stage for XML reader in stored procedure #3354
sfc-gh-jdu merged 1 commit intomainfrom
jdu-SNOW-2098847-fix-stage-xml

Conversation

@sfc-gh-jdu
Copy link
Copy Markdown
Collaborator

  1. Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-2098847

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
      • If this test skips Local Testing mode, I'm requesting review from @snowflakedb/local-testing
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am adding new credentials
    • I am adding a new dependency
    • If this is a new feature/behavior, I'm adding the Local Testing parity changes.
    • I acknowledge that I have ensured my changes to be thread-safe. Follow the link for more information: Thread-safe Developer Guidelines
    • If adding any arguments to public Snowpark APIs or creating new public Snowpark APIs, I acknowledge that I have ensured my changes include AST support. Follow the link for more information: AST Support Guidelines
  3. Please describe how your code solves the related issue.

    register_from_file() doesn't work in stored procedure when calling the UDF, if the underlying stage (session stage) is scoped temporary.

@sfc-gh-jdu sfc-gh-jdu requested review from a team as code owners May 12, 2025 21:10
@sfc-gh-jdu sfc-gh-jdu requested a review from sfc-gh-aling May 12, 2025 21:10
@sfc-gh-jdu sfc-gh-jdu added the NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md label May 12, 2025
@sfc-gh-snowflakedb-snyk-sa
Copy link
Copy Markdown

sfc-gh-snowflakedb-snyk-sa commented May 12, 2025

🎉 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-jdu sfc-gh-jdu force-pushed the jdu-SNOW-2098847-fix-stage-xml branch from ac3a34a to d18d851 Compare May 13, 2025 18:32
@sfc-gh-jdu sfc-gh-jdu force-pushed the jdu-SNOW-2098847-fix-stage-xml branch from d18d851 to d0a482a Compare May 13, 2025 20:48
@sfc-gh-jdu sfc-gh-jdu force-pushed the jdu-SNOW-2098847-fix-stage-xml branch from d0a482a to 113eca5 Compare May 13, 2025 23:17
Comment on lines +1136 to +1137
_, input_types = get_types_from_type_hints(
(XML_READER_FILE_PATH, handler_name), TempObjectType.TABLE_FUNCTION
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There appears to be a path inconsistency in the type hint resolution. The code is using XML_READER_FILE_PATH to extract type hints, but then using python_file_path when calling register_from_file(). In the stored procedure case, these paths differ significantly - XML_READER_FILE_PATH is a local filesystem path while python_file_path is a stage path.

For consistency and correctness, the same path should be used in both places. Consider modifying the code to use python_file_path for both the type hint extraction and the registration.

Suggested change
_, input_types = get_types_from_type_hints(
(XML_READER_FILE_PATH, handler_name), TempObjectType.TABLE_FUNCTION
_, input_types = get_types_from_type_hints(
(python_file_path, handler_name), TempObjectType.TABLE_FUNCTION

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

@sfc-gh-jdu sfc-gh-jdu merged commit 4b5feb9 into main May 14, 2025
39 of 41 checks passed
@sfc-gh-jdu sfc-gh-jdu deleted the jdu-SNOW-2098847-fix-stage-xml branch May 14, 2025 01:57
@github-actions github-actions bot locked and limited conversation to collaborators May 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants