diff --git a/core/src/main/java/org/opensearch/sql/data/model/ExprTupleValue.java b/core/src/main/java/org/opensearch/sql/data/model/ExprTupleValue.java index b92fdb51bf2..d2b8850d386 100644 --- a/core/src/main/java/org/opensearch/sql/data/model/ExprTupleValue.java +++ b/core/src/main/java/org/opensearch/sql/data/model/ExprTupleValue.java @@ -14,6 +14,7 @@ import lombok.RequiredArgsConstructor; import org.opensearch.sql.data.type.ExprCoreType; import org.opensearch.sql.data.type.ExprType; +import org.opensearch.sql.data.utils.ComparableLinkedHashMap; import org.opensearch.sql.storage.bindingtuple.BindingTuple; import org.opensearch.sql.storage.bindingtuple.LazyBindingTuple; @@ -44,7 +45,7 @@ public Object value() { @Override public Object valueForCalcite() { - LinkedHashMap resultMap = new LinkedHashMap<>(); + ComparableLinkedHashMap resultMap = new ComparableLinkedHashMap<>(); for (Entry entry : valueMap.entrySet()) { resultMap.put(entry.getKey(), entry.getValue().valueForCalcite()); } 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 new file mode 100644 index 00000000000..d74891ae547 --- /dev/null +++ b/core/src/main/java/org/opensearch/sql/data/utils/ComparableLinkedHashMap.java @@ -0,0 +1,62 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.data.utils; + +import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.Map; + +public class ComparableLinkedHashMap extends LinkedHashMap + implements Comparable> { + + public ComparableLinkedHashMap() { + super(); + } + + public ComparableLinkedHashMap(int initialCapacity) { + super(initialCapacity); + } + + public ComparableLinkedHashMap(int initialCapacity, float loadFactor) { + super(initialCapacity, loadFactor); + } + + @Override + public int compareTo(ComparableLinkedHashMap other) { + if (this.isEmpty() && other.isEmpty()) return 0; + if (this.isEmpty()) return -1; + if (other.isEmpty()) return 1; + Iterator> thisIterator = this.entrySet().iterator(); + Iterator> otherIterator = other.entrySet().iterator(); + return compareRecursive(thisIterator, otherIterator); + } + + private int compareRecursive( + Iterator> thisIterator, Iterator> otherIterator) { + boolean thisHasNext = thisIterator.hasNext(); + boolean otherHasNext = otherIterator.hasNext(); + if (!thisHasNext && !otherHasNext) return 0; + if (!thisHasNext) return -1; + if (!otherHasNext) return 1; + + V thisValue = thisIterator.next().getValue(); + V otherValue = otherIterator.next().getValue(); + int comparison = compareValues(thisValue, otherValue); + if (comparison != 0) return comparison; + return compareRecursive(thisIterator, otherIterator); + } + + @SuppressWarnings("unchecked") + private int compareValues(V value1, V value2) { + if (value1 == null && value2 == null) return 0; + if (value1 == null) return -1; + if (value2 == null) return 1; + if (value1 instanceof Comparable) { + return ((Comparable) value1).compareTo(value2); + } + return value1.toString().compareTo(value2.toString()); + } +} diff --git a/core/src/test/java/org/opensearch/sql/utils/ComparableLinkedHashMapTest.java b/core/src/test/java/org/opensearch/sql/utils/ComparableLinkedHashMapTest.java new file mode 100644 index 00000000000..4797a9a76b8 --- /dev/null +++ b/core/src/test/java/org/opensearch/sql/utils/ComparableLinkedHashMapTest.java @@ -0,0 +1,258 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.utils; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Comparator; +import java.util.Iterator; +import java.util.TreeSet; +import org.junit.jupiter.api.Test; +import org.opensearch.sql.data.utils.ComparableLinkedHashMap; + +public class ComparableLinkedHashMapTest { + + @Test + public void testEmptyMaps() { + ComparableLinkedHashMap map1 = new ComparableLinkedHashMap<>(); + ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); + + assertEquals(0, map1.compareTo(map2)); + } + + @Test + public void testOneEmptyMap() { + ComparableLinkedHashMap map1 = new ComparableLinkedHashMap<>(); + ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); + map2.put("a", 1); + + assertTrue(map1.compareTo(map2) < 0); + assertTrue(map2.compareTo(map1) > 0); + } + + @Test + public void testEqualMaps() { + ComparableLinkedHashMap map1 = new ComparableLinkedHashMap<>(); + map1.put("a", 1); + map1.put("b", 2); + + ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); + map2.put("c", 1); + map2.put("d", 2); + + assertEquals(0, map1.compareTo(map2)); + } + + @Test + public void testDifferentFirstValue() { + ComparableLinkedHashMap map1 = new ComparableLinkedHashMap<>(); + map1.put("a", 1); + map1.put("b", 2); + + ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); + map2.put("c", 3); + map2.put("d", 2); + + assertTrue(map1.compareTo(map2) < 0); + assertTrue(map2.compareTo(map1) > 0); + } + + @Test + public void testDifferentLaterValue() { + ComparableLinkedHashMap map1 = new ComparableLinkedHashMap<>(); + map1.put("a", 1); + map1.put("b", 2); + map1.put("c", 3); + + ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); + map2.put("d", 1); + map2.put("e", 3); + map2.put("f", 3); + + assertTrue(map1.compareTo(map2) < 0); + assertTrue(map2.compareTo(map1) > 0); + } + + @Test + public void testDifferentSizes() { + ComparableLinkedHashMap map1 = new ComparableLinkedHashMap<>(); + map1.put("a", 1); + map1.put("b", 2); + + ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); + map2.put("c", 1); + + assertTrue(map1.compareTo(map2) > 0); + assertTrue(map2.compareTo(map1) < 0); + } + + @Test + public void testNullValues() { + ComparableLinkedHashMap map1 = new ComparableLinkedHashMap<>(); + map1.put("a", null); + map1.put("b", 2); + + ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); + map2.put("c", 1); + map2.put("d", 2); + + assertTrue(map1.compareTo(map2) < 0); + assertTrue(map2.compareTo(map1) > 0); + + ComparableLinkedHashMap map3 = new ComparableLinkedHashMap<>(); + map3.put("e", null); + map3.put("f", 2); + + assertEquals(0, map1.compareTo(map3)); + } + + @Test + public void testCustomObjects() { + class Person { + String name; + + Person(String name) { + this.name = name; + } + + @Override + public String toString() { + return name; + } + } + + ComparableLinkedHashMap map1 = new ComparableLinkedHashMap<>(); + map1.put("a", new Person("Alice")); + map1.put("b", new Person("Bob")); + + ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); + map2.put("c", new Person("Alice")); + map2.put("d", new Person("Charlie")); + + assertTrue(map1.compareTo(map2) < 0); + assertTrue(map2.compareTo(map1) > 0); + } + + @Test + public void testMixedTypes() { + ComparableLinkedHashMap map1 = new ComparableLinkedHashMap<>(); + map1.put("a", 1); + map1.put("b", "test"); + + ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); + map2.put("c", 1); + map2.put("d", "test"); + + assertEquals(0, map1.compareTo(map2)); + + ComparableLinkedHashMap map3 = new ComparableLinkedHashMap<>(); + map3.put("e", 1); + map3.put("f", "test2"); + + assertTrue(map1.compareTo(map3) < 0); + assertTrue(map3.compareTo(map1) > 0); + } + + @Test + public void testWithTreeSet() { + TreeSet> set = new TreeSet<>(); + + ComparableLinkedHashMap map1 = new ComparableLinkedHashMap<>(); + map1.put("a", 1); + map1.put("b", 2); + + ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); + map2.put("c", 1); + map2.put("d", 3); + + ComparableLinkedHashMap map3 = new ComparableLinkedHashMap<>(); + map3.put("e", 0); + map3.put("f", 4); + + set.add(map2); + set.add(map1); + set.add(map3); + + Iterator> iterator = set.iterator(); + ComparableLinkedHashMap first = iterator.next(); + 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(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(3, set.size()); + } + + @Test + public void testWithComparator() { + Comparator> comparator = + ComparableLinkedHashMap::compareTo; + + ComparableLinkedHashMap map1 = new ComparableLinkedHashMap<>(); + map1.put("a", 5); + + ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); + map2.put("b", 3); + + assertTrue(comparator.compare(map1, map2) > 0); + assertTrue(comparator.compare(map2, map1) < 0); + } + + @Test + public void testEqualValuesDifferentKeys() { + ComparableLinkedHashMap map1 = new ComparableLinkedHashMap<>(); + map1.put("a", 1); + map1.put("b", 2); + map1.put("c", 3); + + ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); + map2.put("d", 1); + map2.put("e", 2); + map2.put("f", 3); + + assertEquals(0, map1.compareTo(map2)); + } + + @Test + public void testDifferentOrder() { + ComparableLinkedHashMap map1 = new ComparableLinkedHashMap<>(); + map1.put("a", 1); + map1.put("b", 2); + + ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); + map2.put("c", 2); + map2.put("d", 1); + + assertTrue(map1.compareTo(map2) < 0); + assertTrue(map2.compareTo(map1) > 0); + } + + @Test + public void testLargeMaps() { + ComparableLinkedHashMap map1 = new ComparableLinkedHashMap<>(); + for (int i = 0; i < 100; i++) { + map1.put("key" + i, i); + } + + ComparableLinkedHashMap map2 = new ComparableLinkedHashMap<>(); + for (int i = 0; i < 100; i++) { + map2.put("differentKey" + i, i); + } + + assertEquals(0, map1.compareTo(map2)); + + map2.put("differentKey99", 100); + assertTrue(map1.compareTo(map2) < 0); + } +} diff --git a/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4339.yml b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4339.yml new file mode 100644 index 00000000000..c751313987b --- /dev/null +++ b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4339.yml @@ -0,0 +1,80 @@ +setup: + - do: + indices.create: + index: test + body: + settings: + number_of_shards: 1 + number_of_replicas: 0 + mappings: + properties: + log: + properties: + url: + properties: + message: + type: text + fields: + keyword: + type: keyword + ignore_above: 256 + time: + type: long + message_alias: + type: alias + path: log.url.message + time_alias: + type: alias + path: log.url.time + + - do: + query.settings: + body: + transient: + plugins.calcite.enabled : true + +--- +teardown: + - do: + query.settings: + body: + transient: + plugins.calcite.enabled : false + +--- +"dedup struct field": + - skip: + features: + - headers + - do: + bulk: + index: test + refresh: true + body: + - '{"index": {}}' + - '{"log": {"url": {"message": "/e2e/h/zap", "time": 1} } }' + - '{"index": {}}' + - '{"log": {"url": {"message": "/e2e/h/zap", "time": 1} } }' + - '{"index": {}}' + - '{"log": {"url": {"message": "/e2e/h/zap", "time": 2} } }' + - '{"index": {}}' + - '{"log": {"url": {"message": "/e2e/h/zap", "time": 2} } }' + - '{"index": {}}' + - '{"log": {"url": {"message": "/e2e/h/aap", "time": 1} } }' + - '{"index": {}}' + - '{"log": {"url": {"message": "/e2e/h/aap", "time": 1} } }' + - '{"index": {}}' + - '{"log": {"url": {"time": 1} } }' + - '{"index": {}}' + - '{"log": {"url": {"time": 1} } }' + - '{"index": {}}' + - '{"log": {"url": {"time": 2} } }' + - '{"index": {}}' + - '{"log": {"url": {"time": 2} } }' + - do: + headers: + Content-Type: 'application/json' + ppl: + body: + query: 'source=test | dedup log' + - match: {"total": 5}