From 0bbfc2d48585befc10a965dd12c8106f3a067d8f Mon Sep 17 00:00:00 2001 From: "Capponi, Francesco" Date: Wed, 6 Jan 2021 23:52:09 -0500 Subject: [PATCH 1/3] Adding additional logging to ExprMissingValue exception --- .../legacy/expression/domain/BindingTuple.java | 2 +- .../legacy/expression/model/ExprMissingValue.java | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/expression/domain/BindingTuple.java b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/expression/domain/BindingTuple.java index 857f6ddb02..179fa7b80e 100644 --- a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/expression/domain/BindingTuple.java +++ b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/expression/domain/BindingTuple.java @@ -46,7 +46,7 @@ public class BindingTuple { * @return binding value. */ public ExprValue resolve(String bindingName) { - return bindingMap.getOrDefault(bindingName, new ExprMissingValue()); + return bindingMap.getOrDefault(bindingName, new ExprMissingValue(bindingName, bindingMap.keySet())); } @Override diff --git a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/expression/model/ExprMissingValue.java b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/expression/model/ExprMissingValue.java index 52c841a089..9d5d5dab70 100644 --- a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/expression/model/ExprMissingValue.java +++ b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/expression/model/ExprMissingValue.java @@ -15,10 +15,25 @@ package com.amazon.opendistroforelasticsearch.sql.legacy.expression.model; +import java.util.Set; + /** * The definition of the missing value. */ public class ExprMissingValue implements ExprValue { + private final String missingValue; + private final Set availableValues; + + public ExprMissingValue(String missingValue, Set availableValues) { + this.missingValue = missingValue; + this.availableValues = availableValues; + } + + @Override + public Object value() { + throw new IllegalStateException("invalid value operation on " + kind() + " (" + this.missingValue + "), available: " + this.availableValues.toString()); + } + @Override public ExprValueKind kind() { return ExprValueKind.MISSING_VALUE; From 76248c78d744ca97b39a23ac3247e89c94e98987 Mon Sep 17 00:00:00 2001 From: Francesco Capponi Date: Mon, 7 Jun 2021 10:55:31 -0400 Subject: [PATCH 2/3] Adding field comparison in Legacy Engine --- .../sql/legacy/MethodQueryIT.java | 27 +++++++++++++++++ .../sql/legacy/domain/Where.java | 20 ++++++++++++- .../sql/legacy/parser/SqlParser.java | 2 +- .../sql/legacy/parser/WhereParser.java | 30 +++++++++++++++---- 4 files changed, 71 insertions(+), 8 deletions(-) diff --git a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/MethodQueryIT.java b/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/MethodQueryIT.java index 862363df8c..77c564337c 100644 --- a/integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/legacy/MethodQueryIT.java +++ b/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,30 @@ 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}}}")); + + } } diff --git a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/domain/Where.java b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/domain/Where.java index 8b7f436f76..d929ba8280 100644 --- a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/domain/Where.java +++ b/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 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) { diff --git a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/SqlParser.java b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/SqlParser.java index d296f66b0c..44ecc36daf 100644 --- a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/SqlParser.java +++ b/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); diff --git a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/WhereParser.java b/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/WhereParser.java index 73814fff0e..8e67aff653 100644 --- a/legacy/src/main/java/com/amazon/opendistroforelasticsearch/sql/legacy/parser/WhereParser.java +++ b/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; } @@ -608,14 +618,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 values = new ArrayList<>(); for (SQLExpr innerExpr : method.getParameters()) { From 295c6f693a7c90ea3bb171ad8f28900c14758302 Mon Sep 17 00:00:00 2001 From: "Capponi, Francesco" Date: Fri, 13 Aug 2021 10:37:20 -0400 Subject: [PATCH 3/3] Default files in lf format --- .gitattributes | 1 + 1 file changed, 1 insertion(+) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000000..6313b56c57 --- /dev/null +++ b/.gitattributes @@ -0,0 +1 @@ +* text=auto eol=lf