[Backport 2.19-dev] [BugFix] Fix incorrect push down for Sarg with nullAs is TRUE (#3882)

integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/3477.yml

@@ -49,6 +49,8 @@ teardown:
   - match: {"datarows": [[{ "json" : { "status": "SUCCESS", "time": 100} }], [{ "json" : { "status": "SUCCESS", "time": 100} }], [{ "json" : { "status": "SUCCESS", "time": 100} }], [{ "json" : { "status": "SUCCESS", "time": 100} }], [{ "json" : { "status": "SUCCESS", "time": 100} }]]}
 
   - do:
+      allowed_warnings:
+        - 'Loading the fielddata on the _id field is deprecated and will be removed in future versions. If you require sorting or aggregating on this field you should also include the id in the body of your documents, and map this field as a keyword field that has [doc_values] enabled'
       headers:
         Content-Type: 'application/json'
       ppl:

integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/3570.yml

@@ -101,7 +101,11 @@ teardown:
   - skip:
       features:
         - headers
+        - allowed_warnings
+
   - do:
+      allowed_warnings:
+        - 'Loading the fielddata on the _id field is deprecated and will be removed in future versions. If you require sorting or aggregating on this field you should also include the id in the body of your documents, and map this field as a keyword field that has [doc_values] enabled'
       headers:
         Content-Type: 'application/json'
       ppl:

integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/3881.yml

@@ -0,0 +1,58 @@
+setup:
+  - do:
+      query.settings:
+        body:
+          transient:
+            plugins.calcite.enabled : true
+            plugins.calcite.fallback.allowed : false
+
+---
+teardown:
+  - do:
+      query.settings:
+        body:
+          transient:
+            plugins.calcite.enabled : false
+            plugins.calcite.fallback.allowed : true
+
+---
+"Handle sag with nullAs":
+  - skip:
+      features:
+        - headers
+        - allowed_warnings
+  - do:
+      bulk:
+        index: test
+        refresh: true
+        body:
+          - '{"index": {}}'
+          - '{"a": 1}'
+          - '{"index": {}}'
+          - '{"a": 2}'
+          - '{"index": {}}'
+          - '{}'
+
+  - do:
+      allowed_warnings:
+        - 'Loading the fielddata on the _id field is deprecated and will be removed in future versions. If you require sorting or aggregating on this field you should also include the id in the body of your documents, and map this field as a keyword field that has [doc_values] enabled'
+      headers:
+        Content-Type: 'application/json'
+      ppl:
+        body:
+          query: 'source=test | where a = 1 or a = 2 or isNull(a)'
+  - match: {"total": 3}
+  - match: {"schema": [{"name": "a", "type": "bigint"}]}
+  - match: {"datarows": [[1], [2], [null]]}
+
+  - do:
+      allowed_warnings:
+        - 'Loading the fielddata on the _id field is deprecated and will be removed in future versions. If you require sorting or aggregating on this field you should also include the id in the body of your documents, and map this field as a keyword field that has [doc_values] enabled'
+      headers:
+        Content-Type: 'application/json'
+      ppl:
+        body:
+          query: 'source=test | where (a = 1 or a = 2) and isNotNull(a)'
+  - match: { "total": 2 }
+  - match: { "schema": [ { "name": "a", "type": "bigint" } ] }
+  - match: {"datarows": [[1], [2]]}

opensearch/src/main/java/org/opensearch/sql/opensearch/request/PredicateAnalyzer.java

@@ -60,6 +60,7 @@
 import org.apache.calcite.rex.RexInputRef;
 import org.apache.calcite.rex.RexLiteral;
 import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.rex.RexUnknownAs;
 import org.apache.calcite.rex.RexVisitorImpl;
 import org.apache.calcite.sql.SqlKind;
 import org.apache.calcite.sql.SqlOperator;
@@ -76,7 +77,7 @@
 import org.opensearch.index.query.RangeQueryBuilder;
 import org.opensearch.sql.calcite.plan.OpenSearchConstants;
 import org.opensearch.sql.calcite.type.ExprSqlType;
-import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory;
+import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory.ExprUDT;
 import org.opensearch.sql.data.model.ExprTimestampValue;
 import org.opensearch.sql.data.type.ExprCoreType;
 import org.opensearch.sql.data.type.ExprType;
@@ -256,6 +257,12 @@ static boolean isSearchWithComplementedPoints(RexCall search) {
       return sarg.isComplementedPoints();
     }
 
+    static RexUnknownAs getNullAsForSearch(RexCall search) {
+      RexLiteral literal = (RexLiteral) search.getOperands().get(1);
+      final Sarg<?> sarg = requireNonNull(literal.getValueAs(Sarg.class), "Sarg");
+      return sarg.nullAs;
+    }
+
     @Override
     public Expression visitCall(RexCall call) {
 
@@ -332,7 +339,7 @@ private QueryExpression visitRelevanceFunc(RexCall call) {
       }
 
       throw new PredicateAnalyzerException(
-          String.format(Locale.ROOT, "Unsupported search relevance function: [%s]", funcName));
+          format(Locale.ROOT, "Unsupported search relevance function: [%s]", funcName));
     }
 
     @FunctionalInterface
