Adding field comparison in Legacy Engine

core/src/main/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprTimestampValue.java

@@ -19,6 +19,7 @@
 
 import com.amazon.opendistroforelasticsearch.sql.data.type.ExprCoreType;
 import com.amazon.opendistroforelasticsearch.sql.data.type.ExprType;
+import com.amazon.opendistroforelasticsearch.sql.data.utils.ExprDateFormatters;
 import com.amazon.opendistroforelasticsearch.sql.exception.SemanticCheckException;
 import java.time.Instant;
 import java.time.LocalDate;
@@ -69,7 +70,8 @@ public class ExprTimestampValue extends AbstractExprValue {
    */
   public ExprTimestampValue(String timestamp) {
     try {
-      this.timestamp = LocalDateTime.parse(timestamp, FORMATTER_VARIABLE_MICROS)
+      this.timestamp = LocalDateTime.parse(timestamp,
+          ExprDateFormatters.TOLERANT_PARSER_DATE_TIME_FORMATTER)
           .atZone(ZONE)
           .toInstant();
     } catch (DateTimeParseException e) {

elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/data/value/ElasticsearchDateFormatters.java → core/src/main/java/com/amazon/opendistroforelasticsearch/sql/data/utils/ExprDateFormatters.java

@@ -15,7 +15,7 @@
  *
  */
 
-package com.amazon.opendistroforelasticsearch.sql.elasticsearch.data.value;
+package com.amazon.opendistroforelasticsearch.sql.data.utils;
 
 import static java.time.temporal.ChronoField.DAY_OF_MONTH;
 import static java.time.temporal.ChronoField.HOUR_OF_DAY;
@@ -37,7 +37,7 @@
  * Reference org.elasticsearch.common.time.DateFormatters.
  */
 @UtilityClass
-public class ElasticsearchDateFormatters {
+public class ExprDateFormatters {
 
   public static final DateTimeFormatter TIME_ZONE_FORMATTER_NO_COLON =
       new DateTimeFormatterBuilder()
@@ -73,7 +73,16 @@ public class ElasticsearchDateFormatters {
       new DateTimeFormatterBuilder()
           .append(STRICT_YEAR_MONTH_DAY_FORMATTER)
           .optionalStart()
+
+          // otherwise use space
+          .optionalStart()
+          .appendLiteral(' ')
+          .optionalEnd()
+          // optional T
+          .optionalStart()
           .appendLiteral('T')
+          .optionalEnd()
+
           .optionalStart()
           .appendValue(HOUR_OF_DAY, 2, 2, SignStyle.NOT_NEGATIVE)
           .optionalStart()
@@ -82,13 +91,18 @@ public class ElasticsearchDateFormatters {
           .optionalStart()
           .appendLiteral(':')
           .appendValue(SECOND_OF_MINUTE, 2, 2, SignStyle.NOT_NEGATIVE)
+
+          // optional millis with dot
           .optionalStart()
           .appendFraction(NANO_OF_SECOND, 1, 9, true)
           .optionalEnd()
+
+          // otherwise optional millis use with comma
           .optionalStart()
           .appendLiteral(',')
           .appendFraction(NANO_OF_SECOND, 1, 9, false)
           .optionalEnd()
+
           .optionalEnd()
           .optionalEnd()
           .optionalStart()
@@ -104,4 +118,11 @@ public class ElasticsearchDateFormatters {
 
   public static final DateTimeFormatter SQL_LITERAL_DATE_TIME_FORMAT = DateTimeFormatter
           .ofPattern("yyyy-MM-dd HH:mm:ss");
+
+  public static final DateTimeFormatter TOLERANT_PARSER_DATE_TIME_FORMATTER =
+      new DateTimeFormatterBuilder()
+          .appendOptional(STRICT_DATE_OPTIONAL_TIME_FORMATTER)
+          .appendOptional(SQL_LITERAL_DATE_TIME_FORMAT)
+          .appendOptional(STRICT_HOUR_MINUTE_SECOND_FORMATTER)
+          .toFormatter();
 }

core/src/test/java/com/amazon/opendistroforelasticsearch/sql/data/model/DateTimeValueTest.java

@@ -113,9 +113,9 @@ public void timeInUnsupportedFormat() {
   public void timestampInUnsupportedFormat() {
     SemanticCheckException exception =
         assertThrows(SemanticCheckException.class,
-            () -> new ExprTimestampValue("2020-07-07T01:01:01Z"));
+            () -> new ExprTimestampValue("2020-07-07T1:01:01Z"));
     assertEquals(
-        "timestamp:2020-07-07T01:01:01Z in unsupported format, "
+        "timestamp:2020-07-07T1:01:01Z in unsupported format, "
             + "please use yyyy-MM-dd HH:mm:ss[.SSSSSS]",
         exception.getMessage());
   }
@@ -239,9 +239,9 @@ public void datetimeWithVariableMicroPrecision() {
   public void timestampOverMaxMicroPrecision() {
     SemanticCheckException exception =
         assertThrows(SemanticCheckException.class,
-            () -> new ExprTimestampValue("2020-07-07 01:01:01.1234567"));
+            () -> new ExprTimestampValue("2020-07-07 01:01:01.12345678910"));
     assertEquals(
-        "timestamp:2020-07-07 01:01:01.1234567 in unsupported format, "
+        "timestamp:2020-07-07 01:01:01.12345678910 in unsupported format, "
                 + "please use yyyy-MM-dd HH:mm:ss[.SSSSSS]",
         exception.getMessage());
   }

elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/data/value/ElasticsearchExprValueFactory.java

@@ -36,9 +36,6 @@
 import static com.amazon.opendistroforelasticsearch.sql.elasticsearch.data.type.ElasticsearchDataType.ES_IP;
 import static com.amazon.opendistroforelasticsearch.sql.elasticsearch.data.type.ElasticsearchDataType.ES_TEXT;
 import static com.amazon.opendistroforelasticsearch.sql.elasticsearch.data.type.ElasticsearchDataType.ES_TEXT_KEYWORD;
-import static com.amazon.opendistroforelasticsearch.sql.elasticsearch.data.value.ElasticsearchDateFormatters.SQL_LITERAL_DATE_TIME_FORMAT;
-import static com.amazon.opendistroforelasticsearch.sql.elasticsearch.data.value.ElasticsearchDateFormatters.STRICT_DATE_OPTIONAL_TIME_FORMATTER;
-import static com.amazon.opendistroforelasticsearch.sql.elasticsearch.data.value.ElasticsearchDateFormatters.STRICT_HOUR_MINUTE_SECOND_FORMATTER;
 
 import com.amazon.opendistroforelasticsearch.sql.data.model.ExprBooleanValue;
 import com.amazon.opendistroforelasticsearch.sql.data.model.ExprByteValue;
@@ -57,15 +54,14 @@
 import com.amazon.opendistroforelasticsearch.sql.data.model.ExprTupleValue;
 import com.amazon.opendistroforelasticsearch.sql.data.model.ExprValue;
 import com.amazon.opendistroforelasticsearch.sql.data.type.ExprType;
+import com.amazon.opendistroforelasticsearch.sql.data.utils.ExprDateFormatters;
 import com.amazon.opendistroforelasticsearch.sql.elasticsearch.data.utils.Content;
 import com.amazon.opendistroforelasticsearch.sql.elasticsearch.data.utils.ElasticsearchJsonContent;
 import com.amazon.opendistroforelasticsearch.sql.elasticsearch.data.utils.ObjectContent;
 import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.collect.ImmutableMap;
 import java.time.Instant;
-import java.time.format.DateTimeFormatter;
-import java.time.format.DateTimeFormatterBuilder;
 import java.time.format.DateTimeParseException;
 import java.util.ArrayList;
 import java.util.LinkedHashMap;
@@ -87,13 +83,6 @@ public class ElasticsearchExprValueFactory {
   @Setter
   private Map<String, ExprType> typeMapping;
 
-  private static final DateTimeFormatter DATE_TIME_FORMATTER =
-      new DateTimeFormatterBuilder()
-          .appendOptional(SQL_LITERAL_DATE_TIME_FORMAT)
-          .appendOptional(STRICT_DATE_OPTIONAL_TIME_FORMATTER)
-          .appendOptional(STRICT_HOUR_MINUTE_SECOND_FORMATTER)
-          .toFormatter();
-
   private static final String TOP_PATH = "";
 
   private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
@@ -185,7 +174,8 @@ private ExprValue constructTimestamp(String value) {
     try {
       return new ExprTimestampValue(
           // Using Elasticsearch DateFormatters for now.
-          DateFormatters.from(DATE_TIME_FORMATTER.parse(value)).toInstant());
+          DateFormatters.from(ExprDateFormatters.TOLERANT_PARSER_DATE_TIME_FORMATTER
+              .parse(value)).toInstant());
     } catch (DateTimeParseException e) {
       throw new IllegalStateException(
           String.format(

elasticsearch/src/test/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/data/value/ElasticsearchExprValueFactoryTest.java

@@ -190,6 +190,12 @@ public void constructDate() {
     assertEquals(
         new ExprTimestampValue("2015-01-01 12:10:30"),
         tupleValue("{\"timestampV\":\"2015-01-01 12:10:30\"}").get("timestampV"));
+    assertEquals(
+        new ExprTimestampValue("2020-08-17T19:44:00.100500"),
+        tupleValue("{\"timestampV\":\"2020-08-17T19:44:00.100500\"}").get("timestampV"));
+    assertEquals(
+        new ExprTimestampValue("2020-08-17 19:44:00.100500"),
+        tupleValue("{\"timestampV\":\"2020-08-17 19:44:00.100500\"}").get("timestampV"));
     assertEquals(
         new ExprTimestampValue(Instant.ofEpochMilli(1420070400001L)),
         tupleValue("{\"timestampV\":1420070400001}").get("timestampV"));
@@ -221,9 +227,10 @@ public void constructDate() {
   public void constructDateFromUnsupportedFormatThrowException() {
     IllegalStateException exception =
         assertThrows(
-            IllegalStateException.class, () -> tupleValue("{\"timestampV\":\"2015-01-01 12:10\"}"));
+            IllegalStateException.class, () ->
+                tupleValue("{\"timestampV\":\"2015-01-01 1:10:10\"}"));
     assertEquals(
-        "Construct ExprTimestampValue from \"2015-01-01 12:10\" failed, "
+        "Construct ExprTimestampValue from \"2015-01-01 1:10:10\" failed, "
             + "unsupported date format.",
         exception.getMessage());
   }

integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/MethodQueryIT.java

@@ -22,6 +22,7 @@
 import java.io.IOException;
 import java.util.Locale;
 import org.junit.Assert;
+import org.junit.Assume;
 import org.junit.Test;
 
 /**
@@ -144,4 +145,44 @@ public void matchPhraseQueryTest() throws IOException {
     Assert.assertThat(result,
         containsString("{\"match_phrase\":{\"address\":{\"query\":\"671 Bristol Street\""));
   }
+
+  @Test
+  public void testFieldToFieldComparison() throws IOException {
+    Assume.assumeFalse(isNewQueryEngineEabled());
+    String result =  explainQuery("select * from " + TestsConstants.TEST_INDEX_ACCOUNT
+        + " where age > account_number");
+
+    Assert.assertThat(result,
+        containsString("{\"script\":{\"script\":{\"source\":\"doc['age'].value "
+            + "> doc['account_number'].value\",\"lang\":\"painless\"}"));
+
+  }
+
+  @Test
+  public void testFieldToFieldComparisonWithExtraClause() throws IOException {
+    Assume.assumeFalse(isNewQueryEngineEabled());
+    String result =  explainQuery("select * from " + TestsConstants.TEST_INDEX_ACCOUNT
+        + " where age > account_number AND age in (1)");
+
+    Assert.assertThat(result,
+        containsString("{\"script\":{\"script\":{\"source\":\"doc['age'].value "
+            + "> doc['account_number'].value\",\"lang\":\"painless\"}"));
+    Assert.assertThat(result,
+        containsString("{\"term\":{\"age\":{\"value\":1,\"boost\":1.0}}}"));
+
+  }
+
+  @Test
+  public void testFieldToFieldComparisonWithDifferentOperator() throws IOException {
+    Assume.assumeFalse(isNewQueryEngineEabled());
+    String result =  explainQuery("select * from " + TestsConstants.TEST_INDEX_ACCOUNT
+        + " where age <> account_number AND age in (1)");
+
+    Assert.assertThat(result,
+        containsString("{\"script\":{\"script\":{\"source\":\"doc['age'].value "
+            + "!= doc['account_number'].value\",\"lang\":\"painless\"}"));
+    Assert.assertThat(result,
+        containsString("{\"term\":{\"age\":{\"value\":1,\"boost\":1.0}}}"));
+
+  }
 }

legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/domain/Where.java

@@ -31,16 +31,34 @@ public static Where newInstance() {
         return new Where(CONN.AND);
     }
 
+    public static Where newInstance(boolean isJoin) {
+        return new Where(CONN.AND, isJoin);
+    }
+
     private LinkedList<Where> wheres = new LinkedList<>();
 
     protected CONN conn;
+    protected final boolean isJoin;
 
     public Where(String connStr) {
-        this.conn = CONN.valueOf(connStr.toUpperCase());
+        this(connStr, false);
+    }
+
+    public Where(String connStr, boolean isJoin) {
+        this(CONN.valueOf(connStr.toUpperCase()), isJoin);
     }
 
     public Where(CONN conn) {
+        this(conn, false);
+    }
+
+    public Where(CONN conn, boolean isJoin) {
         this.conn = conn;
+        this.isJoin=isJoin;
+    }
+
+    public boolean isJoin() {
+        return isJoin;
     }
 
     public void addWhere(Where where) {

legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/SqlParser.java

@@ -448,7 +448,7 @@ private JoinSelect createBasicJoinSelectAccordingToTableSource(SQLJoinTableSourc
             throws SqlParseException {
         JoinSelect joinSelect = new JoinSelect();
         if (joinTableSource.getCondition() != null) {
-            Where where = Where.newInstance();
+            Where where = Where.newInstance(true);
             WhereParser whereParser = new WhereParser(this, joinTableSource.getCondition());
             whereParser.parseWhere(joinTableSource.getCondition(), where);
             joinSelect.setConnectedWhere(where);

legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/WhereParser.java

@@ -214,7 +214,7 @@ private void routeCond(SQLBinaryOpExpr bExpr, SQLExpr sub, Where where) throws S
         if (sub instanceof SQLBinaryOpExpr && !isCond((SQLBinaryOpExpr) sub)) {
             SQLBinaryOpExpr binarySub = (SQLBinaryOpExpr) sub;
             if (binarySub.getOperator().priority != bExpr.getOperator().priority) {
-                Where subWhere = new Where(bExpr.getOperator().name);
+                Where subWhere = new Where(bExpr.getOperator().name, where.isJoin());
                 where.addWhere(subWhere);
                 parseWhere(binarySub, subWhere);
             } else {
@@ -291,7 +291,7 @@ private void explainCond(String opear, SQLExpr expr, Where where) throws SqlPars
                     condition = new Condition(Where.CONN.valueOf(opear), soExpr.getLeft().toString(), soExpr.getLeft(),
                             soExpr.getOperator().name, parseValue(soExpr.getRight()), soExpr.getRight(), childrenType);
                 } else {
-                    SQLMethodInvokeExpr sqlMethodInvokeExpr = parseSQLBinaryOpExprWhoIsConditionInWhere(soExpr);
+                    SQLMethodInvokeExpr sqlMethodInvokeExpr = parseSQLBinaryOpExprWhoIsConditionInWhere(soExpr, where.isJoin());
                     if (sqlMethodInvokeExpr == null) {
                         condition = new Condition(Where.CONN.valueOf(opear), soExpr.getLeft().toString(),
                                 soExpr.getLeft(), soExpr.getOperator().name, parseValue(soExpr.getRight()),
@@ -536,10 +536,20 @@ private MethodField parseSQLCastExprWithFunctionInWhere(SQLCastExpr soExpr) thro
         );
     }
 
-    private SQLMethodInvokeExpr parseSQLBinaryOpExprWhoIsConditionInWhere(SQLBinaryOpExpr soExpr)
+    private SQLMethodInvokeExpr parseSQLBinaryOpExprWhoIsConditionInWhere(SQLBinaryOpExpr soExpr,
+        boolean join)
             throws SqlParseException {
 
-        if (bothSideAreNotFunction(soExpr) && bothSidesAreNotCast(soExpr)) {
+        if (
+            // if one of the two side is a method, it has to be resolved as script
+            neitherSideIsFunction(soExpr)
+            // if one of the two side is a cast, it has to be resolved as script
+            && neitherSidesIsCast(soExpr)
+            // if both sides are field, we cannot use the QueryRange (which would be more efficient
+            // `field > integer` comparisons) but ES doesn't allow comparing two fields and
+            // we must use a SCRIPT
+            && (!bothSidesAreFieldIdentifier(soExpr) || join)
+        ) {
             return null;
         }
 
@@ -600,6 +610,10 @@ private SQLMethodInvokeExpr parseSQLBinaryOpExprWhoIsConditionInWhere(SQLBinaryO
             operator = "==";
         }
 
+        if (operator.equals("<>")) {
+            operator = "!=";
+        }
+
         String finalStr = v1Dec + v2Dec + v1 + " " + operator + " " + v2;
 
         SQLMethodInvokeExpr scriptMethod = new SQLMethodInvokeExpr("script", null);
@@ -608,14 +622,22 @@ private SQLMethodInvokeExpr parseSQLBinaryOpExprWhoIsConditionInWhere(SQLBinaryO
 
     }
 
-    private Boolean bothSideAreNotFunction(SQLBinaryOpExpr soExpr) {
+    private Boolean neitherSideIsFunction(SQLBinaryOpExpr soExpr) {
         return !(soExpr.getLeft() instanceof SQLMethodInvokeExpr || soExpr.getRight() instanceof SQLMethodInvokeExpr);
     }
 
-    private Boolean bothSidesAreNotCast(SQLBinaryOpExpr soExpr) {
+    private Boolean neitherSidesIsCast(SQLBinaryOpExpr soExpr) {
         return !(soExpr.getLeft() instanceof SQLCastExpr || soExpr.getRight() instanceof SQLCastExpr);
     }
 
+    private Boolean bothSidesAreFieldIdentifier(SQLBinaryOpExpr soExpr) {
+        return (soExpr.getLeft() instanceof SQLIdentifierExpr && soExpr.getRight() instanceof SQLIdentifierExpr)
+            // "missing" is a SQLIdentifier but not a Field Identifier.
+            // We might want to have a subclass for "missing" or for "fieldIdentifier" or
+            // change type altogether to avoid conflicts
+            && !"missing".equalsIgnoreCase(((SQLIdentifierExpr) soExpr.getRight()).getLowerName());
+    }
+
     private Object[] getMethodValuesWithSubQueries(SQLMethodInvokeExpr method) throws SqlParseException {
         List<Object> values = new ArrayList<>();
         for (SQLExpr innerExpr : method.getParameters()) {