From 8de31cdb3cc74a82144c0899a6636a70d6588b66 Mon Sep 17 00:00:00 2001 From: cai can <94670132+caican00@users.noreply.github.com> Date: Fri, 11 Oct 2024 18:52:50 -0700 Subject: [PATCH] [#5094] Fix(catalog-lakehouse-paimon): make some table properties from reserved to immutable (#5095) ### What changes were proposed in this pull request? make some table properties from reserved to immutable. ``` merge-engine sequence.field rowkind.field ``` ### Why are the changes needed? these table properties are immutable in Paimon. Fix: https://github.com/apache/gravitino/issues/5094 ### Does this PR introduce _any_ user-facing change? updated related doc. ### How was this patch tested? modified UT. Co-authored-by: caican --- .../paimon/PaimonTablePropertiesMetadata.java | 10 ++- .../paimon/TestGravitinoPaimonTable.java | 73 ++++++++++++------- docs/lakehouse-paimon-catalog.md | 9 ++- 3 files changed, 59 insertions(+), 33 deletions(-) diff --git a/catalogs/catalog-lakehouse-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/PaimonTablePropertiesMetadata.java b/catalogs/catalog-lakehouse-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/PaimonTablePropertiesMetadata.java index 1c57e5b2cc5..671dd9d6682 100644 --- a/catalogs/catalog-lakehouse-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/PaimonTablePropertiesMetadata.java +++ b/catalogs/catalog-lakehouse-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/PaimonTablePropertiesMetadata.java @@ -18,6 +18,7 @@ */ package org.apache.gravitino.catalog.lakehouse.paimon; +import static org.apache.gravitino.connector.PropertyEntry.stringImmutablePropertyEntry; import static org.apache.gravitino.connector.PropertyEntry.stringReservedPropertyEntry; import com.google.common.collect.ImmutableList; @@ -51,9 +52,12 @@ public class PaimonTablePropertiesMetadata extends BasePropertiesMetadata { stringReservedPropertyEntry(COMMENT, "The table comment", true), stringReservedPropertyEntry(OWNER, "The table owner", false), stringReservedPropertyEntry(BUCKET_KEY, "The table bucket key", false), - stringReservedPropertyEntry(MERGE_ENGINE, "The table merge engine", false), - stringReservedPropertyEntry(SEQUENCE_FIELD, "The table sequence field", false), - stringReservedPropertyEntry(ROWKIND_FIELD, "The table rowkind field", false), + stringImmutablePropertyEntry( + MERGE_ENGINE, "The table merge engine", false, null, false, false), + stringImmutablePropertyEntry( + SEQUENCE_FIELD, "The table sequence field", false, null, false, false), + stringImmutablePropertyEntry( + ROWKIND_FIELD, "The table rowkind field", false, null, false, false), stringReservedPropertyEntry(PRIMARY_KEY, "The table primary key", false), stringReservedPropertyEntry(PARTITION, "The table partition", false)); PROPERTIES_METADATA = Maps.uniqueIndex(propertyEntries, PropertyEntry::getName); diff --git a/catalogs/catalog-lakehouse-paimon/src/test/java/org/apache/gravitino/catalog/lakehouse/paimon/TestGravitinoPaimonTable.java b/catalogs/catalog-lakehouse-paimon/src/test/java/org/apache/gravitino/catalog/lakehouse/paimon/TestGravitinoPaimonTable.java index 05e36219d99..466ecb3459d 100644 --- a/catalogs/catalog-lakehouse-paimon/src/test/java/org/apache/gravitino/catalog/lakehouse/paimon/TestGravitinoPaimonTable.java +++ b/catalogs/catalog-lakehouse-paimon/src/test/java/org/apache/gravitino/catalog/lakehouse/paimon/TestGravitinoPaimonTable.java @@ -382,16 +382,41 @@ void testTableProperty() { try (PaimonCatalogOperations ops = new PaimonCatalogOperations()) { ops.initialize( initBackendCatalogProperties(), entity.toCatalogInfo(), PAIMON_PROPERTIES_METADATA); - Map map = Maps.newHashMap(); - map.put(PaimonTablePropertiesMetadata.COMMENT, "test"); - map.put(PaimonTablePropertiesMetadata.OWNER, "test"); - map.put(PaimonTablePropertiesMetadata.BUCKET_KEY, "test"); - map.put(PaimonTablePropertiesMetadata.MERGE_ENGINE, "test"); - map.put(PaimonTablePropertiesMetadata.SEQUENCE_FIELD, "test"); - map.put(PaimonTablePropertiesMetadata.ROWKIND_FIELD, "test"); - map.put(PaimonTablePropertiesMetadata.PRIMARY_KEY, "test"); - map.put(PaimonTablePropertiesMetadata.PARTITION, "test"); - for (Map.Entry entry : map.entrySet()) { + HashMap reservedProps = + new HashMap() { + { + put(PaimonTablePropertiesMetadata.COMMENT, "test"); + put(PaimonTablePropertiesMetadata.OWNER, "test"); + put(PaimonTablePropertiesMetadata.BUCKET_KEY, "test"); + put(PaimonTablePropertiesMetadata.PRIMARY_KEY, "test"); + put(PaimonTablePropertiesMetadata.PARTITION, "test"); + } + }; + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + PropertiesMetadataHelpers.validatePropertyForCreate( + paimonCatalog.tablePropertiesMetadata(), reservedProps)); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + PropertiesMetadataHelpers.validatePropertyForAlter( + paimonCatalog.tablePropertiesMetadata(), reservedProps, Collections.emptyMap())); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + PropertiesMetadataHelpers.validatePropertyForAlter( + paimonCatalog.tablePropertiesMetadata(), Collections.emptyMap(), reservedProps)); + + Map immutableProps = + new HashMap() { + { + put(PaimonTablePropertiesMetadata.MERGE_ENGINE, "test"); + put(PaimonTablePropertiesMetadata.SEQUENCE_FIELD, "test"); + put(PaimonTablePropertiesMetadata.ROWKIND_FIELD, "test"); + } + }; + for (Map.Entry entry : immutableProps.entrySet()) { HashMap properties = new HashMap() { { @@ -399,26 +424,18 @@ void testTableProperty() { } }; PropertiesMetadata metadata = paimonCatalog.tablePropertiesMetadata(); + Assertions.assertDoesNotThrow( + () -> PropertiesMetadataHelpers.validatePropertyForCreate(metadata, properties)); Assertions.assertThrows( IllegalArgumentException.class, - () -> PropertiesMetadataHelpers.validatePropertyForCreate(metadata, properties)); - } - - map = Maps.newHashMap(); - map.put("key1", "val1"); - map.put("key2", "val2"); - for (Map.Entry entry : map.entrySet()) { - HashMap properties = - new HashMap() { - { - put(entry.getKey(), entry.getValue()); - } - }; - PropertiesMetadata metadata = paimonCatalog.tablePropertiesMetadata(); - Assertions.assertDoesNotThrow( - () -> { - PropertiesMetadataHelpers.validatePropertyForCreate(metadata, properties); - }); + () -> + PropertiesMetadataHelpers.validatePropertyForAlter( + metadata, properties, Collections.emptyMap())); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + PropertiesMetadataHelpers.validatePropertyForAlter( + metadata, Collections.emptyMap(), properties)); } } } diff --git a/docs/lakehouse-paimon-catalog.md b/docs/lakehouse-paimon-catalog.md index b9c2772bc7a..2a8226668c0 100644 --- a/docs/lakehouse-paimon-catalog.md +++ b/docs/lakehouse-paimon-catalog.md @@ -175,11 +175,16 @@ The Gravitino server doesn't allow passing the following reserved fields. | `comment` | The table comment. | | `owner` | The table owner. | | `bucket-key` | The table bucket-key. | +| `primary-key` | The table primary-key. | +| `partition` | The table partition. | + +The Gravitino server doesn't allow the following immutable fields to be modified, but allows them to be specified when creating a new table. + +| Configuration item | Description | +|------------------------------------|--------------------------------------------------------------| | `merge-engine` | The table merge-engine. | | `sequence.field` | The table sequence.field. | | `rowkind.field` | The table rowkind.field. | -| `primary-key` | The table primary-key. | -| `partition` | The table partition. | ### Table operations