Stage copy with directory structure #942

sfc-gh-astus · 2024-03-27T12:00:15Z

Pre-review checklist

I've confirmed that instructions included in README.md are still correct after my changes in the codebase.
I've added or updated automated unit tests to verify correctness of my new code.
I've added or updated integration tests to verify correctness of my new code.
I've confirmed that my changes are working by executing CLI's commands manually.
I've confirmed that my changes are up-to-date with the target branch.
I've described my changes in the release notes.
I've described my changes in the section below.

Changes description

stage copy from stage will reproduce the directory structure locally.

tests/__snapshots__/test_help_messages.ambr

tests/object/stage/__snapshots__/test_stage.ambr

sfc-gh-turbaszek · 2024-03-27T14:31:53Z

RELEASE-NOTES.md

@@ -21,6 +21,7 @@
 * Changing imports in function/procedure section in `snowflake.yml` will cause the definition update on replace
 * Adding `--pattern` flag to `stage list` command for filtering out results with regex.
 * Fixed snowpark build paths for builds with --project option (fixed empty zip issue).
+* `stage copy` from stage will reproduce the directory structure locally.


Hm, this is potentially breaking change? Should we introduce this behind a flag -r/--recursive or something?

Yes, it is :( . Added --recursive flag

Should we also add a warning for default use? In this way we will be able to prompt users to use --recursive always and later make it default behavior. What do you think?

Good idea, but I wonder if it won't be a breaking change

Warning is never a breaking change. We should utilize the cli_console.warning so it is automatically suppressed in formatted outputs

With --format JSON no warning will appear, so maybe it will not be a problem? 🤔

Added warning

src/snowflake/cli/api/output/types.py

sfc-gh-turbaszek · 2024-03-27T14:34:15Z

src/snowflake/cli/api/output/types.py

+class AggregatedResult(CollectionResult):
+    """
+    Prints results from many cursors as single result. Cursors must have the same number and order of columns.
+    """


What will happen if this is not true? Should we print a warning / fail in runtime?

It is even more complicated, so I decided to use MultipleResult.

tests/object/stage/test_stage.py

src/snowflake/cli/plugins/object/stage/commands.py

sfc-gh-turbaszek · 2024-03-29T09:43:37Z

src/snowflake/cli/plugins/object/stage/manager.py

+            dest_directory = dest_path / "/".join(
+                Path(file).parts[stage_parts_length:-1]
+            )
+            self._assure_is_existing_directory(Path(dest_directory))


Does it mean the directory has to exists?

Shouldn't get make sure it creates the directories? If I want to use this piece of code in other place, does it mean I have also to create all directories?

_assure_is_existing_directory is already existing function which checks if directory exists and creates it if not. Normal get uses the same function.

src/snowflake/cli/plugins/object/stage/manager.py

sfc-gh-turbaszek · 2024-04-02T10:47:17Z

src/snowflake/cli/plugins/object/stage/commands.py

@@ -68,12 +73,20 @@ def copy(
        raise click.ClickException(
            "Both source and target path are local. This operation is not supported."
        )
+    if is_put and recursive:
+        raise click.ClickException("Recursive for PUT is not supported.")


Suggested change

raise click.ClickException("Recursive for PUT is not supported.")

raise click.ClickException("Recursive for upload is not supported.")

Or should we ignore the flag?

I would leave exception, because we will support it soon. I hope 😄 . If we ignore it, command behaviour will change when we finally support it.

sfc-gh-turbaszek · 2024-04-02T10:49:17Z

src/snowflake/cli/plugins/object/stage/commands.py

@@ -68,12 +73,20 @@ def copy(
        raise click.ClickException(
            "Both source and target path are local. This operation is not supported."
        )
+    if is_put and recursive:
+        raise click.ClickException("Recursive for PUT is not supported.")

    if is_get:


Changing order of this if can improve readability, WDYT?

Suggested change

if is_get:

if not is_get:

I don't see that improvement 🤔 , for me it will not change anything. I added second if for put, WDYT?

if not is_get: # PUT case source = Path(source_path).resolve() local_path = str(source) + "/*" if source.is_dir() else str(source) cursor = StageManager().put( local_path=local_path, stage_path=destination_path, overwrite=overwrite, parallel=parallel, ) return QueryResult(cursor) # GET case if recursive: cursors = StageManager().get_recursive( stage_path=source_path, dest_path=target, parallel=parallel ) result = MultipleResults([QueryResult(c) for c in cursors]) else: cli_console.warning( "Use `--recursive` flag, which copy files recursively with directory structure. This will be the default behavior in the future." ) cursor = StageManager().get( stage_path=source_path, dest_path=target, parallel=parallel ) result = QueryResult(cursor)

But if we foresee more changes in put then introducing helper methods may make more sense:

if is_get: return _get(...) return _put(...)

WDYT?

I like helper methods

sfc-gh-turbaszek · 2024-04-02T13:41:21Z

src/snowflake/cli/plugins/object/stage/commands.py

+                overwrite=overwrite,
+                parallel=parallel,
+            )
+            return QueryResult(cursor)


Bad design, if for some reason we do not enter any of the if the function will return None causing unexpected behavior

sfc-gh-turbaszek · 2024-04-02T13:41:48Z

src/snowflake/cli/plugins/object/stage/commands.py

+            return MultipleResults([QueryResult(c) for c in cursors])
+        else:
+            cli_console.warning(
+                "Use `--recursive` flag, which copy files recursively with directory structure. This will be the default behavior in the future."
+            )
+            cursor = StageManager().get(
+                stage_path=source_path, dest_path=target, parallel=parallel
+            )
+            return QueryResult(cursor)


Is the output structure consistent in both cases if used with --format-json?

It wasn't :( , now it is

src/snowflake/cli/plugins/object/stage/commands.py

sfc-gh-turbaszek · 2024-04-04T08:30:40Z

src/snowflake/cli/plugins/object/stage/commands.py

+
+def _get(recursive: bool, source_path: str, destination_path: str, parallel: int):
+    target = Path(destination_path).resolve()
+    if recursive:


I would reverse the order if not recursive seems to be a shorter block. WDYT?

For me block are the same :) , changed

sfc-gh-zblackwood · 2024-04-04T12:49:08Z

src/snowflake/cli/plugins/object/stage/commands.py

+    overwrite: bool,
+):
+    if recursive:
+        raise click.ClickException("Recursive flag for upload is not supported.")


