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 731c873f3f [#13232] fix(api): remove Distribution.equals(Distribution)
overload trap (#13233)
731c873f3f is described below
commit 731c873f3fd885591c34049c0f78770f780eaaee
Author: YangJie <[email protected]>
AuthorDate: Sun Sep 20 08:17:45 2026 -0400
[#13232] fix(api): remove Distribution.equals(Distribution) overload trap
(#13233)
### What changes were proposed in this pull request?
This removes the `default boolean equals(Distribution)` overload from
the `Distribution` interface and adds
`Distributions.isNone(Distribution)`, a structural NONE check that
preserves the cross-representation (DTO/impl) semantics the overload
provided at its call sites. `isNone` accepts a `@Nullable` argument and
treats a null distribution as NONE (a null means no distribution was
specified). All eleven call sites are migrated (common `DTOConverters`,
the jdbc-mysql/postgresql/clickhouse/oceanbase/hologres and
iceberg/delta/hive converters, the hive IT, and the trino
`HiveMetadataAdapter`); two now-redundant `distribution == null ||`
guards (`DTOConverters`, `DeltaTableOperations`) are simplified
accordingly. The javadoc documents that implementations must override
`equals(Object)`/`hashCode`.
### Why are the changes needed?
The overload never overrode `Object.equals`, so the same objects
compared through an `Object` reference (identity) and as `Distribution`
(structural) could disagree, and implementations relying on the default
got identity equality in collections.
Fix: #13232
### Does this PR introduce _any_ user-facing change?
Yes. The public `default boolean equals(Distribution)` overload is
removed from the `Distribution` interface, so implementers must override
`equals(Object)`/`hashCode` (all in-repo implementations already do).
`Distributions.isNone(Distribution)` is added and treats null as NONE.
`TableOperationDispatcher` already normalizes a null distribution to
`Distributions.NONE` before it reaches catalogs, so behavior at the
migrated call sites is unchanged at runtime.
### How was this patch tested?
Added `TestDistributions`, which pins `Distributions.isNone` across
DTO-NONE, impl-NONE, HASH, EVEN, and null (null is treated as NONE). The
overload removal is enforced at compile time; because `isNone` is new,
this test does not run against the pre-fix tree.
---
.../expressions/distributions/Distribution.java | 29 +++-----
.../expressions/distributions/Distributions.java | 16 +++++
.../distributions/TestDistributions.java | 80 ++++++++++++++++++++++
.../operations/ClickHouseTableOperations.java | 2 +-
.../operation/HologresTableOperations.java | 2 +-
.../operation/OceanBaseTableOperations.java | 2 +-
.../hive/integration/test/CatalogHive2IT.java | 4 +-
.../mysql/operation/MysqlTableOperations.java | 2 +-
.../operation/PostgreSqlTableOperations.java | 2 +-
.../lakehouse/delta/DeltaTableOperations.java | 2 +-
.../iceberg/IcebergCatalogOperations.java | 2 +-
.../hive/converter/HiveTableConverter.java | 2 +-
.../apache/gravitino/dto/util/DTOConverters.java | 2 +-
.../catalog/hive/HiveMetadataAdapter.java | 2 +-
14 files changed, 119 insertions(+), 30 deletions(-)
diff --git
a/api/src/main/java/org/apache/gravitino/rel/expressions/distributions/Distribution.java
b/api/src/main/java/org/apache/gravitino/rel/expressions/distributions/Distribution.java
index 564b328fde..e393caf311 100644
---
a/api/src/main/java/org/apache/gravitino/rel/expressions/distributions/Distribution.java
+++
b/api/src/main/java/org/apache/gravitino/rel/expressions/distributions/Distribution.java
@@ -18,11 +18,20 @@
*/
package org.apache.gravitino.rel.expressions.distributions;
-import java.util.Arrays;
import org.apache.gravitino.annotation.Evolving;
import org.apache.gravitino.rel.expressions.Expression;
-/** An interface that defines how data is distributed across partitions. */
+/**
+ * An interface that defines how data is distributed across partitions.
+ *
+ * <p>This interface intentionally does not define a {@code boolean
equals(Distribution)} overload.
+ * Such an overload would not override {@link Object#equals(Object)}, so it
would be invisible to
+ * {@code HashSet}/{@code HashMap} and to any code comparing through {@code
Object} references,
+ * which would make structural equality silently dispatch-dependent.
Implementations must override
+ * {@link Object#equals(Object)} and {@link Object#hashCode()} themselves
(both {@code
+ * DistributionImpl} and {@code DistributionDTO} do). Use {@link
Distributions#isNone(Distribution)}
+ * to test for the NONE distribution across representations.
+ */
@Evolving
public interface Distribution extends Expression {
@@ -46,20 +55,4 @@ public interface Distribution extends Expression {
default Expression[] children() {
return expressions();
}
-
- /**
- * Indicates whether some other object is "equal to" this one.
- *
- * @param distribution The reference distribution object with which to
compare.
- * @return returns true if this object is the same as the obj argument;
false otherwise.
- */
- default boolean equals(Distribution distribution) {
- if (distribution == null) {
- return false;
- }
-
- return strategy().equals(distribution.strategy())
- && number() == distribution.number()
- && Arrays.equals(expressions(), distribution.expressions());
- }
}
diff --git
a/api/src/main/java/org/apache/gravitino/rel/expressions/distributions/Distributions.java
b/api/src/main/java/org/apache/gravitino/rel/expressions/distributions/Distributions.java
index c7aeb62189..555d1bd30b 100644
---
a/api/src/main/java/org/apache/gravitino/rel/expressions/distributions/Distributions.java
+++
b/api/src/main/java/org/apache/gravitino/rel/expressions/distributions/Distributions.java
@@ -20,6 +20,7 @@ package org.apache.gravitino.rel.expressions.distributions;
import java.util.Arrays;
import java.util.Objects;
+import javax.annotation.Nullable;
import org.apache.gravitino.rel.expressions.Expression;
import org.apache.gravitino.rel.expressions.NamedReference;
@@ -36,6 +37,21 @@ public class Distributions {
public static final Distribution NONE =
new DistributionImpl(Strategy.NONE, 0, Expression.EMPTY_EXPRESSION);
+ /**
+ * Returns true if the distribution is the NONE distribution. A null
distribution means "no
+ * distribution specified" and is therefore treated as NONE. For non-null
values the comparison is
+ * structural, so both the built-in implementation and DTO representations
of NONE match.
+ *
+ * @param distribution The distribution to check; may be null.
+ * @return true if the distribution is null or represents the NONE
distribution.
+ */
+ public static boolean isNone(@Nullable Distribution distribution) {
+ return distribution == null
+ || (distribution.strategy() == Strategy.NONE
+ && distribution.number() == 0
+ && Arrays.equals(distribution.expressions(),
Expression.EMPTY_EXPRESSION));
+ }
+
/** List bucketing strategy hash, TODO: #1505 Separate the bucket number
from the Distribution. */
public static final Distribution HASH =
new DistributionImpl(Strategy.HASH, 0, Expression.EMPTY_EXPRESSION);
diff --git
a/api/src/test/java/org/apache/gravitino/rel/expressions/distributions/TestDistributions.java
b/api/src/test/java/org/apache/gravitino/rel/expressions/distributions/TestDistributions.java
new file mode 100644
index 0000000000..9763957c3e
--- /dev/null
+++
b/api/src/test/java/org/apache/gravitino/rel/expressions/distributions/TestDistributions.java
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.rel.expressions.distributions;
+
+import org.apache.gravitino.rel.expressions.Expression;
+import org.apache.gravitino.rel.expressions.NamedReference;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestDistributions {
+
+ // A separate Distribution implementation, used to prove
Distributions.isNone recognizes NONE
+ // structurally across representations rather than by concrete type.
DistributionDTO.NONE would be
+ // the natural other representation, but DistributionDTO lives in the common
module and the api
+ // module does not depend on common (main or test), so it is not on this
test classpath. An
+ // anonymous implementation stands in for that cross-representation check.
+ private static Distribution distributionOf(Strategy strategy, int number,
Expression... exprs) {
+ return new Distribution() {
+ @Override
+ public Strategy strategy() {
+ return strategy;
+ }
+
+ @Override
+ public int number() {
+ return number;
+ }
+
+ @Override
+ public Expression[] expressions() {
+ return exprs;
+ }
+ };
+ }
+
+ @Test
+ public void testIsNone() {
+ Assertions.assertTrue(Distributions.isNone(Distributions.NONE));
+
+ // A structurally equal NONE distribution from a different implementation
also matches.
+ Assertions.assertTrue(
+ Distributions.isNone(distributionOf(Strategy.NONE, 0,
Expression.EMPTY_EXPRESSION)));
+
+ // A null distribution means "no distribution specified", which is treated
as NONE.
+ Assertions.assertTrue(Distributions.isNone(null));
+ Assertions.assertFalse(Distributions.isNone(Distributions.HASH));
+ Assertions.assertFalse(Distributions.isNone(Distributions.RANGE));
+ Assertions.assertFalse(
+ Distributions.isNone(Distributions.even(10,
NamedReference.field("col"))));
+ Assertions.assertFalse(
+ Distributions.isNone(distributionOf(Strategy.NONE, 5,
Expression.EMPTY_EXPRESSION)));
+ }
+
+ @Test
+ public void testImplEqualityIsStructuralWithinSameClass() {
+ Assertions.assertEquals(Distributions.NONE, Distributions.NONE);
+ Assertions.assertNotEquals(
+ Distributions.even(10, NamedReference.field("col")),
+ Distributions.even(10, NamedReference.field("other")));
+ Assertions.assertEquals(
+ Distributions.even(10, NamedReference.field("col")),
+ Distributions.even(10, NamedReference.field("col")));
+ }
+}
diff --git
a/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java
b/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java
index 148172cefe..55f4ad5d59 100644
---
a/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java
+++
b/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java
@@ -333,7 +333,7 @@ public class ClickHouseTableOperations extends
JdbcTableOperations {
SortOrder[] sortOrders) {
Preconditions.checkArgument(
- Distributions.NONE.equals(distribution), "ClickHouse does not support
distribution");
+ Distributions.isNone(distribution), "ClickHouse does not support
distribution");
StringBuilder sqlBuilder = new StringBuilder();
diff --git
a/catalogs-contrib/catalog-jdbc-hologres/src/main/java/org/apache/gravitino/catalog/hologres/operation/HologresTableOperations.java
b/catalogs-contrib/catalog-jdbc-hologres/src/main/java/org/apache/gravitino/catalog/hologres/operation/HologresTableOperations.java
index c78037d291..1ace95f3b1 100644
---
a/catalogs-contrib/catalog-jdbc-hologres/src/main/java/org/apache/gravitino/catalog/hologres/operation/HologresTableOperations.java
+++
b/catalogs-contrib/catalog-jdbc-hologres/src/main/java/org/apache/gravitino/catalog/hologres/operation/HologresTableOperations.java
@@ -227,7 +227,7 @@ public class HologresTableOperations extends
JdbcTableOperations
List<String> withEntries = new ArrayList<>();
// Add distribution_key from Distribution parameter
- if (!Distributions.NONE.equals(distribution)) {
+ if (!Distributions.isNone(distribution)) {
validateDistribution(distribution);
String distributionColumns =
Arrays.stream(distribution.expressions())
diff --git
a/catalogs-contrib/catalog-jdbc-oceanbase/src/main/java/org/apache/gravitino/catalog/oceanbase/operation/OceanBaseTableOperations.java
b/catalogs-contrib/catalog-jdbc-oceanbase/src/main/java/org/apache/gravitino/catalog/oceanbase/operation/OceanBaseTableOperations.java
index aa9a49cf53..bd09770adc 100644
---
a/catalogs-contrib/catalog-jdbc-oceanbase/src/main/java/org/apache/gravitino/catalog/oceanbase/operation/OceanBaseTableOperations.java
+++
b/catalogs-contrib/catalog-jdbc-oceanbase/src/main/java/org/apache/gravitino/catalog/oceanbase/operation/OceanBaseTableOperations.java
@@ -77,7 +77,7 @@ public class OceanBaseTableOperations extends
JdbcTableOperations {
"Currently we do not support Partitioning in oceanbase");
}
- if (!Distributions.NONE.equals(distribution)) {
+ if (!Distributions.isNone(distribution)) {
throw new UnsupportedOperationException("OceanBase does not support
distribution");
}
diff --git
a/catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/integration/test/CatalogHive2IT.java
b/catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/integration/test/CatalogHive2IT.java
index 6ec869c7f9..8b3d9a9158 100644
---
a/catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/integration/test/CatalogHive2IT.java
+++
b/catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/integration/test/CatalogHive2IT.java
@@ -453,8 +453,8 @@ public class CatalogHive2IT extends BaseIT {
}
private void compareDistributions(Distribution expected, Distribution
actual) {
- boolean expectedEmpty = expected == null ||
Distributions.NONE.equals(expected);
- boolean actualEmpty = actual == null || Distributions.NONE.equals(actual);
+ boolean expectedEmpty = expected == null || Distributions.isNone(expected);
+ boolean actualEmpty = actual == null || Distributions.isNone(actual);
Assertions.assertEquals(expectedEmpty, actualEmpty);
if (expectedEmpty) {
return;
diff --git
a/catalogs/catalog-jdbc-mysql/src/main/java/org/apache/gravitino/catalog/mysql/operation/MysqlTableOperations.java
b/catalogs/catalog-jdbc-mysql/src/main/java/org/apache/gravitino/catalog/mysql/operation/MysqlTableOperations.java
index 813f44b6de..0417d05d08 100644
---
a/catalogs/catalog-jdbc-mysql/src/main/java/org/apache/gravitino/catalog/mysql/operation/MysqlTableOperations.java
+++
b/catalogs/catalog-jdbc-mysql/src/main/java/org/apache/gravitino/catalog/mysql/operation/MysqlTableOperations.java
@@ -90,7 +90,7 @@ public class MysqlTableOperations extends JdbcTableOperations
{
}
Preconditions.checkArgument(
- Distributions.NONE.equals(distribution), "MySQL does not support
distribution");
+ Distributions.isNone(distribution), "MySQL does not support
distribution");
validateIncrementCol(columns, indexes);
StringBuilder sqlBuilder = new StringBuilder();
diff --git
a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java
b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java
index 9ce61a0281..bf6d7e99ee 100644
---
a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java
+++
b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java
@@ -165,7 +165,7 @@ public class PostgreSqlTableOperations extends
JdbcTableOperations
"Currently we do not support Partitioning in PostgreSQL");
}
Preconditions.checkArgument(
- Distributions.NONE.equals(distribution), "PostgreSQL does not support
distribution");
+ Distributions.isNone(distribution), "PostgreSQL does not support
distribution");
StringBuilder sqlBuilder = new StringBuilder();
sqlBuilder
diff --git
a/catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/delta/DeltaTableOperations.java
b/catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/delta/DeltaTableOperations.java
index f2b0f2ec55..2991a56437 100644
---
a/catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/delta/DeltaTableOperations.java
+++
b/catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/delta/DeltaTableOperations.java
@@ -198,7 +198,7 @@ public class DeltaTableOperations extends
ManagedTableOperations {
}
Preconditions.checkArgument(
- distribution == null || distribution.equals(Distributions.NONE),
+ Distributions.isNone(distribution),
"Delta table doesn't support specifying distribution in CREATE TABLE. "
+ "Distribution is not applicable for external Delta tables.");
diff --git
a/catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java
b/catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java
index 140db99ff0..79eec2fd8d 100644
---
a/catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java
+++
b/catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/IcebergCatalogOperations.java
@@ -609,7 +609,7 @@ public class IcebergCatalogOperations
// Gravitino NONE distribution means the client side doesn't specify
distribution, which is
// not the same as none distribution in Iceberg.
- if (Distributions.NONE.equals(distribution)) {
+ if (Distributions.isNone(distribution)) {
distribution =
getIcebergDefaultDistribution(sortOrders.length > 0,
partitioning.length > 0);
}
diff --git
a/catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/converter/HiveTableConverter.java
b/catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/converter/HiveTableConverter.java
index 27100a6026..efe8f3bc58 100644
---
a/catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/converter/HiveTableConverter.java
+++
b/catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/converter/HiveTableConverter.java
@@ -240,7 +240,7 @@ public class HiveTableConverter {
}
}
- if (table.distribution() != null &&
!Distributions.NONE.equals(table.distribution())) {
+ if (table.distribution() != null &&
!Distributions.isNone(table.distribution())) {
strgDesc.setBucketCols(
Arrays.stream(table.distribution().expressions())
.map(t -> ((NamedReference.FieldReference) t).fieldName()[0])
diff --git
a/common/src/main/java/org/apache/gravitino/dto/util/DTOConverters.java
b/common/src/main/java/org/apache/gravitino/dto/util/DTOConverters.java
index 717d955681..c9d36b951e 100644
--- a/common/src/main/java/org/apache/gravitino/dto/util/DTOConverters.java
+++ b/common/src/main/java/org/apache/gravitino/dto/util/DTOConverters.java
@@ -368,7 +368,7 @@ public class DTOConverters {
* @return The distribution DTO.
*/
public static DistributionDTO toDTO(Distribution distribution) {
- if (Distributions.NONE.equals(distribution) || null == distribution) {
+ if (Distributions.isNone(distribution)) {
return DistributionDTO.NONE;
}
diff --git
a/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/hive/HiveMetadataAdapter.java
b/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/hive/HiveMetadataAdapter.java
index d997d9f4f1..6925829bd4 100644
---
a/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/hive/HiveMetadataAdapter.java
+++
b/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/hive/HiveMetadataAdapter.java
@@ -217,7 +217,7 @@ public class HiveMetadataAdapter extends
CatalogConnectorMetadataAdapter {
}
if (gravitinoTable.getDistribution() != null
- && !Distributions.NONE.equals(gravitinoTable.getDistribution())) {
+ && !Distributions.isNone(gravitinoTable.getDistribution())) {
properties.put(
HivePropertyMeta.HIVE_BUCKET_KEY,
Arrays.stream(gravitinoTable.getDistribution().expressions())
