-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Core: Introduce AssertViewUUID #8831
Conversation
b5a2e6e
to
c7f0dfb
Compare
c7f0dfb
to
1bbf5f3
Compare
1bbf5f3
to
f343516
Compare
@Override | ||
public void validate(TableMetadata base) { | ||
throw new ValidationException( | ||
"Cannot validate %s against a table", this.getClass().getSimpleName()); |
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.
Isn't this in the base class?
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 not. We only throw for ViewMetadata
but not for TableMetadata
in https://github.com/apache/iceberg/pull/8831/files#diff-3594848799883f9d669511640aadba2b625ce3e07b93a2ae211fcc2bb999e0aaR28, but we could change that. Let me know if you'd like me to make that change
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 went ahead and moved this to the base class, so that we're in line with MetadataUpdate
:
iceberg/core/src/main/java/org/apache/iceberg/MetadataUpdate.java
Lines 31 to 39 in d9eb937
default void applyTo(TableMetadata.Builder metadataBuilder) { | |
throw new UnsupportedOperationException( | |
String.format("Cannot apply update %s to a table", this.getClass().getSimpleName())); | |
} | |
default void applyTo(ViewMetadata.Builder viewMetadataBuilder) { | |
throw new UnsupportedOperationException( | |
String.format("Cannot apply update %s to a view", this.getClass().getSimpleName())); | |
} |
@@ -2510,6 +2510,32 @@ components: | |||
default-sort-order-id: | |||
type: integer | |||
|
|||
ViewRequirement: |
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.
Why is this separate? I thought that we were just going to add assert-view-uuid to the other. I don't like having separate View and Table requirements since we use the same ones in most places.
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 followed up with @nastra offline. Since the only requirement for views is assert-view-uuid and we don't want to change or reuse assert-table-uuid, it does make sense to have a separate set of requirements.
+1 when tests are passing. Thanks for making the refactor to move methods into the base class. |
No description provided.