Skip to content

Commit

Permalink
[#1546] fix(lakehouse-iceberg): fix create Iceberg table failed if co…
Browse files Browse the repository at this point in the history
…mment is null (#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: #1546 

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

### How was this patch tested?
add UT
  • Loading branch information
FANNG1 authored Jan 24, 2024
1 parent 0ec0d3d commit f29cc7b
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 f29cc7b

Please sign in to comment.