Copilot commented on code in PR #13233:
URL: https://github.com/apache/gravitino/pull/13233#discussion_r4026176046
##########
api/src/main/java/org/apache/gravitino/rel/expressions/distributions/Distributions.java:
##########
@@ -36,6 +36,20 @@ 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. 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 not null and represents the NONE
distribution.
+ */
+ public static boolean isNone(Distribution distribution) {
Review Comment:
The parameter is explicitly nullable in the Javadoc and implementation, but
the public API does not annotate it with `@Nullable`. This prevents
nullness-aware consumers from seeing the actual contract; nullable public
parameters are annotated elsewhere, for example
`api/src/main/java/org/apache/gravitino/rel/ViewCatalog.java:96-100`.
##########
api/src/test/java/org/apache/gravitino/rel/expressions/distributions/TestDistributions.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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 {
+
+ 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)));
Review Comment:
This test claims to cover cross-representation NONE, but it uses an
anonymous `Distribution`, not `DistributionDTO.NONE`, which is the
representation this helper must recognize at the common conversion call sites.
Exercise `DistributionDTO.NONE` here (and keep the anonymous case if desired)
so a DTO-specific regression in `expressions()` or representation conversion is
caught.
##########
api/src/main/java/org/apache/gravitino/rel/expressions/distributions/Distribution.java:
##########
@@ -47,19 +46,10 @@ 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());
- }
+ // Note: this interface intentionally does NOT define a `boolean
equals(Distribution)` overload.
+ // Such an overload never overrides Object.equals, so it is invisible to
HashSet/HashMap and any
+ // code comparing through Object references, which makes structural equality
silently
+ // dispatch-dependent. Implementations must override
equals(Object)/hashCode() themselves
+ // (DistributionImpl and DistributionDTO do). Use Distributions.isNone to
test for the NONE
+ // distribution across representations.
Review Comment:
This requirement is in a `//` comment, so it is omitted from generated
Javadocs. Removing the public overload changes the implementation contract, and
external implementers need to see that they must provide
`equals(Object)`/`hashCode`; move this text into the interface's `/** ... */`
Javadoc (for example by expanding the type Javadoc).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
