johntomcat7408-cmyk commented on code in PR #4284:
URL: https://github.com/apache/amoro/pull/4284#discussion_r3680271266
##########
amoro-ams/src/main/java/org/apache/amoro/server/table/internal/InternalTableCreator.java:
##########
@@ -25,16 +25,19 @@
/** Interface to create an internal table. */
public interface InternalTableCreator extends Closeable {
- /**
- * Do all things about create an internal table, and prepare the {@link
TableMetadata} for {@link
- * InternalTableManager#createTable(java.lang.String, TableMetadata)}
- */
+ /** Build Iceberg metadata without writing files or registering the table. */
+ org.apache.iceberg.TableMetadata stage();
Review Comment:
Both `org.apache.amoro.server.table.TableMetadata` and
`org.apache.iceberg.TableMetadata` are used in this interface. Java cannot
import both classes with the same simple name, so one of them must remain fully
qualified. I kept the existing Amoro import because both `create` methods
return that type; reversing the import would only move the FQCN.
##########
amoro-ams/src/main/java/org/apache/amoro/server/RestCatalogService.java:
##########
@@ -358,25 +366,101 @@ public void loadTable(Context ctx) {
/** POST PREFIX/v1/catalogs/{catalog}/namespaces/{namespace}/tables/{table}
*/
public void commitTable(Context ctx) {
- handleTable(
- ctx,
- handler -> {
- UpdateTableRequest request = bodyAsClass(ctx,
UpdateTableRequest.class);
- TableOperations ops = handler.newTableOperator();
- TableMetadata base = ops.current();
- if (base == null) {
- throw new CommitFailedException("table metadata lost.");
- }
+ UpdateTableRequest request = bodyAsClass(ctx, UpdateTableRequest.class);
+ request.validate();
+ if (isCreate(request)) {
+ handleNamespace(
+ ctx,
+ (catalog, database) -> {
+ String tableName = ctx.pathParam("table");
+ Preconditions.checkNotNull(tableName, "table name is null");
+ return commitCreateTable(catalog, database, tableName, request);
+ });
+ } else {
+ handleTable(
+ ctx,
+ handler -> {
+ TableOperations ops = handler.newTableOperator();
+ TableMetadata base = ops.current();
+ if (base == null) {
+ throw new CommitFailedException("table metadata lost.");
+ }
- TableMetadata.Builder builder = TableMetadata.buildFrom(base);
- request.requirements().forEach(r -> r.validate(base));
- request.updates().forEach(u -> u.applyTo(builder));
- TableMetadata newMetadata = builder.build();
+ TableMetadata.Builder builder = TableMetadata.buildFrom(base);
+ request.requirements().forEach(r -> r.validate(base));
+ request.updates().forEach(u -> u.applyTo(builder));
+ TableMetadata newMetadata = builder.build();
- ops.commit(base, newMetadata);
- TableMetadata current = ops.current();
- return
LoadTableResponse.builder().withTableMetadata(current).build();
- });
+ ops.commit(base, newMetadata);
+ TableMetadata current = ops.current();
+ return
LoadTableResponse.builder().withTableMetadata(current).build();
+ });
+ }
+ }
+
+ private LoadTableResponse commitCreateTable(
+ InternalCatalog catalog, String database, String tableName,
UpdateTableRequest request) {
+ request.requirements().forEach(requirement ->
requirement.validate((TableMetadata) null));
+
+ Optional<Integer> formatVersion =
+ request.updates().stream()
+ .filter(MetadataUpdate.UpgradeFormatVersion.class::isInstance)
+ .map(MetadataUpdate.UpgradeFormatVersion.class::cast)
+ .map(MetadataUpdate.UpgradeFormatVersion::formatVersion)
+ .findFirst();
+ TableMetadata.Builder builder =
Review Comment:
Thanks for pointing this out. In Iceberg 1.7.2,
`TableMetadata.buildFromEmpty()` starts at format v2, so
`builder.upgradeFormatVersion(1)` would be a downgrade and fail for an explicit
v1 request. `buildFromEmpty(int)` initializes the requested version; I made
that overload selection explicit and added coverage for both default v2 and
explicit v1.
##########
amoro-ams/src/main/java/org/apache/amoro/server/RestCatalogService.java:
##########
@@ -358,25 +366,101 @@ public void loadTable(Context ctx) {
/** POST PREFIX/v1/catalogs/{catalog}/namespaces/{namespace}/tables/{table}
*/
public void commitTable(Context ctx) {
- handleTable(
- ctx,
- handler -> {
- UpdateTableRequest request = bodyAsClass(ctx,
UpdateTableRequest.class);
- TableOperations ops = handler.newTableOperator();
- TableMetadata base = ops.current();
- if (base == null) {
- throw new CommitFailedException("table metadata lost.");
- }
+ UpdateTableRequest request = bodyAsClass(ctx, UpdateTableRequest.class);
+ request.validate();
+ if (isCreate(request)) {
+ handleNamespace(
+ ctx,
+ (catalog, database) -> {
+ String tableName = ctx.pathParam("table");
+ Preconditions.checkNotNull(tableName, "table name is null");
+ return commitCreateTable(catalog, database, tableName, request);
+ });
+ } else {
+ handleTable(
+ ctx,
+ handler -> {
+ TableOperations ops = handler.newTableOperator();
+ TableMetadata base = ops.current();
+ if (base == null) {
+ throw new CommitFailedException("table metadata lost.");
+ }
- TableMetadata.Builder builder = TableMetadata.buildFrom(base);
- request.requirements().forEach(r -> r.validate(base));
- request.updates().forEach(u -> u.applyTo(builder));
- TableMetadata newMetadata = builder.build();
+ TableMetadata.Builder builder = TableMetadata.buildFrom(base);
+ request.requirements().forEach(r -> r.validate(base));
+ request.updates().forEach(u -> u.applyTo(builder));
+ TableMetadata newMetadata = builder.build();
- ops.commit(base, newMetadata);
- TableMetadata current = ops.current();
- return
LoadTableResponse.builder().withTableMetadata(current).build();
- });
+ ops.commit(base, newMetadata);
+ TableMetadata current = ops.current();
+ return
LoadTableResponse.builder().withTableMetadata(current).build();
+ });
+ }
+ }
+
+ private LoadTableResponse commitCreateTable(
+ InternalCatalog catalog, String database, String tableName,
UpdateTableRequest request) {
+ request.requirements().forEach(requirement ->
requirement.validate((TableMetadata) null));
+
+ Optional<Integer> formatVersion =
+ request.updates().stream()
+ .filter(MetadataUpdate.UpgradeFormatVersion.class::isInstance)
+ .map(MetadataUpdate.UpgradeFormatVersion.class::cast)
+ .map(MetadataUpdate.UpgradeFormatVersion::formatVersion)
+ .findFirst();
+ TableMetadata.Builder builder =
+
formatVersion.map(TableMetadata::buildFromEmpty).orElseGet(TableMetadata::buildFromEmpty);
+ request.updates().forEach(update -> update.applyTo(builder));
+ TableMetadata icebergMetadata = builder.build();
+
+ CreateTableRequest createRequest =
Review Comment:
Added the comment. It now clarifies that the reconstructed
`CreateTableRequest` only satisfies the current `InternalTableCreator`
construction API, while `creator.create(icebergMetadata)` persists the fully
reconstructed metadata.
##########
amoro-ams/src/test/java/org/apache/amoro/server/TestInternalIcebergCatalogService.java:
##########
@@ -284,6 +289,29 @@ public void testCreateTableInNamespaceLocation() {
Assertions.assertEquals(namespaceLocation + "/" + table,
created.location());
}
+ @Test
Review Comment:
Added `testStagedCreateWithAppendFiles`. It writes real data files, appends
them inside the staged transaction, verifies the table is still not registered
before the final commit, and then verifies the committed snapshot and data-file
paths.
--
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]
