Address PR feedback. Revert ExprCollectionValue and `ExprTupleValue…

…` comparison.

Signed-off-by: Yury-Fridlyand <[email protected]>

core/src/main/java/org/opensearch/sql/data/model/ExprCollectionValue.java

@@ -73,7 +73,7 @@ public boolean equal(ExprValue o) {
    */
   @Override
   public int compare(ExprValue other) {
-    return equal(other) ? 0 : 1;
+    return Integer.compare(valueList.size(), other.collectionValue().size());
   }
 
   @Override

core/src/main/java/org/opensearch/sql/data/model/ExprDateValue.java

@@ -11,7 +11,6 @@
 import java.time.LocalDate;
 import java.time.LocalDateTime;
 import java.time.LocalTime;
-import java.time.ZoneId;
 import java.time.ZonedDateTime;
 import java.time.format.DateTimeFormatter;
 import java.time.format.DateTimeParseException;
@@ -67,7 +66,7 @@ public LocalDateTime datetimeValue() {
 
   @Override
   public Instant timestampValue() {
-    return ZonedDateTime.of(date, timeValue(), ZoneId.of("UTC")).toInstant();
+    return ZonedDateTime.of(date, timeValue(), ExprTimestampValue.ZONE).toInstant();
   }
 
   @Override

core/src/main/java/org/opensearch/sql/data/model/ExprDatetimeValue.java

@@ -11,7 +11,6 @@
 import java.time.LocalDate;
 import java.time.LocalDateTime;
 import java.time.LocalTime;
-import java.time.ZoneId;
 import java.time.ZonedDateTime;
 import java.time.format.DateTimeFormatter;
 import java.time.format.DateTimeFormatterBuilder;
@@ -71,7 +70,7 @@ public LocalTime timeValue() {
 
   @Override
   public Instant timestampValue() {
-    return ZonedDateTime.of(datetime, ZoneId.of("UTC")).toInstant();
+    return ZonedDateTime.of(datetime, ExprTimestampValue.ZONE).toInstant();
   }
 
   @Override

core/src/main/java/org/opensearch/sql/data/model/ExprTimeValue.java

@@ -12,7 +12,6 @@
 import java.time.LocalDate;
 import java.time.LocalDateTime;
 import java.time.LocalTime;
-import java.time.ZoneId;
 import java.time.ZonedDateTime;
 import java.time.format.DateTimeFormatter;
 import java.time.format.DateTimeParseException;
@@ -68,7 +67,7 @@ public LocalDateTime datetimeValue() {
 
   @Override
   public Instant timestampValue() {
-    return ZonedDateTime.of(dateValue(), timeValue(), ZoneId.of("UTC")).toInstant();
+    return ZonedDateTime.of(dateValue(), timeValue(), ExprTimestampValue.ZONE).toInstant();
   }
 
   @Override

core/src/main/java/org/opensearch/sql/data/model/ExprTimestampValue.java

@@ -30,7 +30,7 @@ public class ExprTimestampValue extends AbstractExprValue {
   /**
    * todo. only support UTC now.
    */
-  private static final ZoneId ZONE = ZoneId.of("UTC");
+  public static final ZoneId ZONE = ZoneId.of("UTC");
 
   private final Instant timestamp;
 

core/src/main/java/org/opensearch/sql/data/model/ExprTupleValue.java

@@ -96,7 +96,7 @@ public boolean equal(ExprValue o) {
    */
   @Override
   public int compare(ExprValue other) {
-    return equal(other) ? 0 : 1;
+    return Integer.compare(valueMap.size(), other.tupleValue().size());
   }
 
   @Override

core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java

@@ -98,7 +98,7 @@ public static ExprValue tupleValue(Map<String, Object> map) {
    */
   public static ExprValue collectionValue(List<Object> list) {
     List<ExprValue> valueList = new ArrayList<>();
-    list.forEach(o -> valueList.add(o instanceof ExprValue ? (ExprValue) o : fromObjectValue(o)));
+    list.forEach(o -> valueList.add(fromObjectValue(o)));
     return new ExprCollectionValue(valueList);
   }
 
@@ -145,8 +145,6 @@ public static ExprValue fromObjectValue(Object o) {
       return timeValue((LocalTime) o);
     } else if (o instanceof Instant) {
       return timestampValue((Instant) o);
-    } else if (o instanceof TemporalAmount) {
-      return intervalValue((TemporalAmount) o);
     } else {
       throw new ExpressionEvaluationException("unsupported object " + o.getClass());
     }

core/src/main/java/org/opensearch/sql/expression/operator/predicate/BinaryPredicateOperator.java

@@ -247,15 +247,11 @@ private static ExprValue lookupTableFunction(ExprValue arg1, ExprValue arg2,
   }
 
   private static List<Pair<ExprCoreType, ExprCoreType>> permuteTemporalTypesByPairs() {
-    var res = new ArrayList<Pair<ExprCoreType, ExprCoreType>>();
-    var datatypes = List.of(TIME, DATE, DATETIME, TIMESTAMP);
-    datatypes.forEach(left -> {
-      datatypes.forEach(right -> {
-        if (left != right) {
-          res.add(Pair.of(left, right));
-        }
-      });
-    });
-    return res;
+    return List.of(
+        Pair.of(DATE, TIME), Pair.of(DATE, DATETIME), Pair.of(DATE, TIMESTAMP),
+        Pair.of(TIME, DATE), Pair.of(TIME, DATETIME), Pair.of(TIME, TIMESTAMP),
+        Pair.of(DATETIME, TIME), Pair.of(DATETIME, DATE), Pair.of(DATETIME, TIMESTAMP),
+        Pair.of(TIMESTAMP, TIME), Pair.of(TIMESTAMP, DATE), Pair.of(TIMESTAMP, DATETIME)
+    );
   }
 }

core/src/test/java/org/opensearch/sql/data/model/DateTimeValueTest.java

@@ -15,7 +15,6 @@
 import java.time.LocalDate;
 import java.time.LocalDateTime;
 import java.time.LocalTime;
-import java.time.ZoneId;
 import java.time.ZonedDateTime;
 import org.junit.jupiter.api.Test;
 import org.opensearch.sql.exception.ExpressionEvaluationException;
@@ -34,7 +33,7 @@ public void timeValueInterfaceTest() {
     assertEquals(LocalDate.now(), timeValue.dateValue());
     assertEquals(LocalDate.now().atTime(1, 1, 1), timeValue.datetimeValue());
     assertEquals(ZonedDateTime.of(LocalTime.parse("01:01:01").atDate(LocalDate.now()),
-        ZoneId.of("UTC")).toInstant(), timeValue.timestampValue());
+        ExprTimestampValue.ZONE).toInstant(), timeValue.timestampValue());
     assertEquals("01:01:01", timeValue.value());
     assertEquals("TIME '01:01:01'", timeValue.toString());
     assertThrows(ExpressionEvaluationException.class, () -> integerValue(1).timeValue(),
@@ -47,7 +46,7 @@ public void timestampValueInterfaceTest() {
 
     assertEquals(TIMESTAMP, timestampValue.type());
     assertEquals(ZonedDateTime.of(LocalDateTime.parse("2020-07-07T01:01:01"),
-        ZoneId.of("UTC")).toInstant(), timestampValue.timestampValue());
+        ExprTimestampValue.ZONE).toInstant(), timestampValue.timestampValue());
     assertEquals("2020-07-07 01:01:01", timestampValue.value());
     assertEquals("TIMESTAMP '2020-07-07 01:01:01'", timestampValue.toString());
     assertEquals(LocalDate.parse("2020-07-07"), timestampValue.dateValue());
@@ -65,7 +64,7 @@ public void dateValueInterfaceTest() {
     assertEquals(LocalTime.parse("00:00:00"), dateValue.timeValue());
     assertEquals(LocalDateTime.parse("2012-07-07T00:00:00"), dateValue.datetimeValue());
     assertEquals(ZonedDateTime.of(LocalDateTime.parse("2012-07-07T00:00:00"),
-        ZoneId.of("UTC")).toInstant(), dateValue.timestampValue());
+        ExprTimestampValue.ZONE).toInstant(), dateValue.timestampValue());
     ExpressionEvaluationException exception =
         assertThrows(ExpressionEvaluationException.class, () -> integerValue(1).dateValue());
     assertEquals("invalid to get dateValue from value of type INTEGER",
@@ -80,7 +79,7 @@ public void datetimeValueInterfaceTest() {
     assertEquals(LocalDate.parse("2020-08-17"), datetimeValue.dateValue());
     assertEquals(LocalTime.parse("19:44:00"), datetimeValue.timeValue());
     assertEquals(ZonedDateTime.of(LocalDateTime.parse("2020-08-17T19:44:00"),
-        ZoneId.of("UTC")).toInstant(), datetimeValue.timestampValue());
+        ExprTimestampValue.ZONE).toInstant(), datetimeValue.timestampValue());
     assertEquals("DATETIME '2020-08-17 19:44:00'", datetimeValue.toString());
     assertThrows(ExpressionEvaluationException.class, () -> integerValue(1).datetimeValue(),
         "invalid to get datetimeValue from value of type INTEGER");

core/src/test/java/org/opensearch/sql/data/model/ExprCollectionValueTest.java

@@ -8,14 +8,14 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
-import static org.opensearch.sql.data.type.ExprCoreType.ARRAY;
 import static org.opensearch.sql.utils.ComparisonUtil.compare;
 
 import com.google.common.collect.ImmutableList;
 import java.util.Arrays;
-import java.util.List;
 import org.junit.jupiter.api.Test;
+import org.opensearch.sql.exception.ExpressionEvaluationException;
 
 public class ExprCollectionValueTest {
   @Test
@@ -47,23 +47,9 @@ public void compare_collection_with_int_object() {
 
   @Test
   public void comparabilityTest() {
-    ExprValue value1 = ExprValueUtils.collectionValue(Arrays.asList(0, 1));
-    ExprValue value2 = ExprValueUtils.collectionValue(ImmutableList.of(1, 2));
-    assertEquals(0, compare(value1, value1));
-    assertEquals(0, compare(value2, value2));
-    assertEquals(1, compare(value1, value2));
-    assertEquals(1, compare(value2, value1));
-  }
-
-  @Test
-  public void value() {
-    ExprValue value = ExprValueUtils.collectionValue(List.of(42));
-    assertEquals(List.of(42), value.value());
-  }
-
-  @Test
-  public void type() {
-    ExprValue value = ExprValueUtils.collectionValue(List.of());
-    assertEquals(ARRAY, value.type());
+    ExprValue collectionValue = ExprValueUtils.collectionValue(Arrays.asList(0, 1));
+    ExpressionEvaluationException exception = assertThrows(ExpressionEvaluationException.class,
+        () -> compare(collectionValue, collectionValue));
+    assertEquals("ExprCollectionValue instances are not comparable", exception.getMessage());
   }
 }

core/src/test/java/org/opensearch/sql/data/model/ExprIntervalValueTest.java

@@ -17,30 +17,24 @@
 import org.opensearch.sql.exception.ExpressionEvaluationException;
 
 public class ExprIntervalValueTest {
-
   @Test
   public void equals_to_self() {
     ExprValue interval = ExprValueUtils.intervalValue(Duration.ofNanos(1000));
     assertEquals(interval.intervalValue(), Duration.ofNanos(1000));
-    assertTrue(interval.equals(interval));
   }
 
   @Test
   public void equal() {
     ExprValue v1 = new ExprIntervalValue(Duration.ofMinutes(1));
     ExprValue v2 = ExprValueUtils.intervalValue(Duration.ofSeconds(60));
     assertTrue(v1.equals(v2));
-    assertTrue(v2.equals(v1));
-    assertEquals(0, ((ExprIntervalValue)v1).compare((ExprIntervalValue)v2));
-    assertEquals(0, ((ExprIntervalValue)v2).compare((ExprIntervalValue)v1));
   }
 
   @Test
   public void compare() {
     ExprIntervalValue v1 = new ExprIntervalValue(Period.ofDays(1));
     ExprIntervalValue v2 = new ExprIntervalValue(Period.ofDays(2));
-    assertEquals(-1, v1.compare(v2));
-    assertEquals(1, v2.compare(v1));
+    assertEquals(v1.compare(v2), -1);
   }
 
   @Test
@@ -61,12 +55,12 @@ public void invalid_get_value() {
   @Test
   public void value() {
     ExprValue value = new ExprIntervalValue(Period.ofWeeks(1));
-    assertEquals(Period.ofWeeks(1), value.value());
+    assertEquals(value.value(), Period.ofWeeks(1));
   }
 
   @Test
   public void type() {
     ExprValue interval = new ExprIntervalValue(Period.ofYears(1));
-    assertEquals(INTERVAL, interval.type());
+    assertEquals(interval.type(), INTERVAL);
   }
 }

core/src/test/java/org/opensearch/sql/data/model/ExprTupleValueTest.java

@@ -9,14 +9,13 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
-import static org.opensearch.sql.data.type.ExprCoreType.STRUCT;
 import static org.opensearch.sql.utils.ComparisonUtil.compare;
 
 import com.google.common.collect.ImmutableMap;
-import java.util.LinkedHashMap;
-import java.util.Map;
 import org.junit.jupiter.api.Test;
+import org.opensearch.sql.exception.ExpressionEvaluationException;
 
 class ExprTupleValueTest {
   @Test
@@ -52,26 +51,9 @@ public void compare_tuple_with_different_size() {
 
   @Test
   public void comparabilityTest() {
-    ExprValue value1 = ExprValueUtils.tupleValue(ImmutableMap.of("integer_value", 2));
-    ExprValue value2 =
-        ExprValueUtils.tupleValue(ImmutableMap.of("integer_value", 2, "float_value", 1f));
-    assertEquals(0, compare(value1, value1));
-    assertEquals(0, compare(value2, value2));
-    assertEquals(1, compare(value1, value2));
-    assertEquals(1, compare(value2, value1));
-  }
-
-  @Test
-  public void value() {
-    ExprValue value =
-        ExprValueUtils.tupleValue(ImmutableMap.of("integer_value", 2, "float_value", 1f));
-    assertEquals(new LinkedHashMap<>(ImmutableMap.of("integer_value", 2, "float_value", 1f)),
-        value.value());
-  }
-
-  @Test
-  public void type() {
-    ExprValue tuple = ExprValueUtils.tupleValue(Map.of());
-    assertEquals(STRUCT, tuple.type());
+    ExprValue tupleValue = ExprValueUtils.tupleValue(ImmutableMap.of("integer_value", 2));
+    ExpressionEvaluationException exception = assertThrows(ExpressionEvaluationException.class,
+        () -> compare(tupleValue, tupleValue));
+    assertEquals("ExprTupleValue instances are not comparable", exception.getMessage());
   }
 }

core/src/test/java/org/opensearch/sql/data/model/ExprValueCompareTest.java

@@ -91,71 +91,6 @@ public void intervalValueCompare() {
         .compareTo(new ExprIntervalValue(Period.ofDays(2))));
   }
 
-  @Test
-  public void collectionValueCompare() {
-    assertEquals(0, new ExprCollectionValue(ImmutableList.of())
-        .compareTo(new ExprCollectionValue(ImmutableList.of())));
-    assertEquals(0, new ExprCollectionValue(ImmutableList.of(new ExprIntegerValue(1)))
-        .compareTo(new ExprCollectionValue(ImmutableList.of(new ExprIntegerValue(1)))));
-    assertEquals(0, new ExprCollectionValue(ImmutableList.of(
-            new ExprIntegerValue(1), new ExprIntegerValue(2)))
-        .compareTo(new ExprCollectionValue(ImmutableList.of(
-            new ExprIntegerValue(1), new ExprIntegerValue(2)))));
-    assertEquals(1, new ExprCollectionValue(ImmutableList.of(new ExprIntegerValue(2)))
-        .compareTo(new ExprCollectionValue(ImmutableList.of(new ExprIntegerValue(1)))));
-    assertEquals(1, new ExprCollectionValue(ImmutableList.of(new ExprIntegerValue(1)))
-        .compareTo(new ExprCollectionValue(ImmutableList.of(new ExprIntegerValue(2)))));
-    assertEquals(1, new ExprCollectionValue(ImmutableList.of(new ExprIntegerValue(1)))
-        .compareTo(new ExprCollectionValue(ImmutableList.of(
-            new ExprIntegerValue(1), new ExprIntegerValue(1)))));
-    assertEquals(1, new ExprCollectionValue(ImmutableList.of(
-            new ExprIntegerValue(1), new ExprIntegerValue(2)))
-        .compareTo(new ExprCollectionValue(ImmutableList.of(
-            new ExprIntegerValue(1), new ExprIntegerValue(1)))));
-    assertEquals(1, new ExprCollectionValue(ImmutableList.of(
-            new ExprIntegerValue(1), new ExprIntegerValue(2)))
-        .compareTo(new ExprCollectionValue(ImmutableList.of(
-            new ExprIntegerValue(2), new ExprIntegerValue(1)))));
-  }
-
-  @Test
-  public void tupleValueCompare() {
-    assertEquals(0, new ExprTupleValue(new LinkedHashMap<>())
-        .compareTo(new ExprTupleValue(new LinkedHashMap<>())));
-    assertEquals(0, new ExprTupleValue(new LinkedHashMap<>(
-            ImmutableMap.of("1", new ExprIntegerValue(1))))
-        .compareTo(new ExprTupleValue(new LinkedHashMap<>(
-            ImmutableMap.of("1", new ExprIntegerValue(1))))));
-    assertEquals(0, new ExprTupleValue(new LinkedHashMap<>(
-            ImmutableMap.of("1", new ExprIntegerValue(1), "2", new ExprIntegerValue(2))))
-        .compareTo(new ExprTupleValue(new LinkedHashMap<>(
-            ImmutableMap.of("1", new ExprIntegerValue(1), "2", new ExprIntegerValue(2))))));
-    assertEquals(1, new ExprTupleValue(new LinkedHashMap<>(
-            ImmutableMap.of("1", new ExprIntegerValue(2))))
-        .compareTo(new ExprTupleValue(new LinkedHashMap<>(
-            ImmutableMap.of("1", new ExprIntegerValue(1))))));
-    assertEquals(1, new ExprTupleValue(new LinkedHashMap<>(
-            ImmutableMap.of("1", new ExprIntegerValue(1))))
-        .compareTo(new ExprTupleValue(new LinkedHashMap<>(
-            ImmutableMap.of("1", new ExprIntegerValue(2))))));
-    assertEquals(1, new ExprTupleValue(new LinkedHashMap<>(
-            ImmutableMap.of("1", new ExprIntegerValue(1))))
-        .compareTo(new ExprTupleValue(new LinkedHashMap<>(
-            ImmutableMap.of("2", new ExprIntegerValue(1))))));
-    assertEquals(1, new ExprTupleValue(new LinkedHashMap<>(
-            ImmutableMap.of("2", new ExprIntegerValue(1))))
-        .compareTo(new ExprTupleValue(new LinkedHashMap<>(
-            ImmutableMap.of("1", new ExprIntegerValue(1))))));
-    assertEquals(1, new ExprTupleValue(new LinkedHashMap<>(
-            ImmutableMap.of("1", new ExprIntegerValue(1), "2", new ExprIntegerValue(1))))
-        .compareTo(new ExprTupleValue(new LinkedHashMap<>(
-            ImmutableMap.of("1", new ExprIntegerValue(1))))));
-    assertEquals(1, new ExprTupleValue(new LinkedHashMap<>(
-            ImmutableMap.of("1", new ExprIntegerValue(1), "2", new ExprIntegerValue(1))))
-        .compareTo(new ExprTupleValue(new LinkedHashMap<>(
-            ImmutableMap.of("2", new ExprIntegerValue(1), "1", new ExprIntegerValue(1))))));
-  }
-
   @Test
   public void missingCompareToMethodShouldNotBeenCalledDirectly() {
     IllegalStateException exception = assertThrows(IllegalStateException.class,