From d5a386b2d9a7e43f2218dc35fbb10a460e17eba0 Mon Sep 17 00:00:00 2001
From: Michael Dowling <mtdowling@gmail.com>
Date: Fri, 25 Aug 2023 15:41:11 -0500
Subject: [PATCH] Improve node number equality

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
---
 .../amazon/smithy/model/node/NumberNode.java  | 47 ++++++++++++++++++-
 .../smithy/model/node/NumberNodeTest.java     | 42 +++++++++++++++++
 2 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/node/NumberNode.java b/smithy-model/src/main/java/software/amazon/smithy/model/node/NumberNode.java
index 10346dbd601..f8ec35925a6 100644
--- a/smithy-model/src/main/java/software/amazon/smithy/model/node/NumberNode.java
+++ b/smithy-model/src/main/java/software/amazon/smithy/model/node/NumberNode.java
@@ -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;
+        }
     }
 
     /**
@@ -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
diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/node/NumberNodeTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/node/NumberNodeTest.java
index 25d9e2ea163..910860574d3 100644
--- a/smithy-model/src/test/java/software/amazon/smithy/model/node/NumberNodeTest.java
+++ b/smithy-model/src/test/java/software/amazon/smithy/model/node/NumberNodeTest.java
@@ -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;
@@ -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));
+    }
 }