-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
docs:Add an example of CTAS with PARTITIONED BY (fix #3854) #4606
Conversation
LGTM, thanks @kyle-cx !! |
PARTITIONED BY (part) | ||
TBLPROPERTIES ('key'='value') | ||
AS SELECT ... |
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 think it would make more sense to either:
- Update the existing CTAS statement above to use partitioning plus tblproperties.
- Add a comment in between the two blocks indicating what's different between the two.
I would prefer (1) given that the CTAS syntax is not iceberg specific, this is just covering more Spark DML.
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.
…rove as suggestion from apache#4606 (comment)
docs/spark/spark-ddl.md
Outdated
@@ -91,11 +91,15 @@ Supported transformations are: | |||
|
|||
## `CREATE TABLE ... AS SELECT` | |||
|
|||
Iceberg supports CTAS as an atomic operation when using a [`SparkCatalog`](../spark-configuration#catalog-configuration). CTAS is supported, but is not atomic when using [`SparkSessionCatalog`](../spark-configuration#replacing-the-session-catalog). | |||
Iceberg supports CTAS as an atomic operation when using a [`SparkCatalog`](../spark-configuration#catalog-configuration). CTAS is supported, but is not atomic when using [`SparkSessionCatalog`](../spark-configuration#replacing-the-session-catalog). |
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.
Can you revert the change to this line? Looks like this added trailing whitespace.
|
||
```sql | ||
CREATE TABLE prod.db.sample | ||
USING iceberg | ||
PARTITIONED BY (part) | ||
TBLPROPERTIES ('key'='value') |
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.
Documentation should start with simple examples and add more complexity if needed.
In this case, I think it is valid to add a PARTITIONED BY
example, but not by modifying the first simple example. Please add the new paragraph and a separate example after the existing introduction to CTAS.
623374e
to
ef03f06
Compare
… improve as suggestion from apache#4606 (comment)
506bb57
to
ef03f06
Compare
…ove as suggestion from apache#4606 (comment)
@rdblue thx for the guide, and here is what it looks like now |
@@ -98,6 +98,15 @@ CREATE TABLE prod.db.sample | |||
USING iceberg | |||
AS SELECT ... | |||
``` | |||
```sql |
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.
Can you move the paragraph below up here so it's before the sql snippet? Then I think this is good to merge 👍
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.
@samredai sure, here is what it looks like now
…roved as suggestion from apache#4606 (comment)
LGTM |
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.
LGTM!
@kyle-cx would you mind rebasing this so we can merge it in? The |
Add an example of CTAS with PARTITIONED BY, make it clear that we can use PARTITIONED BY in CTAS. @samredai
fix #3854