You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "singhpk234 (via GitHub)" <gi...@apache.org> on 2025/12/30 01:31:59 UTC
[PR] REST: Reuse table metadata as part of LoadTable in serializable table [iceberg]
singhpk234 opened a new pull request, #14944:
URL: https://github.com/apache/iceberg/pull/14944
### About the change
This change attempts to avoid object store call to recreate the TableMetadata from the object store in case we are making a serialized table copy of the table, this could be seen as an optimization where rather than going to the obj store we keep the serialized copy of the table and then just store it and send it via wire for example (spark driver sending to executor) or can also be looked as a feature where if the catalog has redacted some sensitive info, all the serializable table respects it and reads that rather than going to the object store (which it may not have access to).
Presently its gauraded only for RESTTable and for the
I understand that spec wording the following:
Per REST spec the metadata location is nullable in certain cases
https://github.com/apache/iceberg/blob/4632f3133efbf6b8232ed9526de7142fd50e1aa6/open-api/rest-catalog-open-api.yaml#L3396-L3399
The spec says the following but that is **SHOULD** and not **MUST** :
https://github.com/apache/iceberg/blob/4632f3133efbf6b8232ed9526de7142fd50e1aa6/open-api/rest-catalog-open-api.yaml#L3365
just wanted to float this to folks on what they think of this feature.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST: Reuse table metadata as part of LoadTable in serializable table [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14944:
URL: https://github.com/apache/iceberg/pull/14944#discussion_r2660282665
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -197,7 +197,7 @@ public String toString() {
}
}
- public static class MetadataLogEntry {
+ public static class MetadataLogEntry implements Serializable {
Review Comment:
we should fix this irrespective, IMHO
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST: Reuse table metadata as part of LoadTable in serializable table [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14944:
URL: https://github.com/apache/iceberg/pull/14944#discussion_r2668118345
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -3528,4 +3530,201 @@ private static List<HTTPRequest> allRequests(RESTCatalogAdapter adapter) {
verify(adapter, atLeastOnce()).execute(captor.capture(), any(), any(), any());
return captor.getAllValues();
}
+
+ @Test
+ public void testSerializableTableWithRESTCatalog() throws IOException, ClassNotFoundException {
+ // Create a table with data through REST catalog
+ Schema schema =
+ new Schema(
+ required(1, "id", Types.IntegerType.get()),
+ required(2, "data", Types.StringType.get()));
+
+ TableIdentifier ident = TableIdentifier.of(Namespace.of("ns"), "table");
+ restCatalog.createNamespace(ident.namespace());
+ Table table = restCatalog.createTable(ident, schema);
+
+ // Add data files to create snapshots
+ table.newAppend().appendFile(FILE_A).commit();
+ table.newAppend().appendFile(FILE_B).commit();
+
+ // Create SerializableTable from REST catalog
+ SerializableTable serializableTable = (SerializableTable) SerializableTable.copyOf(table);
+
+ // Test with Java serialization
+ Table javaSerialized = TestHelpers.roundTripSerialize(serializableTable);
+ verifySerializedTable(javaSerialized, table, 2);
+
+ // Test with Kryo serialization
+ Table kryoSerialized = TestHelpers.KryoHelpers.roundTripSerialize(serializableTable);
+ verifySerializedTable(kryoSerialized, table, 2);
+ }
+
+ private void verifySerializedTable(
+ Table deserialized, Table original, int expectedSnapshotCount) {
+ // Verify that basic operations work using serialized table metadata
+ // (tables requiring remote scan planning serialize full metadata)
+ assertThat(deserialized.schema().asStruct()).isEqualTo(original.schema().asStruct());
+ assertThat(deserialized.spec().specId()).isEqualTo(original.spec().specId());
+ assertThat(deserialized.location()).isEqualTo(original.location());
+ assertThat(deserialized.properties()).containsAllEntriesOf(original.properties());
+
+ // Verify snapshot operations work using serialized metadata
+ assertThat(deserialized.currentSnapshot()).isNotNull();
+ assertThat(deserialized.currentSnapshot().snapshotId())
+ .isEqualTo(original.currentSnapshot().snapshotId());
+
+ // Verify snapshots are accessible
+ assertThat(deserialized.snapshots()).isNotNull();
+ int snapshotCount = 0;
+ for (Snapshot snapshot : deserialized.snapshots()) {
+ snapshotCount++;
+ assertThat(snapshot).isNotNull();
+ }
+ assertThat(snapshotCount).isEqualTo(expectedSnapshotCount);
+
+ // Verify scan operations work
+ assertThat(deserialized.newScan()).isNotNull();
+ }
+
+ @Test
+ public void testSerializableTableWithRESTCatalogAndSchemaEvolution()
+ throws IOException, ClassNotFoundException {
+ // Create initial table
+ Schema initialSchema =
+ new Schema(
+ required(1, "id", Types.IntegerType.get()),
+ required(2, "data", Types.StringType.get()));
+
+ TableIdentifier ident = TableIdentifier.of(Namespace.of("ns"), "schema_evolution_table");
+ restCatalog.createNamespace(ident.namespace());
+ Table table = restCatalog.createTable(ident, initialSchema);
+
+ // Evolve schema
+ table.updateSchema().addColumn("new_col1", Types.IntegerType.get()).commit();
+ table.newAppend().appendFile(FILE_A).commit();
+
+ // Evolve schema again
+ table.updateSchema().addColumn("new_col2", Types.StringType.get()).commit();
+ table.newAppend().appendFile(FILE_B).commit();
+
+ // Create and serialize table
+ SerializableTable serializableTable = (SerializableTable) SerializableTable.copyOf(table);
+
+ // Test with Java serialization
+ Table javaSerialized = TestHelpers.roundTripSerialize(serializableTable);
+ verifySchemaEvolution(javaSerialized, table);
+
+ // Test with Kryo serialization
+ Table kryoSerialized = TestHelpers.KryoHelpers.roundTripSerialize(serializableTable);
+ verifySchemaEvolution(kryoSerialized, table);
+ }
+
+ private void verifySchemaEvolution(Table deserialized, Table original) {
+ // Verify current schema
+ assertThat(deserialized.schema().columns()).hasSize(original.schema().columns().size());
+
+ // Verify all historical schemas are preserved
+ assertThat(deserialized.schemas()).isNotNull();
+ assertThat(deserialized.schemas().size()).isEqualTo(original.schemas().size());
Review Comment:
```suggestion
assertThat(deserialized.schemas()).isNotNull().hasSameSizeAs(original.schemas());
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST: Reuse table metadata as part of LoadTable in serializable table [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14944:
URL: https://github.com/apache/iceberg/pull/14944#discussion_r2668104914
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -3528,4 +3530,201 @@ private static List<HTTPRequest> allRequests(RESTCatalogAdapter adapter) {
verify(adapter, atLeastOnce()).execute(captor.capture(), any(), any(), any());
return captor.getAllValues();
}
+
+ @Test
+ public void testSerializableTableWithRESTCatalog() throws IOException, ClassNotFoundException {
+ // Create a table with data through REST catalog
+ Schema schema =
+ new Schema(
+ required(1, "id", Types.IntegerType.get()),
+ required(2, "data", Types.StringType.get()));
+
+ TableIdentifier ident = TableIdentifier.of(Namespace.of("ns"), "table");
Review Comment:
you should be able to use `NS` and `TABLE`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST: Reuse table metadata as part of LoadTable in serializable table [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 closed pull request #14944: REST: Reuse table metadata as part of LoadTable in serializable table
URL: https://github.com/apache/iceberg/pull/14944
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST: Reuse table metadata as part of LoadTable in serializable table [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14944:
URL: https://github.com/apache/iceberg/pull/14944#discussion_r2668101637
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -3528,4 +3530,201 @@ private static List<HTTPRequest> allRequests(RESTCatalogAdapter adapter) {
verify(adapter, atLeastOnce()).execute(captor.capture(), any(), any(), any());
return captor.getAllValues();
}
+
+ @Test
+ public void testSerializableTableWithRESTCatalog() throws IOException, ClassNotFoundException {
+ // Create a table with data through REST catalog
Review Comment:
when would this not be with a REST catalog? I think we can remove the comment and adjust the naming of the method, since we're always testing via REST catalog here
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST: Reuse table metadata as part of LoadTable in serializable table [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on PR #14944:
URL: https://github.com/apache/iceberg/pull/14944#issuecomment-3812676977
Closing this for as this only shows up in distributed planning and metadata table which is presently not supported by remote scan planning.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST: Reuse table metadata as part of LoadTable in serializable table [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14944:
URL: https://github.com/apache/iceberg/pull/14944#discussion_r2668109426
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -3528,4 +3530,201 @@ private static List<HTTPRequest> allRequests(RESTCatalogAdapter adapter) {
verify(adapter, atLeastOnce()).execute(captor.capture(), any(), any(), any());
return captor.getAllValues();
}
+
+ @Test
+ public void testSerializableTableWithRESTCatalog() throws IOException, ClassNotFoundException {
+ // Create a table with data through REST catalog
+ Schema schema =
+ new Schema(
+ required(1, "id", Types.IntegerType.get()),
+ required(2, "data", Types.StringType.get()));
+
+ TableIdentifier ident = TableIdentifier.of(Namespace.of("ns"), "table");
+ restCatalog.createNamespace(ident.namespace());
+ Table table = restCatalog.createTable(ident, schema);
+
+ // Add data files to create snapshots
+ table.newAppend().appendFile(FILE_A).commit();
+ table.newAppend().appendFile(FILE_B).commit();
+
+ // Create SerializableTable from REST catalog
+ SerializableTable serializableTable = (SerializableTable) SerializableTable.copyOf(table);
+
+ // Test with Java serialization
+ Table javaSerialized = TestHelpers.roundTripSerialize(serializableTable);
+ verifySerializedTable(javaSerialized, table, 2);
+
+ // Test with Kryo serialization
+ Table kryoSerialized = TestHelpers.KryoHelpers.roundTripSerialize(serializableTable);
+ verifySerializedTable(kryoSerialized, table, 2);
+ }
+
+ private void verifySerializedTable(
+ Table deserialized, Table original, int expectedSnapshotCount) {
+ // Verify that basic operations work using serialized table metadata
+ // (tables requiring remote scan planning serialize full metadata)
+ assertThat(deserialized.schema().asStruct()).isEqualTo(original.schema().asStruct());
+ assertThat(deserialized.spec().specId()).isEqualTo(original.spec().specId());
+ assertThat(deserialized.location()).isEqualTo(original.location());
+ assertThat(deserialized.properties()).containsAllEntriesOf(original.properties());
+
+ // Verify snapshot operations work using serialized metadata
+ assertThat(deserialized.currentSnapshot()).isNotNull();
+ assertThat(deserialized.currentSnapshot().snapshotId())
+ .isEqualTo(original.currentSnapshot().snapshotId());
+
+ // Verify snapshots are accessible
+ assertThat(deserialized.snapshots()).isNotNull();
Review Comment:
```suggestion
assertThat(deserialized.snapshots()).isNotNull().hasSize(expectedSnapshotCount);
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST: Reuse table metadata as part of LoadTable in serializable table [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14944:
URL: https://github.com/apache/iceberg/pull/14944#discussion_r2668096318
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -136,13 +144,20 @@ private Table lazyTable() {
if (lazyTable == null) {
synchronized (this) {
if (lazyTable == null) {
- if (metadataFileLocation == null) {
+ TableOperations ops;
+
+ if (tableMetadata != null) {
+ // Use serialized TableMetadata (tables requiring remote scan planning)
+ // This avoids storage access for distributed query planning
+ ops = new StaticTableOperations(tableMetadata, io, locationProvider());
+ } else if (metadataFileLocation != null) {
+ // Read from storage using metadata location
+ ops = new StaticTableOperations(metadataFileLocation, io, locationProvider());
+ } else {
throw new UnsupportedOperationException(
- "Cannot load metadata: metadata file location is null");
+ "Cannot load metadata: both tableMetadata and metadataFileLocation are null");
Review Comment:
minor: I don't think we want to mention actual field names here, so it's probably better to keep the error msg more generic by just referring to table metadata / metadata file location
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST: Reuse table metadata as part of LoadTable in serializable table [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14944:
URL: https://github.com/apache/iceberg/pull/14944#discussion_r2668116568
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -3528,4 +3530,201 @@ private static List<HTTPRequest> allRequests(RESTCatalogAdapter adapter) {
verify(adapter, atLeastOnce()).execute(captor.capture(), any(), any(), any());
return captor.getAllValues();
}
+
+ @Test
+ public void testSerializableTableWithRESTCatalog() throws IOException, ClassNotFoundException {
+ // Create a table with data through REST catalog
+ Schema schema =
+ new Schema(
+ required(1, "id", Types.IntegerType.get()),
+ required(2, "data", Types.StringType.get()));
+
+ TableIdentifier ident = TableIdentifier.of(Namespace.of("ns"), "table");
+ restCatalog.createNamespace(ident.namespace());
+ Table table = restCatalog.createTable(ident, schema);
+
+ // Add data files to create snapshots
+ table.newAppend().appendFile(FILE_A).commit();
+ table.newAppend().appendFile(FILE_B).commit();
+
+ // Create SerializableTable from REST catalog
+ SerializableTable serializableTable = (SerializableTable) SerializableTable.copyOf(table);
+
+ // Test with Java serialization
+ Table javaSerialized = TestHelpers.roundTripSerialize(serializableTable);
+ verifySerializedTable(javaSerialized, table, 2);
+
+ // Test with Kryo serialization
+ Table kryoSerialized = TestHelpers.KryoHelpers.roundTripSerialize(serializableTable);
+ verifySerializedTable(kryoSerialized, table, 2);
+ }
+
+ private void verifySerializedTable(
+ Table deserialized, Table original, int expectedSnapshotCount) {
+ // Verify that basic operations work using serialized table metadata
+ // (tables requiring remote scan planning serialize full metadata)
+ assertThat(deserialized.schema().asStruct()).isEqualTo(original.schema().asStruct());
+ assertThat(deserialized.spec().specId()).isEqualTo(original.spec().specId());
+ assertThat(deserialized.location()).isEqualTo(original.location());
+ assertThat(deserialized.properties()).containsAllEntriesOf(original.properties());
+
+ // Verify snapshot operations work using serialized metadata
+ assertThat(deserialized.currentSnapshot()).isNotNull();
+ assertThat(deserialized.currentSnapshot().snapshotId())
+ .isEqualTo(original.currentSnapshot().snapshotId());
+
+ // Verify snapshots are accessible
+ assertThat(deserialized.snapshots()).isNotNull();
+ int snapshotCount = 0;
+ for (Snapshot snapshot : deserialized.snapshots()) {
+ snapshotCount++;
+ assertThat(snapshot).isNotNull();
+ }
+ assertThat(snapshotCount).isEqualTo(expectedSnapshotCount);
+
+ // Verify scan operations work
+ assertThat(deserialized.newScan()).isNotNull();
+ }
+
+ @Test
+ public void testSerializableTableWithRESTCatalogAndSchemaEvolution()
+ throws IOException, ClassNotFoundException {
+ // Create initial table
+ Schema initialSchema =
+ new Schema(
+ required(1, "id", Types.IntegerType.get()),
+ required(2, "data", Types.StringType.get()));
+
+ TableIdentifier ident = TableIdentifier.of(Namespace.of("ns"), "schema_evolution_table");
+ restCatalog.createNamespace(ident.namespace());
+ Table table = restCatalog.createTable(ident, initialSchema);
+
+ // Evolve schema
+ table.updateSchema().addColumn("new_col1", Types.IntegerType.get()).commit();
+ table.newAppend().appendFile(FILE_A).commit();
+
+ // Evolve schema again
+ table.updateSchema().addColumn("new_col2", Types.StringType.get()).commit();
+ table.newAppend().appendFile(FILE_B).commit();
+
+ // Create and serialize table
+ SerializableTable serializableTable = (SerializableTable) SerializableTable.copyOf(table);
+
+ // Test with Java serialization
+ Table javaSerialized = TestHelpers.roundTripSerialize(serializableTable);
+ verifySchemaEvolution(javaSerialized, table);
+
+ // Test with Kryo serialization
+ Table kryoSerialized = TestHelpers.KryoHelpers.roundTripSerialize(serializableTable);
+ verifySchemaEvolution(kryoSerialized, table);
+ }
+
+ private void verifySchemaEvolution(Table deserialized, Table original) {
+ // Verify current schema
+ assertThat(deserialized.schema().columns()).hasSize(original.schema().columns().size());
Review Comment:
```suggestion
assertThat(deserialized.schema().columns()).hasSameSizeAs(original.schema().columns());
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST: Reuse table metadata as part of LoadTable in serializable table [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14944:
URL: https://github.com/apache/iceberg/pull/14944#discussion_r2668110587
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -3528,4 +3530,201 @@ private static List<HTTPRequest> allRequests(RESTCatalogAdapter adapter) {
verify(adapter, atLeastOnce()).execute(captor.capture(), any(), any(), any());
return captor.getAllValues();
}
+
+ @Test
+ public void testSerializableTableWithRESTCatalog() throws IOException, ClassNotFoundException {
+ // Create a table with data through REST catalog
+ Schema schema =
+ new Schema(
+ required(1, "id", Types.IntegerType.get()),
+ required(2, "data", Types.StringType.get()));
+
+ TableIdentifier ident = TableIdentifier.of(Namespace.of("ns"), "table");
+ restCatalog.createNamespace(ident.namespace());
+ Table table = restCatalog.createTable(ident, schema);
+
+ // Add data files to create snapshots
+ table.newAppend().appendFile(FILE_A).commit();
+ table.newAppend().appendFile(FILE_B).commit();
+
+ // Create SerializableTable from REST catalog
+ SerializableTable serializableTable = (SerializableTable) SerializableTable.copyOf(table);
+
+ // Test with Java serialization
+ Table javaSerialized = TestHelpers.roundTripSerialize(serializableTable);
+ verifySerializedTable(javaSerialized, table, 2);
+
+ // Test with Kryo serialization
+ Table kryoSerialized = TestHelpers.KryoHelpers.roundTripSerialize(serializableTable);
+ verifySerializedTable(kryoSerialized, table, 2);
+ }
+
+ private void verifySerializedTable(
+ Table deserialized, Table original, int expectedSnapshotCount) {
+ // Verify that basic operations work using serialized table metadata
+ // (tables requiring remote scan planning serialize full metadata)
+ assertThat(deserialized.schema().asStruct()).isEqualTo(original.schema().asStruct());
+ assertThat(deserialized.spec().specId()).isEqualTo(original.spec().specId());
+ assertThat(deserialized.location()).isEqualTo(original.location());
+ assertThat(deserialized.properties()).containsAllEntriesOf(original.properties());
+
+ // Verify snapshot operations work using serialized metadata
+ assertThat(deserialized.currentSnapshot()).isNotNull();
+ assertThat(deserialized.currentSnapshot().snapshotId())
+ .isEqualTo(original.currentSnapshot().snapshotId());
+
+ // Verify snapshots are accessible
+ assertThat(deserialized.snapshots()).isNotNull();
+ int snapshotCount = 0;
+ for (Snapshot snapshot : deserialized.snapshots()) {
+ snapshotCount++;
Review Comment:
I don't think we need the counter here, since you can check the size right away as mentioned in an earlier comment
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST: Reuse table metadata as part of LoadTable in serializable table [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14944:
URL: https://github.com/apache/iceberg/pull/14944#discussion_r2668097093
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -186,6 +201,13 @@ private int formatVersion(Table table) {
}
}
+ private TableMetadata getTableMetadata(Table table) {
Review Comment:
minor: usage of `get` prefix
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST: Reuse table metadata as part of LoadTable in serializable table [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14944:
URL: https://github.com/apache/iceberg/pull/14944#discussion_r2668121119
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -3528,4 +3530,201 @@ private static List<HTTPRequest> allRequests(RESTCatalogAdapter adapter) {
verify(adapter, atLeastOnce()).execute(captor.capture(), any(), any(), any());
return captor.getAllValues();
}
+
+ @Test
+ public void testSerializableTableWithRESTCatalog() throws IOException, ClassNotFoundException {
+ // Create a table with data through REST catalog
+ Schema schema =
+ new Schema(
+ required(1, "id", Types.IntegerType.get()),
+ required(2, "data", Types.StringType.get()));
+
+ TableIdentifier ident = TableIdentifier.of(Namespace.of("ns"), "table");
+ restCatalog.createNamespace(ident.namespace());
+ Table table = restCatalog.createTable(ident, schema);
+
+ // Add data files to create snapshots
+ table.newAppend().appendFile(FILE_A).commit();
+ table.newAppend().appendFile(FILE_B).commit();
+
+ // Create SerializableTable from REST catalog
+ SerializableTable serializableTable = (SerializableTable) SerializableTable.copyOf(table);
+
+ // Test with Java serialization
+ Table javaSerialized = TestHelpers.roundTripSerialize(serializableTable);
+ verifySerializedTable(javaSerialized, table, 2);
+
+ // Test with Kryo serialization
+ Table kryoSerialized = TestHelpers.KryoHelpers.roundTripSerialize(serializableTable);
+ verifySerializedTable(kryoSerialized, table, 2);
+ }
+
+ private void verifySerializedTable(
+ Table deserialized, Table original, int expectedSnapshotCount) {
+ // Verify that basic operations work using serialized table metadata
+ // (tables requiring remote scan planning serialize full metadata)
+ assertThat(deserialized.schema().asStruct()).isEqualTo(original.schema().asStruct());
+ assertThat(deserialized.spec().specId()).isEqualTo(original.spec().specId());
+ assertThat(deserialized.location()).isEqualTo(original.location());
+ assertThat(deserialized.properties()).containsAllEntriesOf(original.properties());
+
+ // Verify snapshot operations work using serialized metadata
+ assertThat(deserialized.currentSnapshot()).isNotNull();
+ assertThat(deserialized.currentSnapshot().snapshotId())
+ .isEqualTo(original.currentSnapshot().snapshotId());
+
+ // Verify snapshots are accessible
+ assertThat(deserialized.snapshots()).isNotNull();
+ int snapshotCount = 0;
+ for (Snapshot snapshot : deserialized.snapshots()) {
+ snapshotCount++;
+ assertThat(snapshot).isNotNull();
+ }
+ assertThat(snapshotCount).isEqualTo(expectedSnapshotCount);
+
+ // Verify scan operations work
+ assertThat(deserialized.newScan()).isNotNull();
+ }
+
+ @Test
+ public void testSerializableTableWithRESTCatalogAndSchemaEvolution()
+ throws IOException, ClassNotFoundException {
+ // Create initial table
+ Schema initialSchema =
+ new Schema(
+ required(1, "id", Types.IntegerType.get()),
+ required(2, "data", Types.StringType.get()));
+
+ TableIdentifier ident = TableIdentifier.of(Namespace.of("ns"), "schema_evolution_table");
+ restCatalog.createNamespace(ident.namespace());
+ Table table = restCatalog.createTable(ident, initialSchema);
+
+ // Evolve schema
+ table.updateSchema().addColumn("new_col1", Types.IntegerType.get()).commit();
+ table.newAppend().appendFile(FILE_A).commit();
+
+ // Evolve schema again
+ table.updateSchema().addColumn("new_col2", Types.StringType.get()).commit();
+ table.newAppend().appendFile(FILE_B).commit();
+
+ // Create and serialize table
+ SerializableTable serializableTable = (SerializableTable) SerializableTable.copyOf(table);
+
+ // Test with Java serialization
+ Table javaSerialized = TestHelpers.roundTripSerialize(serializableTable);
+ verifySchemaEvolution(javaSerialized, table);
+
+ // Test with Kryo serialization
+ Table kryoSerialized = TestHelpers.KryoHelpers.roundTripSerialize(serializableTable);
+ verifySchemaEvolution(kryoSerialized, table);
+ }
+
+ private void verifySchemaEvolution(Table deserialized, Table original) {
+ // Verify current schema
+ assertThat(deserialized.schema().columns()).hasSize(original.schema().columns().size());
+
+ // Verify all historical schemas are preserved
+ assertThat(deserialized.schemas()).isNotNull();
+ assertThat(deserialized.schemas().size()).isEqualTo(original.schemas().size());
+
+ // Verify scans work
+ assertThat(deserialized.newScan()).isNotNull();
+ }
+
+ @Test
+ public void testSerializableTableWithRESTCatalogAndSnapshotRefs()
+ throws IOException, ClassNotFoundException {
+ // Create table with data
+ Schema schema =
+ new Schema(
+ required(1, "id", Types.IntegerType.get()),
+ required(2, "data", Types.StringType.get()));
+
+ TableIdentifier ident = TableIdentifier.of(Namespace.of("ns"), "refs_table");
+ restCatalog.createNamespace(ident.namespace());
+ Table table = restCatalog.createTable(ident, schema);
+
+ // Create first snapshot and tag it
+ table.newAppend().appendFile(FILE_A).commit();
+ long taggedSnapshotId = table.currentSnapshot().snapshotId();
+ table
+ .manageSnapshots()
+ .createTag("production", taggedSnapshotId)
+ .setMaxRefAgeMs("production", Long.MAX_VALUE)
+ .commit();
+
+ // Add more data
+ table.newAppend().appendFile(FILE_B).commit();
+
+ // Serialize and deserialize
+ SerializableTable serializableTable = (SerializableTable) SerializableTable.copyOf(table);
+
+ // Test with Java serialization
+ Table javaSerialized = TestHelpers.roundTripSerialize(serializableTable);
+ verifySnapshotRefs(javaSerialized, taggedSnapshotId);
+
+ // Test with Kryo serialization
+ Table kryoSerialized = TestHelpers.KryoHelpers.roundTripSerialize(serializableTable);
+ verifySnapshotRefs(kryoSerialized, taggedSnapshotId);
+ }
+
+ private void verifySnapshotRefs(Table deserialized, long taggedSnapshotId) {
+ // Verify refs are preserved
+ assertThat(deserialized.refs()).isNotNull();
+ assertThat(deserialized.refs()).containsKey("production");
+ assertThat(deserialized.refs().get("production").snapshotId()).isEqualTo(taggedSnapshotId);
Review Comment:
these 3 statements can be simplified to a single assertion: `assertThat(deserialized.refs()).containsEntry("production", ...)`.
Also given that this check is so simple, I don't think we need to put this into a separate method
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST: Reuse table metadata as part of LoadTable in serializable table [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14944:
URL: https://github.com/apache/iceberg/pull/14944#discussion_r2668121119
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -3528,4 +3530,201 @@ private static List<HTTPRequest> allRequests(RESTCatalogAdapter adapter) {
verify(adapter, atLeastOnce()).execute(captor.capture(), any(), any(), any());
return captor.getAllValues();
}
+
+ @Test
+ public void testSerializableTableWithRESTCatalog() throws IOException, ClassNotFoundException {
+ // Create a table with data through REST catalog
+ Schema schema =
+ new Schema(
+ required(1, "id", Types.IntegerType.get()),
+ required(2, "data", Types.StringType.get()));
+
+ TableIdentifier ident = TableIdentifier.of(Namespace.of("ns"), "table");
+ restCatalog.createNamespace(ident.namespace());
+ Table table = restCatalog.createTable(ident, schema);
+
+ // Add data files to create snapshots
+ table.newAppend().appendFile(FILE_A).commit();
+ table.newAppend().appendFile(FILE_B).commit();
+
+ // Create SerializableTable from REST catalog
+ SerializableTable serializableTable = (SerializableTable) SerializableTable.copyOf(table);
+
+ // Test with Java serialization
+ Table javaSerialized = TestHelpers.roundTripSerialize(serializableTable);
+ verifySerializedTable(javaSerialized, table, 2);
+
+ // Test with Kryo serialization
+ Table kryoSerialized = TestHelpers.KryoHelpers.roundTripSerialize(serializableTable);
+ verifySerializedTable(kryoSerialized, table, 2);
+ }
+
+ private void verifySerializedTable(
+ Table deserialized, Table original, int expectedSnapshotCount) {
+ // Verify that basic operations work using serialized table metadata
+ // (tables requiring remote scan planning serialize full metadata)
+ assertThat(deserialized.schema().asStruct()).isEqualTo(original.schema().asStruct());
+ assertThat(deserialized.spec().specId()).isEqualTo(original.spec().specId());
+ assertThat(deserialized.location()).isEqualTo(original.location());
+ assertThat(deserialized.properties()).containsAllEntriesOf(original.properties());
+
+ // Verify snapshot operations work using serialized metadata
+ assertThat(deserialized.currentSnapshot()).isNotNull();
+ assertThat(deserialized.currentSnapshot().snapshotId())
+ .isEqualTo(original.currentSnapshot().snapshotId());
+
+ // Verify snapshots are accessible
+ assertThat(deserialized.snapshots()).isNotNull();
+ int snapshotCount = 0;
+ for (Snapshot snapshot : deserialized.snapshots()) {
+ snapshotCount++;
+ assertThat(snapshot).isNotNull();
+ }
+ assertThat(snapshotCount).isEqualTo(expectedSnapshotCount);
+
+ // Verify scan operations work
+ assertThat(deserialized.newScan()).isNotNull();
+ }
+
+ @Test
+ public void testSerializableTableWithRESTCatalogAndSchemaEvolution()
+ throws IOException, ClassNotFoundException {
+ // Create initial table
+ Schema initialSchema =
+ new Schema(
+ required(1, "id", Types.IntegerType.get()),
+ required(2, "data", Types.StringType.get()));
+
+ TableIdentifier ident = TableIdentifier.of(Namespace.of("ns"), "schema_evolution_table");
+ restCatalog.createNamespace(ident.namespace());
+ Table table = restCatalog.createTable(ident, initialSchema);
+
+ // Evolve schema
+ table.updateSchema().addColumn("new_col1", Types.IntegerType.get()).commit();
+ table.newAppend().appendFile(FILE_A).commit();
+
+ // Evolve schema again
+ table.updateSchema().addColumn("new_col2", Types.StringType.get()).commit();
+ table.newAppend().appendFile(FILE_B).commit();
+
+ // Create and serialize table
+ SerializableTable serializableTable = (SerializableTable) SerializableTable.copyOf(table);
+
+ // Test with Java serialization
+ Table javaSerialized = TestHelpers.roundTripSerialize(serializableTable);
+ verifySchemaEvolution(javaSerialized, table);
+
+ // Test with Kryo serialization
+ Table kryoSerialized = TestHelpers.KryoHelpers.roundTripSerialize(serializableTable);
+ verifySchemaEvolution(kryoSerialized, table);
+ }
+
+ private void verifySchemaEvolution(Table deserialized, Table original) {
+ // Verify current schema
+ assertThat(deserialized.schema().columns()).hasSize(original.schema().columns().size());
+
+ // Verify all historical schemas are preserved
+ assertThat(deserialized.schemas()).isNotNull();
+ assertThat(deserialized.schemas().size()).isEqualTo(original.schemas().size());
+
+ // Verify scans work
+ assertThat(deserialized.newScan()).isNotNull();
+ }
+
+ @Test
+ public void testSerializableTableWithRESTCatalogAndSnapshotRefs()
+ throws IOException, ClassNotFoundException {
+ // Create table with data
+ Schema schema =
+ new Schema(
+ required(1, "id", Types.IntegerType.get()),
+ required(2, "data", Types.StringType.get()));
+
+ TableIdentifier ident = TableIdentifier.of(Namespace.of("ns"), "refs_table");
+ restCatalog.createNamespace(ident.namespace());
+ Table table = restCatalog.createTable(ident, schema);
+
+ // Create first snapshot and tag it
+ table.newAppend().appendFile(FILE_A).commit();
+ long taggedSnapshotId = table.currentSnapshot().snapshotId();
+ table
+ .manageSnapshots()
+ .createTag("production", taggedSnapshotId)
+ .setMaxRefAgeMs("production", Long.MAX_VALUE)
+ .commit();
+
+ // Add more data
+ table.newAppend().appendFile(FILE_B).commit();
+
+ // Serialize and deserialize
+ SerializableTable serializableTable = (SerializableTable) SerializableTable.copyOf(table);
+
+ // Test with Java serialization
+ Table javaSerialized = TestHelpers.roundTripSerialize(serializableTable);
+ verifySnapshotRefs(javaSerialized, taggedSnapshotId);
+
+ // Test with Kryo serialization
+ Table kryoSerialized = TestHelpers.KryoHelpers.roundTripSerialize(serializableTable);
+ verifySnapshotRefs(kryoSerialized, taggedSnapshotId);
+ }
+
+ private void verifySnapshotRefs(Table deserialized, long taggedSnapshotId) {
+ // Verify refs are preserved
+ assertThat(deserialized.refs()).isNotNull();
+ assertThat(deserialized.refs()).containsKey("production");
+ assertThat(deserialized.refs().get("production").snapshotId()).isEqualTo(taggedSnapshotId);
Review Comment:
these 3 statements can be simplified to a single assertion: `assertThat(deserialized.refs()).containsEntry("production", ...)`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST: Reuse table metadata as part of LoadTable in serializable table [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14944:
URL: https://github.com/apache/iceberg/pull/14944#discussion_r2668114261
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -3528,4 +3530,201 @@ private static List<HTTPRequest> allRequests(RESTCatalogAdapter adapter) {
verify(adapter, atLeastOnce()).execute(captor.capture(), any(), any(), any());
return captor.getAllValues();
}
+
+ @Test
+ public void testSerializableTableWithRESTCatalog() throws IOException, ClassNotFoundException {
+ // Create a table with data through REST catalog
+ Schema schema =
+ new Schema(
+ required(1, "id", Types.IntegerType.get()),
+ required(2, "data", Types.StringType.get()));
+
+ TableIdentifier ident = TableIdentifier.of(Namespace.of("ns"), "table");
+ restCatalog.createNamespace(ident.namespace());
+ Table table = restCatalog.createTable(ident, schema);
+
+ // Add data files to create snapshots
+ table.newAppend().appendFile(FILE_A).commit();
+ table.newAppend().appendFile(FILE_B).commit();
+
+ // Create SerializableTable from REST catalog
+ SerializableTable serializableTable = (SerializableTable) SerializableTable.copyOf(table);
+
+ // Test with Java serialization
+ Table javaSerialized = TestHelpers.roundTripSerialize(serializableTable);
+ verifySerializedTable(javaSerialized, table, 2);
+
+ // Test with Kryo serialization
+ Table kryoSerialized = TestHelpers.KryoHelpers.roundTripSerialize(serializableTable);
+ verifySerializedTable(kryoSerialized, table, 2);
+ }
+
+ private void verifySerializedTable(
+ Table deserialized, Table original, int expectedSnapshotCount) {
+ // Verify that basic operations work using serialized table metadata
+ // (tables requiring remote scan planning serialize full metadata)
+ assertThat(deserialized.schema().asStruct()).isEqualTo(original.schema().asStruct());
+ assertThat(deserialized.spec().specId()).isEqualTo(original.spec().specId());
+ assertThat(deserialized.location()).isEqualTo(original.location());
+ assertThat(deserialized.properties()).containsAllEntriesOf(original.properties());
+
+ // Verify snapshot operations work using serialized metadata
+ assertThat(deserialized.currentSnapshot()).isNotNull();
+ assertThat(deserialized.currentSnapshot().snapshotId())
+ .isEqualTo(original.currentSnapshot().snapshotId());
+
+ // Verify snapshots are accessible
+ assertThat(deserialized.snapshots()).isNotNull();
Review Comment:
do we actually need `expectedSnapshotCount`? Can't we just compare whether the snapshot count of `original` is the same via `.hasSameSizeAs(original.snapshots())`?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org