This is an automated email from the ASF dual-hosted git repository.
jerryshao 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 1a888c92bd [#13228] fix(api): structurally compare array values in
LiteralImpl equality (#13229)
1a888c92bd is described below
commit 1a888c92bdbdbb1c29728b3847282cdbe5adee0e
Author: YangJie <[email protected]>
AuthorDate: Sun Sep 20 08:22:32 2026 -0400
[#13228] fix(api): structurally compare array values in LiteralImpl
equality (#13229)
### What changes were proposed in this pull request?
`LiteralImpl` now compares `byte[]` values with `Arrays.equals` and
`Object[]` values with `Arrays.deepEquals`, and uses matching structural
`hashCode`s. Non-array semantics are unchanged.
### Why are the changes needed?
`equals` fell back to `value.toString()` and `Objects.equals`, so binary
literals with equal content were never equal and binary
identity-partition dedup never matched.
Fix: #13228
### Does this PR introduce _any_ user-facing change?
No API change. Binary (`byte[]`) literals with equal content are now
equal and hash consistently, so binary identity-partition deduplication
works. Non-array literal semantics are unchanged.
### How was this patch tested?
Added `TestLiteral.testBinaryLiteralsWithEqualContentAreEqual`, which
pins that two binary literals with equal content are equal and hash
consistently; it fails on the pre-fix tree and passes after the fix.
---
.../rel/expressions/literals/Literals.java | 24 ++++++++-
.../java/org/apache/gravitino/rel/TestLiteral.java | 63 ++++++++++++++++++++++
2 files changed, 86 insertions(+), 1 deletion(-)
diff --git
a/api/src/main/java/org/apache/gravitino/rel/expressions/literals/Literals.java
b/api/src/main/java/org/apache/gravitino/rel/expressions/literals/Literals.java
index 7d9d237fea..f5b99f14c9 100644
---
a/api/src/main/java/org/apache/gravitino/rel/expressions/literals/Literals.java
+++
b/api/src/main/java/org/apache/gravitino/rel/expressions/literals/Literals.java
@@ -21,6 +21,7 @@ package org.apache.gravitino.rel.expressions.literals;
import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.LocalTime;
+import java.util.Arrays;
import java.util.Objects;
import org.apache.gravitino.rel.types.Decimal;
import org.apache.gravitino.rel.types.Type;
@@ -288,13 +289,34 @@ public class Literals {
if (value == null || literal.value == null) {
return Objects.equals(value, literal.value);
}
- // Now, it's safe to compare using toString() since neither value is null
+ // Arrays need structural comparison: Objects.equals is reference
equality for arrays and
+ // the toString() fallback below renders identity hashes, so
equal-content binary and array
+ // literals would never compare equal.
+ if (value instanceof byte[] && literal.value instanceof byte[]) {
+ return Arrays.equals((byte[]) value, (byte[]) literal.value);
+ }
+ if (value instanceof Object[] && literal.value instanceof Object[]) {
+ return Arrays.deepEquals((Object[]) value, (Object[]) literal.value);
+ }
+ // Exactly one side is an array here (or the array kinds differ), so the
values are not
+ // equal. Reaching the toString() fallback below only when neither value
is an array keeps
+ // an array's identity-based toString from spuriously matching a
non-array value.
+ if (value.getClass().isArray() || literal.value.getClass().isArray()) {
+ return false;
+ }
+ // Now, it's safe to compare using toString() since neither value is
null nor an array
return Objects.equals(value, literal.value)
|| value.toString().equals(literal.value.toString());
}
@Override
public int hashCode() {
+ if (value instanceof byte[]) {
+ return Objects.hash(dataType, Arrays.hashCode((byte[]) value));
+ }
+ if (value instanceof Object[]) {
+ return Objects.hash(dataType, Arrays.deepHashCode((Object[]) value));
+ }
return Objects.hash(dataType, value != null ? value.toString() : null);
}
diff --git a/api/src/test/java/org/apache/gravitino/rel/TestLiteral.java
b/api/src/test/java/org/apache/gravitino/rel/TestLiteral.java
index cd71a2f352..a57602f81f 100644
--- a/api/src/test/java/org/apache/gravitino/rel/TestLiteral.java
+++ b/api/src/test/java/org/apache/gravitino/rel/TestLiteral.java
@@ -39,6 +39,9 @@ import java.math.BigDecimal;
import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.LocalTime;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
import org.apache.gravitino.rel.expressions.literals.Literal;
import org.apache.gravitino.rel.expressions.literals.Literals;
import org.apache.gravitino.rel.types.Decimal;
@@ -116,4 +119,64 @@ public class TestLiteral {
Assertions.assertEquals(Decimal.of(new BigDecimal("0.00")),
literal.value());
Assertions.assertEquals(Types.DecimalType.of(2, 2), literal.dataType());
}
+
+ @Test
+ public void testBinaryLiteralsWithEqualContentAreEqual() {
+ Literal<?> first = Literals.of(new byte[] {1, 2, 3},
Types.BinaryType.get());
+ Literal<?> second = Literals.of(new byte[] {1, 2, 3},
Types.BinaryType.get());
+
+ // Before the fix, LiteralImpl.equals fell back to value.toString(), so
structurally identical
+ // binary literals were never equal (Objects.equals on arrays is reference
equality and the
+ // toString fallback renders identity hashes).
+ Assertions.assertEquals(first, second);
+ Assertions.assertEquals(first.hashCode(), second.hashCode());
+ Assertions.assertNotEquals(first, Literals.of(new byte[] {1, 2},
Types.BinaryType.get()));
+
+ Set<Literal<?>> literals = new HashSet<>(Collections.singletonList(first));
+ Assertions.assertTrue(
+ literals.contains(Literals.of(new byte[] {1, 2, 3},
Types.BinaryType.get())));
+ }
+
+ @Test
+ public void testArrayLiteralsWithEqualContentAreEqual() {
+ Types.ListType listType = Types.ListType.of(Types.IntegerType.get(),
false);
+ Literal<?> first = Literals.of(new Object[] {1, 2, 3}, listType);
+ Literal<?> second = Literals.of(new Object[] {1, 2, 3}, listType);
+
+ // Object[] values need Arrays.deepEquals: Objects.equals is reference
equality for arrays and
+ // the toString() fallback renders identity hashes, so equal-content array
literals must match.
+ Assertions.assertEquals(first, second);
+ Assertions.assertEquals(first.hashCode(), second.hashCode());
+ Assertions.assertNotEquals(first, Literals.of(new Object[] {1, 2},
listType));
+
+ Set<Literal<?>> literals = new HashSet<>(Collections.singletonList(first));
+ Assertions.assertTrue(literals.contains(Literals.of(new Object[] {1, 2,
3}, listType)));
+ }
+
+ @Test
+ public void testNestedArrayLiteralsUseDeepEquality() {
+ Types.ListType listType =
+ Types.ListType.of(Types.ListType.of(Types.IntegerType.get(), false),
false);
+ Literal<?> first = Literals.of(new Object[] {new Object[] {1, 2}, new
Object[] {3}}, listType);
+ Literal<?> second = Literals.of(new Object[] {new Object[] {1, 2}, new
Object[] {3}}, listType);
+
+ // Arrays.deepEquals/deepHashCode recurse into nested arrays; a shallow
Arrays.equals would
+ // compare the inner arrays by reference and report these literals as
unequal.
+ Assertions.assertEquals(first, second);
+ Assertions.assertEquals(first.hashCode(), second.hashCode());
+ Assertions.assertNotEquals(
+ first, Literals.of(new Object[] {new Object[] {1, 2}, new Object[]
{4}}, listType));
+ }
+
+ @Test
+ public void testArrayLiteralNeverEqualsNonArrayLiteral() {
+ // Even when they share a dataType, an array value must not match a
non-array value through the
+ // toString() fallback, keeping equals() consistent with the array-aware
hashCode.
+ Types.BinaryType binary = Types.BinaryType.get();
+ Literal<?> arrayLiteral = Literals.of(new byte[] {1, 2, 3}, binary);
+ Literal<?> nonArrayLiteral = Literals.of("1, 2, 3", binary);
+
+ Assertions.assertNotEquals(arrayLiteral, nonArrayLiteral);
+ Assertions.assertNotEquals(nonArrayLiteral, arrayLiteral);
+ }
}