sfc-gh-zblackwood · 2024-04-04T12:50:07Z

Hooray! 🎉

Stage copy with directory structure

sfc-gh-astus requested a review from a team as a code owner March 27, 2024 12:00

sfc-gh-astus commented Mar 27, 2024

View reviewed changes

tests/__snapshots__/test_help_messages.ambr Show resolved Hide resolved

sfc-gh-astus commented Mar 27, 2024

View reviewed changes

tests/object/stage/__snapshots__/test_stage.ambr Show resolved Hide resolved

sfc-gh-astus force-pushed the stage-copy-with-directories branch 3 times, most recently from b0050f5 to 88f894f Compare March 27, 2024 13:38

sfc-gh-turbaszek reviewed Mar 27, 2024

View reviewed changes

src/snowflake/cli/api/output/types.py Outdated Show resolved Hide resolved

sfc-gh-turbaszek reviewed Mar 27, 2024

View reviewed changes

tests/object/stage/test_stage.py Outdated Show resolved Hide resolved

sfc-gh-astus force-pushed the stage-copy-with-directories branch 3 times, most recently from 5962b9a to 8d4b42b Compare March 28, 2024 12:03

sfc-gh-mraba previously approved these changes Mar 28, 2024

View reviewed changes

src/snowflake/cli/plugins/object/stage/commands.py Show resolved Hide resolved

sfc-gh-astus force-pushed the stage-copy-with-directories branch from 8d4b42b to 14abd3f Compare March 28, 2024 16:37

sfc-gh-turbaszek reviewed Mar 29, 2024

View reviewed changes

src/snowflake/cli/plugins/object/stage/manager.py Outdated Show resolved Hide resolved

