diff --git a/core/src/main/java/org/opensearch/sql/data/utils/ComparableLinkedHashMap.java b/core/src/main/java/org/opensearch/sql/data/utils/ComparableLinkedHashMap.java index d74891ae547..31a849dec9b 100644 --- a/core/src/main/java/org/opensearch/sql/data/utils/ComparableLinkedHashMap.java +++ b/core/src/main/java/org/opensearch/sql/data/utils/ComparableLinkedHashMap.java @@ -42,15 +42,29 @@ private int compareRecursive( if (!thisHasNext) return -1; if (!otherHasNext) return 1; - V thisValue = thisIterator.next().getValue(); - V otherValue = otherIterator.next().getValue(); - int comparison = compareValues(thisValue, otherValue); + Map.Entry thisEntry = thisIterator.next(); + Map.Entry otherEntry = otherIterator.next(); + K thisKey = thisEntry.getKey(); + K otherKey = otherEntry.getKey(); + V thisValue = thisEntry.getValue(); + V otherValue = otherEntry.getValue(); + int comparison = compareKV(thisKey, otherKey, thisValue, otherValue); if (comparison != 0) return comparison; return compareRecursive(thisIterator, otherIterator); } @SuppressWarnings("unchecked") - private int compareValues(V value1, V value2) { + private int compareKV(K key1, K key2, V value1, V value2) { + int keyCompare; + if (key1 instanceof Comparable) { + keyCompare = ((Comparable) key1).compareTo(key2); + } else { + keyCompare = key1.toString().compareTo(key2.toString()); + } + if (keyCompare != 0) { + return keyCompare; + } + if (value1 == null && value2 == null) return 0; if (value1 == null) return -1; if (value2 == null) return 1; diff --git a/core/src/test/java/org/opensearch/sql/utils/ComparableLinkedHashMapTest.java b/core/src/test/java/org/opensearch/sql/utils/ComparableLinkedHashMapTest.java index 4797a9a76b8..5cf5cad6bd1 100644 --- a/core/src/test/java/org/opensearch/sql/utils/ComparableLinkedHashMapTest.java +++ b/core/src/test/java/org/opensearch/sql/utils/ComparableLinkedHashMapTest.java @@ -34,6 +34,17 @@ public void testOneEmptyMap() { assertTrue(map2.compareTo(map1) > 0); } + @Test + public void testDifferentKeys() { + ComparableLinkedHashMap map1 = new ComparableLinkedHashMap<>(); + map1.put("a", 1); + + ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); + map2.put("b", 1); + + assertTrue(map1.compareTo(map2) < 0); + } + @Test public void testEqualMaps() { ComparableLinkedHashMap map1 = new ComparableLinkedHashMap<>(); @@ -41,8 +52,8 @@ public void testEqualMaps() { map1.put("b", 2); ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); - map2.put("c", 1); - map2.put("d", 2); + map2.put("a", 1); + map2.put("b", 2); assertEquals(0, map1.compareTo(map2)); } @@ -54,8 +65,8 @@ public void testDifferentFirstValue() { map1.put("b", 2); ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); - map2.put("c", 3); - map2.put("d", 2); + map2.put("a", 3); + map2.put("b", 2); assertTrue(map1.compareTo(map2) < 0); assertTrue(map2.compareTo(map1) > 0); @@ -69,9 +80,9 @@ public void testDifferentLaterValue() { map1.put("c", 3); ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); - map2.put("d", 1); - map2.put("e", 3); - map2.put("f", 3); + map2.put("a", 1); + map2.put("b", 3); + map2.put("c", 3); assertTrue(map1.compareTo(map2) < 0); assertTrue(map2.compareTo(map1) > 0); @@ -84,7 +95,7 @@ public void testDifferentSizes() { map1.put("b", 2); ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); - map2.put("c", 1); + map2.put("a", 1); assertTrue(map1.compareTo(map2) > 0); assertTrue(map2.compareTo(map1) < 0); @@ -97,15 +108,15 @@ public void testNullValues() { map1.put("b", 2); ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); - map2.put("c", 1); - map2.put("d", 2); + map2.put("a", 1); + map2.put("b", 2); assertTrue(map1.compareTo(map2) < 0); assertTrue(map2.compareTo(map1) > 0); ComparableLinkedHashMap map3 = new ComparableLinkedHashMap<>(); - map3.put("e", null); - map3.put("f", 2); + map3.put("a", null); + map3.put("b", 2); assertEquals(0, map1.compareTo(map3)); } @@ -130,8 +141,8 @@ public String toString() { map1.put("b", new Person("Bob")); ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); - map2.put("c", new Person("Alice")); - map2.put("d", new Person("Charlie")); + map2.put("a", new Person("Alice")); + map2.put("b", new Person("Charlie")); assertTrue(map1.compareTo(map2) < 0); assertTrue(map2.compareTo(map1) > 0); @@ -144,14 +155,14 @@ public void testMixedTypes() { map1.put("b", "test"); ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); - map2.put("c", 1); - map2.put("d", "test"); + map2.put("a", 1); + map2.put("b", "test"); assertEquals(0, map1.compareTo(map2)); ComparableLinkedHashMap map3 = new ComparableLinkedHashMap<>(); - map3.put("e", 1); - map3.put("f", "test2"); + map3.put("a", 1); + map3.put("b", "test2"); assertTrue(map1.compareTo(map3) < 0); assertTrue(map3.compareTo(map1) > 0); @@ -166,12 +177,12 @@ public void testWithTreeSet() { map1.put("b", 2); ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); - map2.put("c", 1); - map2.put("d", 3); + map2.put("a", 1); + map2.put("b", 3); ComparableLinkedHashMap map3 = new ComparableLinkedHashMap<>(); - map3.put("e", 0); - map3.put("f", 4); + map3.put("a", 0); + map3.put("b", 4); set.add(map2); set.add(map1); @@ -182,14 +193,14 @@ public void testWithTreeSet() { ComparableLinkedHashMap second = iterator.next(); ComparableLinkedHashMap third = iterator.next(); - assertEquals(Integer.valueOf(0), first.get("e")); - assertEquals(Integer.valueOf(4), first.get("f")); + assertEquals(Integer.valueOf(0), first.get("a")); + assertEquals(Integer.valueOf(4), first.get("b")); assertEquals(Integer.valueOf(1), second.get("a")); assertEquals(Integer.valueOf(2), second.get("b")); - assertEquals(Integer.valueOf(1), third.get("c")); - assertEquals(Integer.valueOf(3), third.get("d")); + assertEquals(Integer.valueOf(1), third.get("a")); + assertEquals(Integer.valueOf(3), third.get("b")); assertEquals(3, set.size()); } @@ -203,7 +214,7 @@ public void testWithComparator() { map1.put("a", 5); ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); - map2.put("b", 3); + map2.put("a", 3); assertTrue(comparator.compare(map1, map2) > 0); assertTrue(comparator.compare(map2, map1) < 0); @@ -221,7 +232,7 @@ public void testEqualValuesDifferentKeys() { map2.put("e", 2); map2.put("f", 3); - assertEquals(0, map1.compareTo(map2)); + assertTrue(map1.compareTo(map2) < 0); } @Test @@ -231,8 +242,8 @@ public void testDifferentOrder() { map1.put("b", 2); ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); - map2.put("c", 2); - map2.put("d", 1); + map2.put("a", 2); + map2.put("b", 1); assertTrue(map1.compareTo(map2) < 0); assertTrue(map2.compareTo(map1) > 0); @@ -247,12 +258,12 @@ public void testLargeMaps() { ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); for (int i = 0; i < 100; i++) { - map2.put("differentKey" + i, i); + map2.put("key" + i, i); } assertEquals(0, map1.compareTo(map2)); - map2.put("differentKey99", 100); + map2.put("key", 100); assertTrue(map1.compareTo(map2) < 0); } }