szehon-ho commented on code in PR #9735:
URL: https://github.com/apache/iceberg/pull/9735#discussion_r1492792745
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -388,6 +388,12 @@ public Transaction newTransaction() {
throw new UnsupportedOperationException(errorMsg("newTransaction"));
}
+ @Override
+ public StaticTableOperations operations() {
+ // Never return an operations that is not static
Review Comment:
I would avoid double negative here, 'always return static table operations`.
But also, I am not sure this comment serves much that the code doesnt show,
should we just remove it?
##########
core/src/test/java/org/apache/iceberg/hadoop/TestTableSerialization.java:
##########
@@ -153,6 +163,22 @@ public void testSerializableMetadataTablesPlanning()
throws IOException {
}
}
+ @Test
+ void testMetadataTableFromSerializedTable() {
Review Comment:
Public, to match the other tests?
##########
core/src/main/java/org/apache/iceberg/MetadataTableUtils.java:
##########
@@ -32,9 +32,16 @@ public static boolean hasMetadataTableName(TableIdentifier
identifier) {
public static Table createMetadataTableInstance(Table table,
MetadataTableType type) {
if (table instanceof BaseTable) {
return createMetadataTableInstance(table,
metadataTableName(table.name(), type), type);
+ } else if (table instanceof HasTableOperations) {
+ return createMetadataTableInstance(
+ ((HasTableOperations) table).operations(),
+ table.name(),
+ metadataTableName(table.name(), type),
+ type);
} else {
throw new IllegalArgumentException(
- String.format("Cannot create metadata table for table %s: not a base
table", table));
+ String.format(
Review Comment:
'table is not a base table or does not have table operations'?
##########
core/src/main/java/org/apache/iceberg/MetadataTableUtils.java:
##########
@@ -32,9 +32,16 @@ public static boolean hasMetadataTableName(TableIdentifier
identifier) {
public static Table createMetadataTableInstance(Table table,
MetadataTableType type) {
if (table instanceof BaseTable) {
return createMetadataTableInstance(table,
metadataTableName(table.name(), type), type);
+ } else if (table instanceof HasTableOperations) {
Review Comment:
Another option is to make the check for SerializableTable only here to make
impact/risk smaller.
But I suppose it's pretty unlikely anyone is using this API on another
version of HasTableOperations and expecting a failure.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