sfc-gh-astus dismissed sfc-gh-mraba’s stale review via a7cdde4 March 29, 2024 10:49

sfc-gh-astus force-pushed the stage-copy-with-directories branch 2 times, most recently from a7cdde4 to 3cf62cd Compare March 29, 2024 11:05

sfc-gh-turbaszek reviewed Apr 2, 2024

View reviewed changes

sfc-gh-turbaszek mentioned this pull request Apr 2, 2024

Added execute immediate command #949

Merged

7 tasks

sfc-gh-astus force-pushed the stage-copy-with-directories branch from 3cf62cd to 31c1b6d Compare April 2, 2024 11:28

sfc-gh-turbaszek linked an issue Apr 2, 2024 that may be closed by this pull request

SNOW-1006186: snow object stage copy should return files in folders, not flatten out directory structure #643

Closed

sfc-gh-turbaszek reviewed Apr 2, 2024

View reviewed changes

sfc-gh-astus force-pushed the stage-copy-with-directories branch 2 times, most recently from 650d5c7 to 0e182d2 Compare April 3, 2024 11:46

sfc-gh-turbaszek added the future-bcr label Apr 3, 2024

sfc-gh-astus added 3 commits April 4, 2024 09:47

Stage copy with directory structure

b6628aa

Stage copy with directory structure

ce5e603

Stage copy with directory structure

5e28362

sfc-gh-astus force-pushed the stage-copy-with-directories branch from 0e182d2 to 5e28362 Compare April 4, 2024 07:47

sfc-gh-turbaszek reviewed Apr 4, 2024

View reviewed changes

src/snowflake/cli/plugins/object/stage/commands.py Outdated Show resolved Hide resolved

sfc-gh-turbaszek reviewed Apr 4, 2024

View reviewed changes

Stage copy with directory structure

c4f6a78

sfc-gh-turbaszek approved these changes Apr 4, 2024

View reviewed changes

sfc-gh-astus merged commit 61faedd into main Apr 4, 2024
11 checks passed

sfc-gh-astus deleted the stage-copy-with-directories branch April 4, 2024 10:49

sfc-gh-zblackwood reviewed Apr 4, 2024

View reviewed changes

sfc-gh-sichen pushed a commit that referenced this pull request Oct 17, 2024

Stage copy with directory structure (#942)

5b85cec

Stage copy with directory structure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stage copy with directory structure #942

Stage copy with directory structure #942

sfc-gh-astus commented Mar 27, 2024

sfc-gh-turbaszek Mar 27, 2024

sfc-gh-astus Mar 27, 2024

sfc-gh-turbaszek Mar 29, 2024

sfc-gh-astus Apr 2, 2024

sfc-gh-turbaszek Apr 2, 2024

sfc-gh-astus Apr 2, 2024

sfc-gh-astus Apr 2, 2024

sfc-gh-turbaszek Mar 27, 2024

sfc-gh-astus Mar 28, 2024

sfc-gh-turbaszek Mar 29, 2024

sfc-gh-astus Mar 29, 2024

sfc-gh-turbaszek Apr 2, 2024

sfc-gh-astus Apr 2, 2024

sfc-gh-turbaszek Apr 2, 2024

sfc-gh-astus Apr 2, 2024

sfc-gh-turbaszek Apr 2, 2024

sfc-gh-astus Apr 2, 2024

sfc-gh-turbaszek Apr 2, 2024

sfc-gh-astus Apr 2, 2024

sfc-gh-turbaszek Apr 2, 2024

sfc-gh-turbaszek Apr 2, 2024

sfc-gh-astus Apr 3, 2024

sfc-gh-turbaszek Apr 4, 2024

sfc-gh-astus Apr 4, 2024

sfc-gh-zblackwood Apr 4, 2024

sfc-gh-zblackwood commented Apr 4, 2024

	raise click.ClickException("Recursive for PUT is not supported.")
	raise click.ClickException("Recursive for upload is not supported.")

Stage copy with directory structure #942

Stage copy with directory structure #942

Conversation

sfc-gh-astus commented Mar 27, 2024

Pre-review checklist

Changes description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-zblackwood commented Apr 4, 2024