Skip to content

Commit

Permalink
Improve node number equality
Browse files Browse the repository at this point in the history
Attempts to use the most optimal comparisons of number nodes,
using .equals if both number values are of the same type, then the
string representation of each value, finally followed by comparing
the BigDecimal representation of each value using the string cache.
If a BigDecimal is needed, it is cached on the number node for future
checks.

Closes #1378
  • Loading branch information
mtdowling committed Aug 25, 2023
1 parent e10e6be commit d5a386b
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,16 @@ public final class NumberNode extends Node {

private final Number value;
private final String stringCache;
private volatile BigDecimal equality;

public NumberNode(Number value, SourceLocation sourceLocation) {
super(sourceLocation);
this.value = Objects.requireNonNull(value);
stringCache = value.toString();

if (value instanceof BigDecimal) {
equality = (BigDecimal) value;
}
}

/**
Expand Down Expand Up @@ -129,7 +134,47 @@ public boolean isZero() {

@Override
public boolean equals(Object other) {
return other instanceof NumberNode && stringCache.equals(((NumberNode) other).stringCache);
if (!(other instanceof NumberNode)) {
return false;
} else if (other == this) {
return true;
} else {
NumberNode otherNode = (NumberNode) other;

// This only works if both values are the same type.
if (value.equals(otherNode.value)) {
return true;
}

// Attempt a cheap check based on the string cache.
if (stringCache.equals(otherNode.stringCache)) {
return true;
}

// Convert both to BigDecimal and compare equality.
return getEquality().equals(otherNode.getEquality());
}
}

private BigDecimal getEquality() {
BigDecimal e = equality;

if (e == null) {
if (value instanceof Integer) {
e = BigDecimal.valueOf(value.intValue());
} else if (value instanceof Short) {
e = BigDecimal.valueOf(value.shortValue());
} else if (value instanceof Byte) {
e = BigDecimal.valueOf(value.byteValue());
} else if (value instanceof Long) {
e = BigDecimal.valueOf(value.longValue());
} else {
e = new BigDecimal(value.toString());
}
equality = e;
}

return e;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@

package software.amazon.smithy.model.node;

import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.instanceOf;
import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
Expand Down Expand Up @@ -241,4 +243,44 @@ public String toString() {
assertEquals(v, result, "Expected " + k + " to be " + v);
});
}

@Test
public void compareIntAndShort() {
Node left = Node.from(1);
Node right = Node.from((short) 1);

assertThat(left, equalTo(right));
}

@Test
public void compareShortAndFloat() {
Node left = Node.from((float) 1.0);
Node right = Node.from((short) 1);

assertThat(left, not(equalTo(right)));
}

@Test
public void compareByteAndFloat() {
Node left = Node.from((float) 1.0);
Node right = Node.from((byte) 1);

assertThat(left, not(equalTo(right)));
}

@Test
public void compareDoubleAndBigDecimal() {
Node left = Node.from(new BigDecimal("1.0e+10"));
Node right = Node.from(1.0e+10);

assertThat(left, equalTo(right));
}

@Test
public void compareDoubleAndFloat() {
Node left = Node.from((float) 1.0e+10);
Node right = Node.from((double) 1.0e+10);

assertThat(left, equalTo(right));
}
}

0 comments on commit d5a386b

Please sign in to comment.