zhoujinsong commented on code in PR #4284:
URL: https://github.com/apache/amoro/pull/4284#discussion_r3673868395
##########
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:
It seems the FQCN here is not necessary.
##########
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:
Format version from `UpgradeFormatVersion` is not applied to the builder，
You may change the format version like:
```java
TableMetadata.Builder builder = TableMetadata.buildFromEmpty();
formatVersion.ifPresent(builder::upgradeFormatVersion);
```
##########
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:
The `icebergMetadata` built from `UpdateTableRequest` updates already
contains everything needed (schema, spec, sort order, location, properties, and
appended data files).
The reconstructed `CreateTableRequest` is only used to construct the
`InternalTableCreator` object, but `creator.create(icebergMetadata)` writes the
`icebergMetadata` directly — the `CreateTableRequest` fields are not used for
the actual metadata file.
This works correctly today but is confusing to read. Could you add a comment
explaining that `CreateTableRequest` here is only to satisfy the current
`InternalTableCreator` constructor signature, and the actual metadata written
is `icebergMetadata`?
Maybe we can refactor the `InternalTableCreator` later.
##########
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:
Maybe we can add a `testStagedCreateWithAppendFiles ` to verify the CTAS
case.
--
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]
