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.
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.
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.
When is this needed?
Also, style nit: no space between arguments to replace.
There was a problem hiding this comment.
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.
I thought those were loaded using all_data_files, all_manifests, etc. and not alldatafiles.
There was a problem hiding this comment.
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.
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.
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.
How about parse out the metadata table name, if present?
There was a problem hiding this comment.
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.
2 participants
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.