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.
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
V2.0 Definition for snowpark #1402
V2.0 Definition for snowpark #1402
Changes from 15 commits
8f9b89a
1603dd8
06c6d63
45703d9
0884c45
55761e9
f226451
e4ef4be
ac53ecc
10fc53f
a3db93b
cf933f2
563886d
5a4a7a0
403e3c7
67710b8
01c284e
8553cf7
83e5012
6a4cc33
aca0d21
4c5ab82
2cc7c4f
9128470
73e29a6
3aad2eb
0747807
a7d449a
2766b72
659636f
be0d58f
221d5e5
4176171
bff4b4d
33ac466
1f6d50c
ccdea97
bd98653
91b7a5f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
source
andimports
should be replaced withartifacts
as per designThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it seems, that
artifacts
mean local files that will be uploaded - soimports
should be separated, as those are the files that already exist server-side, and just need to be included in the SQL declarationThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change neccessary? Are you building on current main?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just for simplicity - same thing is used 4 times, so seems reasonable to put it into variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really being added in this PR? How does it releat to Snowpark?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was moved from line 165. It seems a bit more consistent to have attributes first, then methods - not attribute, method, attribute, method etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's limit the changed to snowpark only. It's easier to revert or review when changes has well defined scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How this change relates to snowpark?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again - it was moved in file, to make the code more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done