-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Core,Api: Add overwrite option when register external table to catalog #12228
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
Conversation
|
Java CI Failure is timing out on concurrent fast append and seems unrelated to the change. @rdblue @RussellSpitzer @danielcweeks do you want to take a look? |
gaborkaszab
left a comment
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 for working on this, @dramaticlly !
aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java
Outdated
Show resolved
Hide resolved
aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java
Outdated
Show resolved
Hide resolved
aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java
Outdated
Show resolved
Hide resolved
9f48d2e to
259ed96
Compare
|
|
||
| @Test | ||
| public void testRegisterAndOverwriteExistingTable() { | ||
| C catalog = 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.
Are we just adding a bucket to test the change? Why not just use the table UUID? I feel like we should be able to just
Make Table 1
Make Table 2
Register overwrite Table1 with Table2
Check that metadata table1 matches table 2?
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.
Initially I think register with overwrite helps revert an existing table to a new previous health state. If we want to support overwrite with another tables's metadata, It seems better suited with drop + register, to reflect the table UUID change.
From the table spec, it asks Implementations to throw an exception if a table's UUID does not match the expected UUID when refreshing metadata. What do you think?
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 added a similar question in the code where it check the UUID doesn't change for register with overwrite. Because API is called registerTable, I would think think UUID change should be allowed. Interesting to hear other people's takes on this one.
Initially I think register with overwrite helps revert an existing table to a new previous health state.
for this initial use case, agree that UUID shouldn't change. But if we want to only solve this specific/narrower problem, maybe a narrower API would make more sense. Enforcing UUID check for this narrow API is totally the right thing to do.
resetTable(TableIdentifier identifier, String metadataFileLocation)
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.
Sharing my thoughts here, so I believe table UUID is the ideal way for consumer to identify the uniqueness of a given table, instead of relying on the table identifier in the given catalog.
Today, the table operation on refresh will check if underlying the UUID has changed, also REST catalog will have requirements on table UUID unchanged for replace and update of the table. If we want to support register overwrite with foreign table, then it secretly break the assumption, implies catalog need to evict the cached table (even with same table identifier) and force to reload.
Personally I think it's probably better to only support same table UUID on register with overwrite (which provides atomicity), and support table UUID change with drop table first and then reregister.
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.
After some thoughts, I changed behavior from reuse existing commit logic which pass the ops.current, to drop the table first and then register with given metadata. This relax the constraints on table UUID check, also ensure that latest metadataFileLocation in TableMetadata after overwrite is the same as user provided.
I also added a comment in interface to highlight the potential table UUID 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.
let me unresolved this comment. wondering if it is desirable to have two different tables share the UUID.
Make Table 1
Make Table 2
Register overwrite Table1 with Table2
E.g., the MV spec PR currently defines storage table refresh-state with only UUID as table/view identifier.
Interesting to hear more inputs.
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java
Outdated
Show resolved
Hide resolved
dell/src/test/java/org/apache/iceberg/dell/ecs/TestEcsCatalog.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
Outdated
Show resolved
Hide resolved
f0e6889 to
f7aa204
Compare
aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/iceberg/rest/requests/RegisterTableRequest.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/iceberg/rest/requests/RegisterTableRequestParser.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Test | ||
| public void testRegisterAndOverwriteExistingTable() { | ||
| C catalog = 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.
I added a similar question in the code where it check the UUID doesn't change for register with overwrite. Because API is called registerTable, I would think think UUID change should be allowed. Interesting to hear other people's takes on this one.
Initially I think register with overwrite helps revert an existing table to a new previous health state.
for this initial use case, agree that UUID shouldn't change. But if we want to only solve this specific/narrower problem, maybe a narrower API would make more sense. Enforcing UUID check for this narrow API is totally the right thing to do.
resetTable(TableIdentifier identifier, String metadataFileLocation)
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java
Outdated
Show resolved
Hide resolved
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java
Outdated
Show resolved
Hide resolved
| "The requested metadata matches the existing metadata. No changes will be committed."); | ||
| return new BaseTable(ops, fullTableName(name(), identifier), metricsReporter()); | ||
| } | ||
| dropTable(identifier, false /* Keep all data and 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.
drop the table first and then register with given metadata
what if the job failed btw these two steps? we can end up with table deleted (but new metadata not registered), which is also not ideal.
Thinking about it again. Enforcing UUID match for overwrite seems reasonable.
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.
Additional notes of switching from TableOperations.commit(ops.current, newTableMetadata) to drop and then re-register:
- This ensure the same input metadata.json is used to commit and result as latest TableMetadata.current.metadataLocation like in Core: Avoid creating new metadata file when
registerTableAPI is used #6591. Existing doCommit logicwill only reuse the metadata.json instead of writing a new one if the commit is for creating a table. It would be difficult to differentiate the register-with-force and normal commit iniceberg/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java
Lines 147 to 151 in c16cefa
protected String writeNewMetadataIfRequired(boolean newTable, TableMetadata metadata) { return newTable && metadata.metadataFileLocation() != null ? metadata.metadataFileLocation() : writeNewMetadata(metadata, currentVersion() + 1); } doCommitmethod without change its interface for all TableOperations - This also relax the constraint on matching the table UUID between existing table and new metadata to be overwritten
- Register-with-force in generally shall be user facing retrievable as end state is having the input metadata as the latest state of the table. Where ops.current() are subject to change if running with a race condition and require retry within method
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.
This ensure the same input metadata.json is used to commit
Register-with-force in generally shall be retrievable as end state is having the input metadata as the latest state of the table. Where ops.current() are subject to change if running with a race condition
I understand this may look intuitive for registerTable API. But what overwrite is essentially update the state of an existing table. Hence, the current behavior of writeNewMetadataIfRequired is fine to me. It would create and commit a new metadata file with the same content as the input file.
Let's assume current metadata file is meta-009.json. It is reasonable to me that registerTable("meta-005.json", true) would commit a new file meta-010.json with the same content as meta-005.json.
The real problem might be piggyback overwrite with registerTable. If it is a separate overwriteTable/resetTable API, then it won't be confusing that the commit metadata file is a new file with the same content as the input metadata file.
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.
This also relax the constraint on matching the table UUID between existing table and new metadata to be overwritten
This is where I have a second thought from my earlier stance. overwrite should enforce UUID match check. See my other comment.
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 I have a disagreement here. I think the behavior should be identical to the registerTable without overwrite. The file that is being passed is the file that should be used for registration. For example, if i'm updating an table to match an existing one and I rely on the metadata.json path to tell what files are copied (like in dr) having a different file name could be really make things difficult.
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 @guykhazma , I don't think we have any clear semantics expectation for register-table with overwrite in REST API to complete atomically. Table specification states that
Table state is maintained in metadata files. All changes to table state create a new metadata file and replace the old metadata with an atomic swap.
IMO, overwrite of table metadata is not a valid state change but rather a state overwrite, where state can even come from another table with a different table UUID. I had some offline discussion with @RussellSpitzer on this where we do agree on this can be catalog implementation specific and open the room for atomic swap if catalog can support this.
As for your proposed alternative approach, i think we can write multiple metadata.json on file system first and rely on catalog for atomic swap, but we might hit the same limitation in TableOperations API, where new metadata.json will be rewritten with a different file name as input and difficult to verify.
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 @dramaticlly, Could you elaborate on why you see this as a state overwrite?
I can imagine a scenario where some state or partial state is transferred from another table (with a different UUID), which might be interpreted as a state change. However, I’m not sure the register API is the appropriate mechanism for that.
From my understanding, the purpose of the register API is to create a named reference to a metadata JSON. It doesn’t inherently imply any change to the actual state of a table. Even if you register it against an existing table and the resulting metadata reflects a different state, it doesn’t mean that the underlying storage state has changed.
For instance, it's possible to register the same table multiple times under different names using distinct metadata files—effectively simulating branching using different entries in the 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.
Sorry for the delayed response. Let's say original table with identifier mytable and state A is represented by A.metadata.json and now we are registering with B.metadata.json. There's no guarantee that A and B has anything in common.
Probably because we are looking from different angles, While the underlying storage state may remain unchanged during a register-table operation, the perception of the table can shift significantly. From a data consumer’s standpoint, if the identifier mytable now references a different collection of data due to registration with the overwrite flag, its internal state—including metadata, schema, partitioning, and data—may have changed entirely.
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.
@dramaticlly I see your point. It seems to me the core question here is whether the responsibility for maintaining the lineage of a table identifier lies with the catalog or the table itself. From my perspective, it makes more sense for the catalog to handle this, especially since the overwrite operation doesn't alter the physical state of the table. Ideally, this reference change should be atomic, but the implementation details can be left to individual catalogs.
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 it makes sense to move this into an explicit Table operation api. We essentially want something that's like
ops.setMetadata(newMetadata)
Which ignores validations and transactionally swaps. May be cleaner than doing the drop/create we are currently doing. This is essentially what any rest catalog could do and would fix @stevenzwu 's issues with atomicity by letting each catalog implementation decide whether to make it atomic or not.
core/src/main/java/org/apache/iceberg/rest/requests/RegisterTableRequest.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Test | ||
| public void testRegisterAndOverwriteExistingTable() { | ||
| C catalog = 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.
let me unresolved this comment. wondering if it is desirable to have two different tables share the UUID.
Make Table 1
Make Table 2
Register overwrite Table1 with Table2
E.g., the MV spec PR currently defines storage table refresh-state with only UUID as table/view identifier.
Interesting to hear more inputs.
2aeeae4 to
118061b
Compare
| */ | ||
| default Table registerTable( | ||
| TableIdentifier identifier, String metadataFileLocation, boolean overwrite) { | ||
| throw new UnsupportedOperationException("Registering tables is not supported"); |
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.
Not sure if this is worth while, but you could decide only to fail if "overwrite" is true"
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 @RussellSpitzer , in this PR I introduced a new register-overwrite API on catalog interface, and changed as new base for register-table (where it can call the new API with overwrite=false).
Before:
default register-table API throw UnsupportedOperationException
After:
default register-table API -> register-overwrite(overwrite=true)
default register-overwrite API throw UnsupportedOperationException
The benefits are all concrete catalog implementations can just implement the new API and interface is only used for delegation between the 2 APIs. This is an easier to reason (as all register logic sits in one place) and follows the convention like drop-table and drop-table-purge.
The potential downside is that some custom catalog implementations outside iceberg repo who implements the register-table API, might need to update their code when upgrades iceberg dependency with the interface change. I feel like it's generally justified for customized catalog to keep up with iceberg interface change. Please let me know if you think otherwise.
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 you are right here, let's just keep it this way (clean it up in 2.0)
| // register table t1 with metadata from table t2 | ||
| Table registered = catalog.registerTable(identT1, opsT2.current().metadataFileLocation(), true); | ||
|
|
||
| assertThat(registered.uuid()) |
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 can just check that it matches t2 uuid rather than checking it doesn't match t1.
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.
In fact why aren't we checking if the T2 object matches the "registered" table? Shouldn't they just be completely identical?
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.
yeah I think the assertions below on line 3210 basically is checking for complete identification.
| .isTrue(); | ||
| assertThat(operation(registered).refresh()) | ||
| .usingRecursiveComparison() | ||
| // Nessie catalog holds different Nessie commit-ID from which the metadata has been loaded. |
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 don't think we should have nessie specific issues in the core module
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.
yeah I agree, but since CatalogTests is abstract and existing TestNessieCatalog implements this require exclusion on some of the table properties. Do you think I shall add a assumeTrue to exclude Nessies from this test instead?
| } | ||
|
|
||
| @Test | ||
| public void testRegisterAndOverwriteExistingTable() throws IOException { |
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.
This probably shouldn't be supported at all for Hadoop Catalog? Won't this only work if you are replacing the Highest-metadata.json with a Higher-metadata.json?
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.
yeah I think this is a good point as Hadoop table operations always look for highest version file in metadata root.
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
dev list email sent, consider the suggestion from Ryan to implement set given metadata.json as current in catalog directly, without the need of new API in TableOperations interface |
ee5f4db to
3c50007
Compare
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
not stale |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
not stale |
api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java
Outdated
Show resolved
Hide resolved
| ops.commit(null, metadata); | ||
| TableMetadata existing = ops.current(); | ||
|
|
||
| if (tableExists(identifier)) { |
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 there is a bit of a lock issue here, ie between line 84 and 91 we may have created the table. I'm not sure this is actually a problem since this is a pretty destructive operation anyway. This implementation though is mixing some logic here
IIMHO I think this should be completely replaced with a catalog specific impl
Something like
if (overwrite) {
CatalogSpecific registerTable logic
} else if (table doesn't exist) {
catalog specific lockTable
Catalog specific registerTable logic
catalog specific removeLock
} else {
// We can't overwrite and the table already exists
throw table already exists exception
}
Here we drop the "operations" path entirely
I think we should probably remove any
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.
Sounds good, updated the control flow as suggested and also move all overwrite logic into a private method
| } | ||
|
|
||
| // Atomically set given metadata as current | ||
| protected void setAsCurrent( |
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 it's super clear how this works from the signature. IMHO, an implementation of this shouldn't care what "base" is. It should either be "setAsCurrent if empty or setAsCurrentRegardless"
In my above snippet maybe something like
if (overwrite)
setAsCurrent(Identifier, Location)
} else {
setAsCurrentIfEmpty(identifier, 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.
Removed base from setAsCurrent. Originally I added this to use for lock-free version of AlterTable in HiveCatalog as base is not used in JDBC or InMemory catalog at all. I think it's better to keep the interface clean and general to all catalogs instead of considering implementation as the constraint
69178c2 to
7e3c09a
Compare
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
not stale |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
This PR adds a new register-table with overwrite option on Catalog interface to allow overwrite table metadata of an existing Iceberg table. The overwrite is atomic in nature and optional for catalog who choose to implement. This PR support the behavior for
Relate to #12134 and openAPI REST spec change can be found in #12239. To limit the scope of this PR, handle of atomic register for new table will be in a follow up change