diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java index f1c7b9862fe..b57d3690de5 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/TableRepository.java @@ -202,7 +202,7 @@ public abstract class TableRepository { updatedTable.setId(storedTable.getId()); validateRelationships(updatedTable, database, newOwner); - // Carry forward non empty description + // Carry forward non-empty description if (storedTable.getDescription() != null && !storedTable.getDescription().isEmpty()) { // Update description only when it is empty updatedTable.setDescription(storedTable.getDescription()); @@ -211,7 +211,7 @@ public abstract class TableRepository { EntityUtil.removeTagsByPrefix(tagDAO(), storedTable.getFullyQualifiedName()); updatedTable.setTags(EntityUtil.mergeTags(updatedTable.getTags(), storedTable.getTags())); - updateColumns(storedTable, updatedTable); + updateColumns(storedTable, updatedTable, false); storeTable(updatedTable, true); updateRelationships(storedTable, updatedTable); @@ -468,7 +468,7 @@ public abstract class TableRepository { // Remove previous tags. Merge tags from the update and the existing tags EntityUtil.removeTags(tagDAO(), original.getFullyQualifiedName()); - updateColumns(original, updated); + updateColumns(original, updated, true); storeTable(updated, true); updateRelationships(original, updated); @@ -515,8 +515,7 @@ public abstract class TableRepository { } //TODO modified columns - private void updateColumns(List storedColumns, List updatedColumns, List addedColumns, - List deletedColumns) { + private void updateColumns(List storedColumns, List updatedColumns, boolean patchOperation) { // Carry forward the user generated metadata from existing columns to new columns for (Column updated : updatedColumns) { // Find stored column matching name, data type and ordinal position @@ -529,13 +528,14 @@ public abstract class TableRepository { .orElse(null); if (stored == null) { // TODO versioning of schema - addedColumns.add(updated); LOG.info("Column {} was newly added", updated.getFullyQualifiedName()); continue; } - // Carry forward user generated metadata to the columns from the update - if (stored.getDescription() != null && !stored.getDescription().isEmpty()) { + // For patch operation don't overwrite updated column with stored column description + // For put operation overwrite updated column with non-empty or non-null stored column description + if (!patchOperation && + stored.getDescription() != null && !stored.getDescription().isEmpty()) { updated.setDescription(stored.getDescription()); // Carry forward non-empty description } @@ -546,7 +546,7 @@ public abstract class TableRepository { updated.setTags(updated.getTags()); if (updated.getChildren() != null && stored.getChildren() != null) { - updateColumns(stored.getChildren(), updated.getChildren(), addedColumns, deletedColumns); + updateColumns(stored.getChildren(), updated.getChildren(), patchOperation); } } @@ -562,14 +562,13 @@ public abstract class TableRepository { .orElse(null); if (updated == null) { // TODO versioning of schema addedColumns.add(stored); - deletedColumns.add(stored); LOG.info("Column {} was deleted", stored.getFullyQualifiedName()); } } } - private void updateColumns(Table storedTable, Table updatedTable) { - updateColumns(storedTable.getColumns(), updatedTable.getColumns(), new ArrayList<>(), new ArrayList<>()); + private void updateColumns(Table storedTable, Table updatedTable, boolean patchOperation) { + updateColumns(storedTable.getColumns(), updatedTable.getColumns(), patchOperation); storedTable.setColumns(updatedTable.getColumns()); } diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/databases/TableResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/databases/TableResourceTest.java index 9d8bc8ece01..386b522d0a1 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/databases/TableResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/databases/TableResourceTest.java @@ -92,6 +92,7 @@ import static org.openmetadata.catalog.resources.databases.DatabaseResourceTest. import static org.openmetadata.catalog.resources.services.DatabaseServiceResourceTest.createService; import static org.openmetadata.catalog.type.ColumnDataType.ARRAY; import static org.openmetadata.catalog.type.ColumnDataType.BIGINT; +import static org.openmetadata.catalog.type.ColumnDataType.BINARY; import static org.openmetadata.catalog.type.ColumnDataType.CHAR; import static org.openmetadata.catalog.type.ColumnDataType.FLOAT; import static org.openmetadata.catalog.type.ColumnDataType.INT; @@ -229,13 +230,12 @@ public class TableResourceTest extends CatalogApplicationTest { } private static Column getColumn(String name, ColumnDataType columnDataType, TagLabel tag) { - return getColumn(name, columnDataType, null, tag); } private static Column getColumn(String name, ColumnDataType columnDataType, String dataTypeDisplay, TagLabel tag) { List tags = tag == null ? new ArrayList<>() : singletonList(tag); - return new Column().withName(name).withDataType(columnDataType) + return new Column().withName(name).withDataType(columnDataType).withDescription(name) .withDataTypeDisplay(dataTypeDisplay).withTags(tags); } @@ -438,9 +438,10 @@ public class TableResourceTest extends CatalogApplicationTest { // // Create a table with column c1, type BIGINT, description c1 and tag USER_ADDRESS_TAB_LABEL // - List tags = new ArrayList<>(singletonList(USER_ADDRESS_TAG_LABEL)); - List columns = singletonList(new Column().withName("c1").withDataType(BIGINT) - .withOrdinalPosition(1).withDescription("c1").withTags(tags)); + List tags = new ArrayList<>(); + tags.add(USER_ADDRESS_TAG_LABEL); + List columns = new ArrayList<>(); + columns.add(getColumn("c1", BIGINT, null).withTags(tags)); CreateTable request = create(test).withColumns(columns); Table table = createAndCheckTable(request, adminAuthHeaders()); @@ -458,8 +459,7 @@ public class TableResourceTest extends CatalogApplicationTest { // tags.add(USER_BANK_ACCOUNT_TAG_LABEL); List updatedColumns = new ArrayList<>(); - updatedColumns.add(new Column().withName("c1").withDataType(BIGINT) - .withTags(tags).withOrdinalPosition(1)); + updatedColumns.add(getColumn("c1", BIGINT, null).withTags(tags)); table = updateAndCheckTable(request.withColumns(updatedColumns), OK, adminAuthHeaders()); // Ensure tag usage counts are updated @@ -470,7 +470,7 @@ public class TableResourceTest extends CatalogApplicationTest { // // Add a new column and make sure it is added by PUT // - updatedColumns.add(new Column().withName("c2").withDataType(ColumnDataType.BINARY).withOrdinalPosition(2) + updatedColumns.add(getColumn("c2", BINARY, null).withOrdinalPosition(2) .withDataLength(10).withFullyQualifiedName(table.getFullyQualifiedName() + ".c2").withTags(tags)); table = updateAndCheckTable(request.withColumns(updatedColumns), OK, adminAuthHeaders()); assertEquals(2, table.getColumns().size()); @@ -1024,7 +1024,7 @@ public class TableResourceTest extends CatalogApplicationTest { } @Test - public void patch_tableColumnTags_200_ok(TestInfo test) throws HttpResponseException, JsonProcessingException { + public void patch_tableColumns_200_ok(TestInfo test) throws HttpResponseException, JsonProcessingException { // Create table without description, table tags, tier, owner, tableType, and tableConstraints List columns = new ArrayList<>(); columns.add(getColumn("c1", INT, USER_ADDRESS_TAG_LABEL)); @@ -1033,10 +1033,11 @@ public class TableResourceTest extends CatalogApplicationTest { Table table = createTable(create(test).withColumns(columns), adminAuthHeaders()); - // Update the columns - columns.get(0).setTags(List.of(USER_ADDRESS_TAG_LABEL, USER_BANK_ACCOUNT_TAG_LABEL)); // Add a tag - columns.get(1).setTags(List.of(USER_ADDRESS_TAG_LABEL)); // No change in tag - columns.get(2).setTags(new ArrayList<>()); // Remove tag + // Update the column tags and description + columns.get(0).withDescription("new0") + .withTags(List.of(USER_ADDRESS_TAG_LABEL, USER_BANK_ACCOUNT_TAG_LABEL)); // Add a tag + columns.get(1).withDescription("new1").withTags(List.of(USER_ADDRESS_TAG_LABEL));// No change in tag + columns.get(2).withDescription("new3").withTags(new ArrayList<>()); // Remove tag table = patchTableColumnAttributesAndCheck(table, columns, adminAuthHeaders()); validateColumns(columns, table.getColumns()); @@ -1242,6 +1243,7 @@ public class TableResourceTest extends CatalogApplicationTest { private static void validateColumn(Column expectedColumn, Column actualColumn) throws HttpResponseException { assertNotNull(actualColumn.getFullyQualifiedName()); assertEquals(expectedColumn.getName(), actualColumn.getName()); + assertEquals(expectedColumn.getDescription(), actualColumn.getDescription()); assertEquals(expectedColumn.getDataType(), actualColumn.getDataType()); assertEquals(expectedColumn.getArrayDataType(), actualColumn.getArrayDataType()); if (expectedColumn.getDataTypeDisplay() != null) {