-
Notifications
You must be signed in to change notification settings - Fork 152
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
SNOW-878116 Add support for PARTITION BY to COPY INTO location #542
SNOW-878116 Add support for PARTITION BY to COPY INTO location #542
Conversation
All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
c9f90d9
to
c8dee12
Compare
src/snowflake/sqlalchemy/base.py
Outdated
partition_by_value = copy_into.partition_by | ||
|
||
partition_by = ( | ||
f"PARTITION BY {partition_by_value}" if partition_by_value != "" else "" |
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.
What happens if the actual partition_by_value is an empty string? The actual partition_by will have the value: "PARTITION BY". Is this the expected behaviour? I have the idea that if the partition_by_value is an empty string the partition_by variable should be empty or not taken into consideration when creating copy query command? Is this case even happening?
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.
Thanks for the observations here. I have updated the code to make partition_by_value None by default and check if the partition_by should be an empty string or not based on this as well.
Right now, if the partition_by_value variable is None (or empty string), then partition_by will be an empty string, effectively not adding anything related at all to the PARTITION BY to the COPY INTO statement.
The existing test case test_copy_into_location already validates the None case in most of the asserts where they do not have the PARTITION BY clause.
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.
I have added an extra test case where the empty string case is being validated 5179d3e
5179d3e
to
d374354
Compare
DESCRIPTION.md
Outdated
@@ -19,6 +19,7 @@ Source code is also available at: | |||
- Add support for iceberg table with Snowflake Catalog | |||
- Fix cluster by option to support explicit expressions | |||
- Add support for MAP datatype | |||
- Add support for partition by to copy into <location> |
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.
Add this to Unreleased section
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.
Thanks. Changed in 300a4fb
Please answer these questions before submitting your pull requests. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-878116:
CopyInto
does not support thePARTITION BY
clause #430Co-authored by @azban
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
This adds an argument to CopyInto which can be used to specify the PARTITION BY value.