nimesh1601 commented on code in PR #17847:
URL: https://github.com/apache/iceberg/pull/17847#discussion_r3885932194
##########
core/src/main/java/org/apache/iceberg/MetadataTableType.java:
##########
@@ -45,4 +46,16 @@ public static MetadataTableType from(String name) {
return null;
}
}
+
+ public static MetadataTableType from(TableIdentifier identifier) {
+ // An identifier only refers to a metadata table when its base table (the
identifier without
+ // its last part) still has a namespace, which requires at least two
namespace levels here.
+ // This lets a regular table reuse a metadata table name (e.g. "db.files")
without being
+ // mistaken for the "files" metadata table of namespace "db".
+ if (identifier.namespace().levels().length >= 2) {
Review Comment:
Thanks @gaborkaszab for reviewing this !. You're right that this isn't a
fully bulletproof resolution, and I don't think any purely syntactic rule can
be: a.b.files is inherently ambiguous between (1) a regular table files in
namespace a.b and (2) the files metadata table of base table a.b .
But multi-level regular tables with a metadata name can be created. The >=
2 check in MetadataTableType.from(TableIdentifier) is only consulted as a
fallback inside loadTable, after the real-table lookup:
```
TableOperations ops = newTableOps(identifier);
if (ops.current() == null) {
// only now fall back to metadata interpretation
if (isValidMetadataIdentifier(identifier)) { ... }
else { throw new NoSuchTableException(...); }
} else {
result = new BaseTable(...); // an existing real table always wins
}
```
So the metadata classification never gates creation and never shadows an
existing real table. Verified on InMemory/JDBC/REST/Hadoop (covered by the new
CatalogTests.tableSharingMetadataTableName , which runs a single-level ns and a
nested ns1.ns2 ):
- Create + load a.b.files (2-level namespace) -> loads as BaseTable , not
a metadata table.
- Its own metadata is still reachable as a.b.files.files -> FilesTable .
What the >= 2 threshold specifically fixes is the #10550 report —
db.files , where the "base table" would be db at the root/empty namespace.
Defaulting that to a regular table is safe because catalogs don't allow
root-level (empty-namespace) tables, so there's no real base table to shadow.
The one case that genuinely can't be disambiguated by identifier alone is
when a real table a.b and a real table a.b.files both exist:
loadTable(a.b.files) -> BaseTable (regular table; a.b's files-metadata is
shadowed)
loadTable(a.b.files.files) -> FilesTable (the regular table's own metadata)
This is inherent to overloading for both nesting and metadata addressing,
and matches the direction of the earlier PRs (#11963, #13223). A truly
bulletproof fix would need a dedicated, unambiguous separator for metadata
tables (like Spark's $ / # ) instead of . , which is a much larger
cross-engine design change and out of scope for this bug fix. Let me know if
this make sense
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
