API: Add name to Table and Catalog APIs #1537
Conversation
| * | ||
| * @return this catalog's name | ||
| */ | ||
| default String name() { |
There was a problem hiding this comment.
I did not intend to add the name to the Catalog API but we need this for proper metadata table names.
| * | ||
| * @return this table's name | ||
| */ | ||
| default String name() { |
There was a problem hiding this comment.
It is debatable whether we have to default this. I did this to avoid breaking custom implementations.
| @@ -59,10 +59,10 @@ public static Table createMetadataTableInstance(TableOperations originTableOps, | |||
| } | |||
|
|
|||
| public static Table createMetadataTableInstance(TableOperations originTableOps, | |||
| String catalogName, | |||
There was a problem hiding this comment.
This was broken before as the name of the metadata table started with its type, not catalog.
| String location = tableDir.toURI().toString(); | ||
| TABLES.create(SCHEMA, spec, location); | ||
|
|
||
| Table table = TABLES.load(location); |
There was a problem hiding this comment.
Metadata tables loaded through HadoopTables will have their names as location.type (which is weird as we normally use location#type). I am not sure whether it is a big deal or not.
There was a problem hiding this comment.
I think ideally you would be able to use the result of name to load the table. I'd prefer to fix it if we can. Maybe we should pass the full metadata table name in, rather than having it added by the table implementation.
There was a problem hiding this comment.
I fixed it but it did require substantially more changes.
| String name = identifier.name(); | ||
| MetadataTableType type = MetadataTableType.from(name); | ||
| String tableName = identifier.name(); | ||
| MetadataTableType type = MetadataTableType.from(tableName); |
There was a problem hiding this comment.
We use name() a few lines below now. I think it will be confusing to have name() that is the catalog name and name that is the metadata table name in the same method.
There was a problem hiding this comment.
Makes sense, but if this is the metadata table name (e.g. "files") instead of the table name, I think it might make more sense to be specific: metadataName?
There was a problem hiding this comment.
I feel like the current way is a bit more consistent with the overall structure of the method: the method name is loadMetadataTable and we use baseTableIdentifier to refer to the base table.
|
Thanks for the review, @rdblue. I went ahead and updated minor comments as well. |
This PR adds
namemethods to theTableandCatalogAPIs.Resolves #658.