Skip to content

Commit

Permalink
[apache#1546] fix(lakehouse-iceberg): fix create Iceberg table failed…
Browse files Browse the repository at this point in the history
… if comment is null (apache#1653)

### What changes were proposed in this pull request?
remove reserveProperties logic from CreateTableRequest since reserved
properties is handled by property system

### Why are the changes needed?

1. when build IcebergTable, comment equals to null is checked when
comment added to properties .
```
      if (null != comment) {
        icebergTable.properties.putIfAbsent(ICEBERG_COMMENT_FIELD_NAME, comment);
      }
```
2. when build CreateTableRequest, the previous implement remove all
reserved properties from property map including comment, and then add
comment to map, the bug happens, not checking the null, but this overall
code is redundant


Fix: apache#1546 

### Does this PR introduce _any_ user-facing change?
no

### How was this patch tested?
add UT
  • Loading branch information
FANNG1 authored and mchades committed Jan 24, 2024
1 parent b8fa890 commit c21c90e
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import com.datastrato.gravitino.catalog.lakehouse.iceberg.converter.FromIcebergSortOrder;
import com.datastrato.gravitino.catalog.lakehouse.iceberg.converter.ToIcebergPartitionSpec;
import com.datastrato.gravitino.catalog.lakehouse.iceberg.converter.ToIcebergSortOrder;
import com.datastrato.gravitino.catalog.lakehouse.iceberg.ops.IcebergTableOpsHelper;
import com.datastrato.gravitino.catalog.rel.BaseTable;
import com.datastrato.gravitino.meta.AuditInfo;
import com.google.common.collect.Maps;
Expand Down Expand Up @@ -45,16 +44,12 @@ private IcebergTable() {}

public CreateTableRequest toCreateTableRequest() {
Schema schema = ConvertUtil.toIcebergSchema(this);

Map<String, String> resultProperties =
Maps.newHashMap(IcebergTableOpsHelper.removeReservedProperties(properties));
resultProperties.putIfAbsent(ICEBERG_COMMENT_FIELD_NAME, comment);
CreateTableRequest.Builder builder =
CreateTableRequest.builder()
.withName(name)
.withLocation(location)
.withSchema(schema)
.setProperties(resultProperties)
.setProperties(properties)
.withPartitionSpec(ToIcebergPartitionSpec.toPartitionSpec(schema, partitioning))
.withWriteOrder(ToIcebergSortOrder.toSortOrder(schema, sortOrders));
return builder.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,7 @@
import com.google.common.collect.Lists;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
import javax.ws.rs.NotSupportedException;
import lombok.Getter;
import lombok.Setter;
Expand Down Expand Up @@ -309,12 +307,6 @@ public IcebergTableChange buildIcebergTableChanges(
return icebergTableChange;
}

public static Map<String, String> removeReservedProperties(Map<String, String> createProperties) {
return createProperties.entrySet().stream()
.filter(entry -> !IcebergReservedProperties.contains(entry.getKey()))
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
}

/**
* Gravitino only supports single-level namespace storage management, which differs from Iceberg.
* Therefore, we need to handle this difference here.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,21 @@ void testOperationIcebergSchema() {
Assertions.assertTrue(schemaNames.contains(schemaName));
}

@Test
void testCreateTableWithNullComment() {
ColumnDTO[] columns = createColumns();
NameIdentifier tableIdentifier =
NameIdentifier.of(metalakeName, catalogName, schemaName, tableName);

TableCatalog tableCatalog = catalog.asTableCatalog();
Table createdTable =
tableCatalog.createTable(tableIdentifier, columns, null, null, null, null, null);
Assertions.assertNull(createdTable.comment());

Table loadTable = tableCatalog.loadTable(tableIdentifier);
Assertions.assertNull(loadTable.comment());
}

@Test
void testCreateAndLoadIcebergTable() {
// Create table from Gravitino API
Expand Down

0 comments on commit c21c90e

Please sign in to comment.