-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Make view metadata path configurable by write.metadata.path
#12017
Conversation
@nastra I believe you implemented the relevant part. If possible, can I ask you to review this change? |
docs/docs/view-configuration.md
Outdated
@@ -28,6 +28,7 @@ Iceberg views support properties to configure view behavior. Below is an overvie | |||
| Property | Default | Description | | |||
|--------------------------------------------|---------|------------------------------------------------------------------------------------| | |||
| write.metadata.compression-codec | gzip | Metadata compression codec: `none` or `gzip` | | |||
| write.metadata.path | table location + /metadata | Base location for metadata files | |
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.
nit: could you align the formatting of the table please?
assertThat(view).isNotNull(); | ||
assertThat(catalog().viewExists(identifier)).as("View should exist").isTrue(); | ||
assertThat(view.properties()).containsEntry("write.metadata.path", customLocation); | ||
assertThat(((BaseView) view).operations().current().metadataFileLocation()).isNotNull(); |
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.
maybe also add a .startsWith(customLocation)
check at the end
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.
Sure, thank you!
.withDefaultNamespace(identifier.namespace()) | ||
.withDefaultCatalog(catalog().name()) | ||
.withQuery("spark", "select * from ns.tbl") | ||
.withProperty("write.metadata.path", customLocation) |
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 use the statically defined property from ViewProperties
here
@@ -239,6 +239,36 @@ public void completeCreateView() { | |||
assertThat(catalog().viewExists(identifier)).as("View should not exist").isFalse(); | |||
} | |||
|
|||
@Test | |||
public void createViewWithCustomMetadataLocation() { |
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 also please add a test to TestViews
with a custom metadata location. You should be able to do a DESCRIBE
in that test and verify that the metadata locations match
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.
Thank you! Added the test in the TestViews.
Thanks for the review! I'm working on it, and will fix them. |
@nastra @amogh-jahagirdar Reflected the review by adding tests. Could you review the new changes? |
docs/docs/view-configuration.md
Outdated
| Property | Default | Description | | ||
|----------------------------------|----------------------------|------------------------------------------------------------------------------------| | ||
| write.metadata.compression-codec | gzip | Metadata compression codec: `none` or `gzip` | | ||
| write.metadata.path | table location + /metadata | Base location for metadata files | |
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.
| write.metadata.path | table location + /metadata | Base location for metadata files | | |
| write.metadata.path | view location + /metadata | Base location for metadata files | |
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.
Oh I missed this, let me fix now. Thank you!
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 with one remaining comment, thanks @tomtongue. I'll also leave this open for a bit so that @amogh-jahagirdar has a chance to review this too
Sure. Thanks so much for the review! |
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 @tomtongue!
Thanks so much for the quick review! |
Overview
This commit makes the View's metadata file location configurable by
write.metadata.path
as the table configuration, it and provides more flexible way to configure the metadata file path for Iceberg Views.Background
Currently, View's metadata file location respects its relevant database location first. If the database doesn't have the location, the View's metadata location is set to a warehouse location.
For now, when you customize the View's metadata location, you need to set the
warehouse
parameter. However, if your database has the configured location in its property such aslocation: /db/table/path
, you need to change the database location (this is not always easy).Example