-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Static Table Operations #1342
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
Static Table Operations #1342
Conversation
Allows for a Table Operations which references a specfic metadata version file. This operation will not change even if the base table it was derived from is changed. This enables it to act like a ReadOnly view of the table's state at a given time.
|
@rdblue + @aokolnychyi As we were talking about, a StaticTableOperations class for implementing #1344 |
| */ | ||
| @Override | ||
| public Table load(String location) { | ||
| TableOperations ops = newTableOps(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.
An alternative to this implementation that I considered was modifying findTable itself. The problem with this is that to do that implementation we need to extract the loadMetadataTable from below. I think this is a cleaner solution but we could do something where we make a StaticTables.java which findTables uses to either return a basetable or metadataTable
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 like this. It is more straightforward than the previous implementation.
core/src/main/java/org/apache/iceberg/StaticTableOperations.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/iceberg/StaticTableOperations.java
Outdated
Show resolved
Hide resolved
| @Test | ||
| public void testMetadataTables() { | ||
| for (MetadataTableType type: MetadataTableType.values()) { | ||
| String enumName = type.name().replace("_","").toLowerCase(); |
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.
When is this needed?
Also, style nit: no space between arguments to replace.
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.
ALL_DATA_FILES, ALL_MANIFESTS, ALL_ENTRIES
All the style things just make me want to upgrade the Checkstyle version even more :) I forgot to run in Intellij where I can force the newer version
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 thought those were loaded using all_data_files, all_manifests, etc. and not alldatafiles.
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.
we are just comparing the ClassName here, so we are comparing AllDataFilesTable.class with ALL_DATA_FILES
I may be misunderstanding your comment here, I'm just making sure that we get back the Class with the right name based on the MetadataType we request.
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.
Okay, I see. Nevermind, then.
core/src/test/java/org/apache/iceberg/hadoop/TestStaticTable.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test(expected = UnsupportedOperationException.class) | ||
| public void testCannotBeAddedTo(){ |
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'm not sure we need both this and the next test, but it's okay to have them.
| public Table load(String location) { | ||
| TableOperations ops = newTableOps(location); | ||
| if (ops.current() == null) { | ||
| //Possibly Load a Metadata Table |
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 about parse out the metadata table name, if present?
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 made a different implementation here where we will instead do
Name, Type = tryParseMetadataLocation
if (name, type).{
loadMetadata
} else {
loadBasic
}
core/src/main/java/org/apache/iceberg/StaticTableOperations.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/iceberg/StaticTableOperations.java
Outdated
Show resolved
Hide resolved
|
Thanks @RussellSpitzer! Overall this is the right approach and I think the implementation looks good. Just a few minor things. |
Cleanup various style mistakes Slightly redo logic around parsing MetadataTableNames Fix test cases which were missing refreshes
|
Updated |
|
@rdblue just a friendly ping, once this gets in I"ll redo the ExpireSnapshots branch, that should be very straightforward |
|
Looks great! Thank you @RussellSpitzer! |
|
Thanks! |
…1342) Allows for a Table Operations which references a specfic metadata version file. This operation will not change even if the base table it was derived from is changed. This enables it to act like a ReadOnly view of the table's state at a given time.
Allows for a Table Operations which references a specfic metadata version
file. This operation will not change even if the base table it was derived
from is changed. This enables it to act like a ReadOnly view of the table's
state at a given time.