@@ -377,7 +384,7 @@ private Map<String, String> parseRelevanceFunctionOptionalArguments(
         String key = ((RexLiteral) aliasPair.alias).getValueAs(String.class);
         if (optionalArguments.containsKey(key)) {
           throw new PredicateAnalyzerException(
-              String.format(
+              format(
                   Locale.ROOT,
                   "Parameter '%s' can only be specified once for function [%s].",
                   key,
@@ -392,7 +399,7 @@ private Map<String, String> parseRelevanceFunctionOptionalArguments(
     private static RexCall expectCall(RexNode node, SqlOperator op, String funcName) {
       if (!(node instanceof RexCall)) {
         throw new IllegalArgumentException(
-            String.format(
+            format(
                 Locale.ROOT,
                 "Expect [%s] RexCall but get [%s] for function [%s]",
                 op.getName(),
@@ -527,38 +534,55 @@ private QueryExpression binary(RexCall call) {
           }
           return QueryExpression.create(pair.getKey()).lte(pair.getValue());
         case SEARCH:
-          if (isSearchWithComplementedPoints(call)) {
-            return QueryExpression.create(pair.getKey()).notIn(pair.getValue());
-          } else if (isSearchWithPoints(call)) {
-            return QueryExpression.create(pair.getKey()).in(pair.getValue());
-          } else {
-            Sarg<?> sarg = pair.getValue().literal.getValueAs(Sarg.class);
-            Set<? extends Range<?>> rangeSet = requireNonNull(sarg).rangeSet.asRanges();
-            boolean isTimeStamp =
-                (pair.getKey() instanceof NamedFieldExpression)
-                    && ((NamedFieldExpression)pair.getKey()).isTimeStampType();
-            List<QueryExpression> queryExpressions =
-                rangeSet.stream()
-                    .map(
-                        range ->
-                            RangeSets.isPoint(range)
-                                ? QueryExpression.create(pair.getKey())
-                                    .equals(sargPointValue(range.lowerEndpoint()), isTimeStamp)
-                                : QueryExpression.create(pair.getKey()).between(range, isTimeStamp))
-                    .collect(Collectors.toList());
-            if (queryExpressions.size() == 1) {
-              return queryExpressions.get(0);
-            } else {
-              return CompoundQueryExpression.or(queryExpressions.toArray(new QueryExpression[0]));
-            }
-          }
+          QueryExpression expression = constructQueryExpressionForSearch(call, pair);
+          RexUnknownAs nullAs = getNullAsForSearch(call);
+          switch (nullAs) {
+              // e.g. where isNotNull(a) and (a = 1 or a = 2)
+              // TODO: For this case, seems return `expression` should be equivalent
+            case FALSE: return CompoundQueryExpression.and(
+                false, expression, QueryExpression.create(pair.getKey()).exists());
+              // e.g. where isNull(a) or a = 1 or a = 2
+            case TRUE: return CompoundQueryExpression.or(
+                expression, QueryExpression.create(pair.getKey()).notExists());
+              // e.g. where a = 1 or a = 2
+            case UNKNOWN: return expression;
+          };
         default:
           break;
       }
       String message = format(Locale.ROOT, "Unable to handle call: [%s]", call);
       throw new PredicateAnalyzerException(message);
     }
 
+    private static QueryExpression constructQueryExpressionForSearch(
+        RexCall call, SwapResult pair) {
+      if (isSearchWithComplementedPoints(call)) {
+        return QueryExpression.create(pair.getKey()).notIn(pair.getValue());
+      } else if (isSearchWithPoints(call)) {
+        return QueryExpression.create(pair.getKey()).in(pair.getValue());
+      } else {
+        Sarg<?> sarg = pair.getValue().literal.getValueAs(Sarg.class);
+        Set<? extends Range<?>> rangeSet = requireNonNull(sarg).rangeSet.asRanges();
+        boolean isTimeStamp =
+            (pair.getKey() instanceof NamedFieldExpression)
+                && ((NamedFieldExpression)pair.getKey()).isTimeStampType();
+        List<QueryExpression> queryExpressions =
+            rangeSet.stream()
+                .map(
+                    range ->
+                        RangeSets.isPoint(range)
+                            ? QueryExpression.create(pair.getKey())
+                                .equals(sargPointValue(range.lowerEndpoint()), isTimeStamp)
+                            : QueryExpression.create(pair.getKey()).between(range, isTimeStamp))
+                .collect(Collectors.toList());
+        if (queryExpressions.size() == 1) {
+          return queryExpressions.get(0);
+        } else {
+          return CompoundQueryExpression.or(queryExpressions.toArray(new QueryExpression[0]));
+        }
+      }
+    }
+
     private QueryExpression andOr(RexCall call) {
       QueryExpression[] expressions = new QueryExpression[call.getOperands().size()];
       PredicateAnalyzerException firstError = null;
@@ -756,8 +780,7 @@ QueryExpression contains(LiteralExpression literal) {
     }
 
     QueryExpression between(Range<?> literal, boolean isTimeStamp) {
-      throw new PredicateAnalyzer.PredicateAnalyzerException(
-          "between cannot be applied to " + this.getClass());
+      throw new PredicateAnalyzerException("between cannot be applied to " + this.getClass());
     }
 
     QueryExpression like(LiteralExpression literal) {
@@ -776,8 +799,7 @@ QueryExpression equals(LiteralExpression literal) {
     }
 
     QueryExpression equals(Object point, boolean isTimeStamp) {
-      throw new PredicateAnalyzer.PredicateAnalyzerException(
-          "equals cannot be applied to " + this.getClass());
+      throw new PredicateAnalyzerException("equals cannot be applied to " + this.getClass());
     }
 
     QueryExpression notEquals(LiteralExpression literal) {
@@ -863,7 +885,7 @@ static QueryExpression create(TerminalExpression expression) {
         return new SimpleQueryExpression((NamedFieldExpression) expression);
       } else {
         String message = format(Locale.ROOT, "Unsupported expression: [%s]", expression);
-        throw new PredicateAnalyzer.PredicateAnalyzerException(message);
+        throw new PredicateAnalyzerException(message);
       }
     }
   }
@@ -1381,7 +1403,7 @@ public boolean isSarg() {
     public boolean isTimestamp() {
       if (literal.getType() instanceof ExprSqlType) {
         ExprSqlType exprSqlType = (ExprSqlType) literal.getType();
-        return exprSqlType.getUdt() == OpenSearchTypeFactory.ExprUDT.EXPR_TIMESTAMP;
+        return exprSqlType.getUdt() == ExprUDT.EXPR_TIMESTAMP;
       }
       return false;
     }
@@ -1452,7 +1474,7 @@ private static void checkForIncompatibleDateTimeOperands(RexCall call) {
         || (SqlTypeFamily.TIMESTAMP.contains(op2) && !SqlTypeFamily.TIMESTAMP.contains(op1))
         || (SqlTypeFamily.TIME.contains(op1) && !SqlTypeFamily.TIME.contains(op2))
         || (SqlTypeFamily.TIME.contains(op2) && !SqlTypeFamily.TIME.contains(op1))) {
-      throw new PredicateAnalyzer.PredicateAnalyzerException(
+      throw new PredicateAnalyzerException(
           "Cannot handle " + call.getKind() + " expression for _id field, " + call);
     }
   }