This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 3fb5c74b38 [#13590] fix(core): Compare column defaults in a canonical
representation (#13603)
3fb5c74b38 is described below
commit 3fb5c74b385d514b44b5b51f4c81929ce7cf02fd
Author: Qi Yu <[email protected]>
AuthorDate: Wed Sep 30 17:16:02 2026 +0800
[#13590] fix(core): Compare column defaults in a canonical representation
(#13603)
### What changes were proposed in this pull request?
Compare column defaults through the existing FunctionArg DTO
representation in TableOperationDispatcher, including difference
logging. Preserve the distinction between an unset default and explicit
NULL. No storage interface or format changes.
### Why are the changes needed?
Connector snapshots use DTO expressions, while relational storage
restores API expressions. Comparing them directly makes unchanged
defaults appear different, causing repeated loadTable calls to rewrite
table and column metadata.
Fix: #13590
### Does this PR introduce _any_ user-facing change?
Loading an unchanged table no longer rewrites metadata because of
default-expression representation differences. Real default changes
still synchronize. No API or configuration changes.
### How was this patch tested?
- Added nine regression cases using connector snapshots and a
memory-store fixture with production relational column
serialization/deserialization. They cover unset/NULL transitions,
literals, literal type changes, functions, nested arguments, unparsed
expressions, and field references. The tests assert that snapshot/stored
expression classes differ. Repeated loads produce no updates; an
external default change on the second column produces one update,
preserves its ID, and leaves the first column unchanged. All nine cases
fail before the fix.
- Updated existing column assertions to compare canonical defaults
without requiring an incidental rewrite to DTOs.
- 84 tests passed across TestTableOperationDispatcher, TestPOConverters,
and TestOperationDispatcherSnapshots with -PskipITs.
- ./gradlew spotlessApply and git diff --check passed.
---
.../catalog/TableOperationDispatcher.java | 32 ++++-
.../catalog/TestTableOperationDispatcher.java | 156 ++++++++++++++++++++-
2 files changed, 185 insertions(+), 3 deletions(-)
diff --git
a/core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java
b/core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java
index 77d2a5a8b7..ce9593d586 100644
---
a/core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java
+++
b/core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java
@@ -56,6 +56,8 @@ import org.apache.gravitino.StringIdentifier;
import org.apache.gravitino.connector.HasPropertyMetadata;
import org.apache.gravitino.connector.MaskAndOmitKeys;
import org.apache.gravitino.connector.capability.Capability;
+import org.apache.gravitino.dto.rel.expressions.FunctionArg;
+import org.apache.gravitino.dto.util.DTOConverters;
import org.apache.gravitino.exceptions.GravitinoRuntimeException;
import org.apache.gravitino.exceptions.NoSuchEntityException;
import org.apache.gravitino.exceptions.NoSuchSchemaException;
@@ -70,6 +72,7 @@ import org.apache.gravitino.meta.TableEntity;
import org.apache.gravitino.rel.Column;
import org.apache.gravitino.rel.Table;
import org.apache.gravitino.rel.TableChange;
+import org.apache.gravitino.rel.expressions.Expression;
import org.apache.gravitino.rel.expressions.distributions.Distribution;
import org.apache.gravitino.rel.expressions.distributions.Distributions;
import org.apache.gravitino.rel.expressions.sorts.SortOrder;
@@ -770,7 +773,32 @@ public class TableOperationDispatcher extends
OperationDispatcher implements Tab
&& Objects.equal(left.comment(), right.comment())
&& left.nullable() == right.nullable()
&& left.autoIncrement() == right.autoIncrement()
- && Objects.equal(left.defaultValue(), right.defaultValue());
+ && isSameDefaultValue(left.defaultValue(), right.defaultValue());
+ }
+
+ @VisibleForTesting
+ static boolean isSameDefaultValue(@Nullable Expression left, @Nullable
Expression right) {
+ if (Objects.equal(left, right)) {
+ return true;
+ }
+
+ // Connector snapshots use DTOs, while relational storage restores API
expressions. Compare
+ // their persisted representation rather than implementation-specific
equals methods.
+ try {
+ return Objects.equal(toDefaultValueArg(left), toDefaultValueArg(right));
+ } catch (IllegalArgumentException e) {
+ // A default value that cannot be converted is treated as changed
instead of failing the
+ // comparison.
+ LOG.debug("Failed to compare column default values {} and {}", left,
right, e);
+ return false;
+ }
+ }
+
+ @Nullable
+ private static FunctionArg toDefaultValueArg(@Nullable Expression
expression) {
+ return expression == null ||
expression.equals(Column.DEFAULT_VALUE_NOT_SET)
+ ? null
+ : DTOConverters.toFunctionArg(expression);
}
private String columnDifferenceContent(
@@ -806,7 +834,7 @@ public class TableOperationDispatcher extends
OperationDispatcher implements Tab
"autoIncrement[catalog=%s, store=%s]",
catalogColumn.autoIncrement(), entityColumn.autoIncrement()));
}
- if (!Objects.equal(catalogColumn.defaultValue(),
entityColumn.defaultValue())) {
+ if (!isSameDefaultValue(catalogColumn.defaultValue(),
entityColumn.defaultValue())) {
differences.add(
String.format(
"defaultValue[catalog=%s, store=%s]",
diff --git
a/core/src/test/java/org/apache/gravitino/catalog/TestTableOperationDispatcher.java
b/core/src/test/java/org/apache/gravitino/catalog/TestTableOperationDispatcher.java
index 27f7fb1902..af0464e5a3 100644
---
a/core/src/test/java/org/apache/gravitino/catalog/TestTableOperationDispatcher.java
+++
b/core/src/test/java/org/apache/gravitino/catalog/TestTableOperationDispatcher.java
@@ -28,12 +28,14 @@ import static
org.apache.gravitino.TestBasePropertiesMetadata.COMMENT_KEY;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.clearInvocations;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.reset;
+import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import com.google.common.collect.ImmutableMap;
@@ -50,6 +52,7 @@ import java.util.function.Function;
import java.util.function.Supplier;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
+import java.util.stream.Stream;
import org.apache.commons.lang3.reflect.FieldUtils;
import org.apache.gravitino.Config;
import org.apache.gravitino.Entity;
@@ -77,15 +80,25 @@ import org.apache.gravitino.meta.TableEntity;
import org.apache.gravitino.rel.Column;
import org.apache.gravitino.rel.Table;
import org.apache.gravitino.rel.TableChange;
+import org.apache.gravitino.rel.expressions.Expression;
+import org.apache.gravitino.rel.expressions.FunctionExpression;
+import org.apache.gravitino.rel.expressions.NamedReference;
+import org.apache.gravitino.rel.expressions.UnparsedExpression;
import org.apache.gravitino.rel.expressions.distributions.Distributions;
import org.apache.gravitino.rel.expressions.literals.Literals;
import org.apache.gravitino.rel.expressions.sorts.SortOrder;
import org.apache.gravitino.rel.expressions.transforms.Transform;
import org.apache.gravitino.rel.indexes.Indexes;
import org.apache.gravitino.rel.types.Types;
+import org.apache.gravitino.storage.relational.po.ColumnPO;
+import org.apache.gravitino.storage.relational.po.TablePO;
+import org.apache.gravitino.storage.relational.utils.POConverters;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
public class TestTableOperationDispatcher extends TestOperationDispatcher {
static TableOperationDispatcher tableOperationDispatcher;
@@ -1046,6 +1059,122 @@ public class TestTableOperationDispatcher extends
TestOperationDispatcher {
Assertions.assertTrue(entityStore.exists(tableIdent, TABLE));
}
+ /**
+ * Verifies that persisted defaults only trigger synchronization when their
content changes.
+ *
+ * @param originalDefault the initial column default
+ * @param changedDefault the default applied outside Gravitino
+ */
+ @ParameterizedTest
+ @MethodSource("columnDefaultChanges")
+ public void testLoadTableComparesPersistedDefaults(
+ Expression originalDefault, Expression changedDefault) throws
IOException {
+ Namespace tableNs = Namespace.of(metalake, catalog, "schema_default_" +
idGenerator.nextId());
+ Map<String, String> props = ImmutableMap.of("k1", "v1", "k2", "v2");
+
schemaOperationDispatcher.createSchema(NameIdentifier.of(tableNs.levels()),
"comment", props);
+ NameIdentifier tableIdent = NameIdentifier.of(tableNs, "table_defaults");
+ tableOperationDispatcher.createTable(
+ tableIdent,
+ new Column[] {
+ Column.of("unchanged", Types.StringType.get(), "comment", true,
false, Literals.NULL),
+ Column.of("col", Types.StringType.get(), "comment", true, false,
originalDefault)
+ },
+ "comment",
+ props);
+
+ // Use the production relational serialization/deserialization path. The
connector snapshot
+ // contains DTOs, while persisted columns are restored as API expressions.
+ doAnswer(
+ invocation -> {
+ TableEntity table = (TableEntity) invocation.callRealMethod();
+ TablePO tablePO =
+ POConverters.initializeTablePOWithVersion(
+ table,
+
TablePO.builder().withMetalakeId(1L).withCatalogId(2L).withSchemaId(3L));
+ return TableEntity.builder()
+ .withId(table.id())
+ .withName(table.name())
+ .withNamespace(table.namespace())
+ .withComment(table.comment())
+ .withProperties(table.properties())
+ .withColumns(
+ POConverters.fromColumnPOs(
+ POConverters.initializeColumnPOs(
+ tablePO, table.columns(),
ColumnPO.ColumnOpType.CREATE)))
+ .withPartitioning(table.partitioning())
+ .withDistribution(table.distribution())
+ .withSortOrders(table.sortOrders())
+ .withIndexes(table.indexes())
+ .withAuditInfo(table.auditInfo())
+ .build();
+ })
+ .when(entityStore)
+ .get(tableIdent, TABLE, TableEntity.class);
+ TableEntity original = entityStore.get(tableIdent, TABLE,
TableEntity.class);
+ clearInvocations(entityStore);
+ for (int i = 0; i < 10; i++) {
+ Table loaded = tableOperationDispatcher.loadTable(tableIdent);
+ Assertions.assertNotEquals(
+ original.columns().get(0).defaultValue().getClass(),
+ loaded.columns()[0].defaultValue().getClass());
+ Assertions.assertEquals(
+
DTOConverters.toFunctionArg(original.columns().get(0).defaultValue()),
+ loaded.columns()[0].defaultValue());
+ Assertions.assertEquals(
+ DTOConverters.toDTO(
+ Column.of("col", Types.StringType.get(), "comment", true,
false, originalDefault))
+ .defaultValue(),
+ loaded.columns()[1].defaultValue());
+ }
+ verify(entityStore, never()).update(eq(tableIdent), eq(TableEntity.class),
eq(TABLE), any());
+ Assertions.assertEquals(
+ original.auditInfo(), entityStore.get(tableIdent, TABLE,
TableEntity.class).auditInfo());
+
+ TestCatalog testCatalog =
+ (TestCatalog)
+ catalogManager.loadCatalogAndWrap(NameIdentifier.of(metalake,
catalog)).catalog();
+ ((TestCatalogOperations) testCatalog.ops())
+ .alterTable(
+ tableIdent, TableChange.updateColumnDefaultValue(new String[]
{"col"}, changedDefault));
+ tableOperationDispatcher.loadTable(tableIdent);
+ verify(entityStore, times(1)).update(eq(tableIdent),
eq(TableEntity.class), eq(TABLE), any());
+ TableEntity updatedTable = entityStore.get(tableIdent, TABLE,
TableEntity.class);
+ Map<String, ColumnEntity> updatedColumns =
+ updatedTable.columns().stream()
+ .collect(Collectors.toMap(ColumnEntity::name,
Function.identity()));
+ Assertions.assertEquals(original.columns().get(0),
updatedColumns.get("unchanged"));
+ ColumnEntity updated = updatedColumns.get("col");
+ Assertions.assertEquals(original.columns().get(1).id(), updated.id());
+ Assertions.assertEquals(
+ DTOConverters.toDTO(
+ Column.of("col", Types.StringType.get(), "comment", true,
false, changedDefault))
+ .defaultValue(),
+ DTOConverters.toDTO(
+ Column.of(
+ "col", Types.StringType.get(), "comment", true, false,
updated.defaultValue()))
+ .defaultValue());
+ for (int i = 0; i < 10; i++) {
+ tableOperationDispatcher.loadTable(tableIdent);
+ }
+ verify(entityStore, times(1)).update(eq(tableIdent),
eq(TableEntity.class), eq(TABLE), any());
+ }
+
+ private static Stream<Arguments> columnDefaultChanges() {
+ return Stream.of(
+ Arguments.of(Column.DEFAULT_VALUE_NOT_SET, Literals.NULL),
+ Arguments.of(Literals.NULL, Column.DEFAULT_VALUE_NOT_SET),
+ Arguments.of(Literals.stringLiteral("a"), Literals.stringLiteral("b")),
+ Arguments.of(Literals.integerLiteral(1), Literals.integerLiteral(2)),
+ Arguments.of(Literals.integerLiteral(1), Literals.stringLiteral("1")),
+ Arguments.of(FunctionExpression.of("now"),
FunctionExpression.of("current_date")),
+ Arguments.of(
+ FunctionExpression.of("f", FunctionExpression.of("g",
Literals.integerLiteral(1))),
+ FunctionExpression.of("f", FunctionExpression.of("g",
Literals.integerLiteral(2)))),
+ Arguments.of(
+ UnparsedExpression.of("CURRENT_TIMESTAMP"),
UnparsedExpression.of("CURRENT_DATE")),
+ Arguments.of(NamedReference.field("a"), NamedReference.field("b")));
+ }
+
@Test
public void testCreateAndLoadTableWithColumn() throws IOException {
Namespace tableNs = Namespace.of(metalake, catalog, "schema91");
@@ -1433,6 +1562,28 @@ public class TestTableOperationDispatcher extends
TestOperationDispatcher {
TableChange.deleteColumn(new String[] {"s", "z"}, false)));
}
+ @Test
+ public void testIsSameDefaultValue() {
+ Expression literal = Literals.stringLiteral("1");
+ Assertions.assertTrue(
+
TableOperationDispatcher.isSameDefaultValue(DTOConverters.toFunctionArg(literal),
literal));
+ Assertions.assertFalse(
+ TableOperationDispatcher.isSameDefaultValue(literal,
Literals.stringLiteral("2")));
+
+ // A missing default value is the same as an unset one.
+ Assertions.assertTrue(TableOperationDispatcher.isSameDefaultValue(null,
null));
+ Assertions.assertTrue(
+ TableOperationDispatcher.isSameDefaultValue(null,
Column.DEFAULT_VALUE_NOT_SET));
+ Assertions.assertFalse(TableOperationDispatcher.isSameDefaultValue(null,
literal));
+
+ // A default value that cannot be converted is reported as changed instead
of throwing.
+ Expression unsupported = () -> Expression.EMPTY_EXPRESSION;
+
Assertions.assertTrue(TableOperationDispatcher.isSameDefaultValue(unsupported,
unsupported));
+
Assertions.assertFalse(TableOperationDispatcher.isSameDefaultValue(unsupported,
literal));
+ Assertions.assertFalse(
+ TableOperationDispatcher.isSameDefaultValue(unsupported,
Column.DEFAULT_VALUE_NOT_SET));
+ }
+
private Map<String, Long> columnIds(NameIdentifier tableIdent) throws
IOException {
return entityStore.get(tableIdent, TABLE,
TableEntity.class).columns().stream()
.collect(Collectors.toMap(ColumnEntity::name, ColumnEntity::id));
@@ -1774,7 +1925,10 @@ public class TestTableOperationDispatcher extends
TestOperationDispatcher {
Assertions.assertEquals(e.nullable(), actualColumn.nullable());
Assertions.assertEquals(e.autoIncrement(),
actualColumn.autoIncrement());
Assertions.assertEquals(
- DTOConverters.toDTO(e).defaultValue(),
actualColumn.defaultValue());
+ DTOConverters.toDTO(e).defaultValue(),
+ actualColumn.defaultValue().equals(Column.DEFAULT_VALUE_NOT_SET)
+ ? Column.DEFAULT_VALUE_NOT_SET
+ : DTOConverters.toFunctionArg(actualColumn.defaultValue()));
});
}
