Core: Fix Metadata table's UUID #9310
Conversation
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Thanks for fixing this @ajantha-bhat just a nit
| .as("UUID should be consistent on multiple calls") | ||
| .isEqualTo(manifestsTable.uuid()); | ||
| Assertions.assertThat(manifestsTable.uuid()) | ||
| .as("Metadata table UUID should be different from main table UUID") |
There was a problem hiding this comment.
Nit: "should be different from the base table's UUID"
|
|
||
| @Override | ||
| public UUID uuid() { | ||
| return UUID.randomUUID(); |
There was a problem hiding this comment.
Wow, I can't believe I wrote it like this originally :). Good fix, yes we do want a random UUID but it should be consistent across the same Metadata Table.
The only thing is if we extend this logic further is that the same base metadata table across different references won't return the same UUID. So for example "all manifests" for t1 may have a different UUID than "all manifests" for t1 in a different query.
I don't really see a way around that without persisting the UUID somewhere (like for normal tables that's in the metadata) but considering metadata tables are really more logical tables, I think that's OK. If we wanted that strict definition for metadata tables, then we would have to throw an unsupported until we persisted those details somewhere.
For now though, I think this change is good as is. It looks like in the next change you have the UUID is used as part of caching, in which case this fix should solve that.
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
Outdated
Show resolved
Hide resolved
|
@ajantha-bhat the same unrelated Flink flaky test is occurring that #9309 is fixing. I'm going to close and re-open this PR to retrigger CI |
2 participants
BaseFileRewriteCoordinator,ScanTaskSetManagercallstable.uuid()of metadata table multiple times for setting and reading from cache. So, uuid has to be consistent on each call for a given table.This issue was discovered during #9298