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

accept stage paths starting with / for GET #2889

Merged
merged 3 commits into from
Jan 28, 2025

Conversation

sfc-gh-tvanderkooy
Copy link
Contributor

@sfc-gh-tvanderkooy sfc-gh-tvanderkooy commented Jan 20, 2025

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

    Fixes SNOW-1793953 (SNOW-1889627: SNOW-1793953: Allow relative path starting with / for GET #2888)

  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
  3. Please describe how your code solves the related issue.

    This builds on the changes from Add support for snow:// stage paths #1346 to allow / as a remote path prefix so file.get('/some/path')will not try to prepend @ to the path. This form of relative path is valid in some native application uses cases.

Copy link

github-actions bot commented Jan 20, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@sfc-gh-tvanderkooy
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@sfc-gh-aalam
Copy link
Contributor

@sfc-gh-tvanderkooy can you fix the failing tests please. change looks good

@sfc-gh-tvanderkooy sfc-gh-tvanderkooy force-pushed the tvanderkooy-SNOW-1793953-get_relative_path branch from 9ba13cf to 59a7d69 Compare January 27, 2025 15:19
@sfc-gh-tvanderkooy sfc-gh-tvanderkooy added the NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md label Jan 27, 2025
@sfc-gh-tvanderkooy
Copy link
Contributor Author

Turns out the test failures were because my change breaks #2470, which adds an explicit error for using a non-stage path with session.read.<file_format>(<path). These functions reads files from a stage that are some table format (avro, csv, etc.) into a data frame, and the error is to help users that are trying to use these functions with local files (which doesn't work).

I think it's unlikely for native app providers to want to use this in their app -- any file that is valid for these functions they could simply share as a table instead of putting as a file in the version stage -- so I opted to NOT support the relative stage path syntax for this case and preserve the existing error message.

cc @sfc-gh-jrose @sfc-gh-aalam

Comment on lines -375 to +379
prefixes = ["file://"] if is_local else SNOWFLAKE_PATH_PREFIXES
prefixes = ["file://"] if is_local else SNOWFLAKE_PATH_PREFIXES_FOR_GET
Copy link
Contributor

Choose a reason for hiding this comment

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

this change adds support when run inside a native apps environment. How does it affect when run outside NA? If it is not supported, then we should only allow the new prefix in NA env.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Outside native apps this will be a SQL error. SQL compilation error: invalid URL prefix found in: '/some/path'

But that's the current behaviour too (for GET) - today if you do session.file.get('/some/path'), snowpark will prepend @ and run the GET, and then throw SQL compilation error: missing stage name in URL: @/some/path

Unfortunately I'm not aware of a good way to check if this is NA env, it might be difficult to check that here.

@sfc-gh-tvanderkooy sfc-gh-tvanderkooy force-pushed the tvanderkooy-SNOW-1793953-get_relative_path branch from 3fb1be2 to d5d5757 Compare January 28, 2025 18:39
@sfc-gh-tvanderkooy sfc-gh-tvanderkooy force-pushed the tvanderkooy-SNOW-1793953-get_relative_path branch from d5d5757 to 96de4a1 Compare January 28, 2025 19:00
@sfc-gh-aalam sfc-gh-aalam removed the NO-CHANGELOG-UPDATES This pull request does not need to update CHANGELOG.md label Jan 28, 2025
@sfc-gh-aalam sfc-gh-aalam enabled auto-merge (squash) January 28, 2025 22:34
@sfc-gh-aalam sfc-gh-aalam disabled auto-merge January 28, 2025 22:53
@sfc-gh-aalam sfc-gh-aalam enabled auto-merge (squash) January 28, 2025 22:53
@sfc-gh-aalam sfc-gh-aalam merged commit 6fc3bee into main Jan 28, 2025
33 of 36 checks passed
@sfc-gh-aalam sfc-gh-aalam deleted the tvanderkooy-SNOW-1793953-get_relative_path branch January 28, 2025 23:29
@github-actions github-actions bot locked and limited conversation to collaborators Jan 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants