Address review: reuse addFormatIfNecessary + value(), add e2e/yaml tests

RyanL1997 · RyanL1997 · commit 311d38ffce6d · 2026-06-08T11:41:21.000-07:00
Refactor the opensearch-project#5481 fix per review: - Reuse the existing addFormatIfNecessary helper (re-keyed on the resolved isTimeStamp boolean) instead of inlining format("date_time") across the six comparison methods; the helper was orphaned by the prior commit. - Reuse LiteralExpression.value() + the existing Sarg-path convertEndpointValue for endpoint normalization, removing the parallel endpointValue helper. - Guard convertEndpointValue against a null endpoint (shared entry point). Tests: - Add stripped-VARCHAR unit cases for equals/notEquals/lte (gt already covered) so every range shape pins the field-type fallback, not just gt. - Add end-to-end IT CalcitePPLBasicIT.testTimestampRangeWithInClausePushDown (timestamp range AND keyword IN), confirmed to reproduce the shard date-parse error when the fix is reverted. - Add yamlRest test issues/5481.yml for the same shape. Signed-off-by: Jialiang Liang <jiallian@amazon.com>
diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLBasicIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLBasicIT.java
@@ -512,6 +512,28 @@ public void testDateBetween() throws IOException {
         actual, rows("Nanette", "2018-06-23 00:00:00"), rows("Elinor", "2018-06-27 00:00:00"));
   }
 
+  /**
+   * Issue #5481: a timestamp range comparison AND'd with an {@code IN} clause on another field must
+   * push down and return rows. Calcite folds the {@code IN} into a {@code Sarg} and strips the
+   * timestamp literal's UDT; without the field-type-keyed fix the range query ships an unformatted
+   * date to the shard, which rejects it with a date-parse error (HTTP 500).
+   */
+  @Test
+  public void testTimestampRangeWithInClausePushDown() throws IOException {
+    JSONObject actual =
+        executeQuery(
+            String.format(
+                "source=%s | where birthdate > timestamp('2018-06-01 00:00:00') | where state in"
+                    + " ('IL', 'TN', 'WA') | fields firstname, state, birthdate",
+                TEST_INDEX_BANK));
+    verifySchema(
+        actual,
+        schema("firstname", "string"),
+        schema("state", "string"),
+        schema("birthdate", "timestamp"));
+    verifyDataRows(actual, rows("Elinor", "WA", "2018-06-27 00:00:00"));
+  }
+
   @Test
   public void testXor() throws IOException {
     JSONObject result =
diff --git a/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5481.yml b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5481.yml
@@ -0,0 +1,69 @@
+# Issue: https://github.com/opensearch-project/sql/issues/5481
+# A timestamp range comparison AND'd with an IN clause on another field must push down and
+# return rows. Calcite folds the IN into a Sarg and strips the timestamp literal's UDT; without
+# the field-type-keyed fix the range query ships an unformatted date and the shard rejects it.
+setup:
+  - do:
+      query.settings:
+        body:
+          transient:
+            plugins.calcite.enabled: true
+
+  - do:
+      indices.create:
+        index: issue5481
+        body:
+          settings:
+            number_of_shards: 1
+            number_of_replicas: 0
+          mappings:
+            properties:
+              event_time:
+                type: date
+              severity:
+                type: keyword
+
+  - do:
+      bulk:
+        refresh: true
+        body:
+          - '{"index": {"_index": "issue5481", "_id": "1"}}'
+          - '{"event_time": "2026-05-28T10:00:00Z", "severity": "ERROR"}'
+          - '{"index": {"_index": "issue5481", "_id": "2"}}'
+          - '{"event_time": "2026-05-28T10:05:00Z", "severity": "WARN"}'
+          - '{"index": {"_index": "issue5481", "_id": "3"}}'
+          - '{"event_time": "2026-05-28T10:10:00Z", "severity": "INFO"}'
+          - '{"index": {"_index": "issue5481", "_id": "4"}}'
+          - '{"event_time": "2026-05-28T10:15:00Z", "severity": "ERROR"}'
+          - '{"index": {"_index": "issue5481", "_id": "5"}}'
+          - '{"event_time": "2026-05-28T10:20:00Z", "severity": "WARN"}'
+          - '{"index": {"_index": "issue5481", "_id": "6"}}'
+          - '{"event_time": "2026-05-28T10:25:00Z", "severity": "DEBUG"}'
+
+---
+teardown:
+  - do:
+      indices.delete:
+        index: issue5481
+        ignore_unavailable: true
+  - do:
+      query.settings:
+        body:
+          transient:
+            plugins.calcite.enabled: false
+
+---
+"Issue 5481: timestamp range AND keyword IN pushes down and returns rows":
+  - skip:
+      features:
+        - headers
+  - do:
+      headers:
+        Content-Type: 'application/json'
+      ppl:
+        body:
+          query: source=issue5481 | where event_time > timestamp('2026-05-28 10:08:00') | where severity in ('ERROR', 'WARN') | fields severity, event_time | sort event_time
+
+  - match: { total: 2 }
+  - match: { schema: [ { name: severity, type: "string" }, { name: event_time, type: "timestamp" } ] }
+  - match: { datarows: [ [ "ERROR", "2026-05-28 10:15:00" ], [ "WARN", "2026-05-28 10:20:00" ] ] }
diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/request/PredicateAnalyzer.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/request/PredicateAnalyzer.java
@@ -1379,9 +1379,11 @@ public QueryExpression notLike(LiteralExpression literal) {
     @Override
     public QueryExpression equals(LiteralExpression literal) {
       boolean isTimeStamp = isFieldOrLiteralDateTime(literal);
-      Object value = endpointValue(literal, isTimeStamp);
+      Object value = convertEndpointValue(literal.value(), isTimeStamp);
       if (isTimeStamp) {
-        builder = rangeQuery(getFieldReference()).gte(value).lte(value).format("date_time");
+        builder =
+            addFormatIfNecessary(
+                isTimeStamp, rangeQuery(getFieldReference()).gte(value).lte(value));
       } else {
         builder = termQuery(getFieldReferenceForTermQuery(), value);
       }
@@ -1391,12 +1393,14 @@ public QueryExpression equals(LiteralExpression literal) {
     @Override
     public QueryExpression notEquals(LiteralExpression literal) {
       boolean isTimeStamp = isFieldOrLiteralDateTime(literal);
-      Object value = endpointValue(literal, isTimeStamp);
+      Object value = convertEndpointValue(literal.value(), isTimeStamp);
       if (isTimeStamp) {
         builder =
             boolQuery()
-                .should(rangeQuery(getFieldReference()).gt(value).format("date_time"))
-                .should(rangeQuery(getFieldReference()).lt(value).format("date_time"));
+                .should(
+                    addFormatIfNecessary(isTimeStamp, rangeQuery(getFieldReference()).gt(value)))
+                .should(
+                    addFormatIfNecessary(isTimeStamp, rangeQuery(getFieldReference()).lt(value)));
       } else {
         builder =
             boolQuery()
@@ -1410,56 +1414,47 @@ public QueryExpression notEquals(LiteralExpression literal) {
     @Override
     public QueryExpression gt(LiteralExpression literal) {
       boolean isTimeStamp = isFieldOrLiteralDateTime(literal);
-      Object value = endpointValue(literal, isTimeStamp);
-      RangeQueryBuilder rq = rangeQuery(getFieldReference()).gt(value);
-      if (isTimeStamp) rq.format("date_time");
-      builder = rq;
+      Object value = convertEndpointValue(literal.value(), isTimeStamp);
+      builder = addFormatIfNecessary(isTimeStamp, rangeQuery(getFieldReference()).gt(value));
       return this;
     }
 
     @Override
     public QueryExpression gte(LiteralExpression literal) {
       boolean isTimeStamp = isFieldOrLiteralDateTime(literal);
-      Object value = endpointValue(literal, isTimeStamp);
-      RangeQueryBuilder rq = rangeQuery(getFieldReference()).gte(value);
-      if (isTimeStamp) rq.format("date_time");
-      builder = rq;
+      Object value = convertEndpointValue(literal.value(), isTimeStamp);
+      builder = addFormatIfNecessary(isTimeStamp, rangeQuery(getFieldReference()).gte(value));
       return this;
     }
 
     @Override
     public QueryExpression lt(LiteralExpression literal) {
       boolean isTimeStamp = isFieldOrLiteralDateTime(literal);
-      Object value = endpointValue(literal, isTimeStamp);
-      RangeQueryBuilder rq = rangeQuery(getFieldReference()).lt(value);
-      if (isTimeStamp) rq.format("date_time");
-      builder = rq;
+      Object value = convertEndpointValue(literal.value(), isTimeStamp);
+      builder = addFormatIfNecessary(isTimeStamp, rangeQuery(getFieldReference()).lt(value));
       return this;
     }
 
     @Override
     public QueryExpression lte(LiteralExpression literal) {
       boolean isTimeStamp = isFieldOrLiteralDateTime(literal);
-      Object value = endpointValue(literal, isTimeStamp);
-      RangeQueryBuilder rq = rangeQuery(getFieldReference()).lte(value);
-      if (isTimeStamp) rq.format("date_time");
-      builder = rq;
+      Object value = convertEndpointValue(literal.value(), isTimeStamp);
+      builder = addFormatIfNecessary(isTimeStamp, rangeQuery(getFieldReference()).lte(value));
       return this;
     }
 
-    // Field type is the reliable source: RexSimplify can strip the literal's UDT when a sibling
-    // clause is folded into a Sarg, leaving it as VARCHAR.
+    /**
+     * Whether the comparison is a timestamp/date range. The field type is the reliable signal:
+     * {@code literal.isDateTime()} reads the literal's UDT, which {@link
+     * org.apache.calcite.rex.RexSimplify} can strip (to VARCHAR) when a sibling clause is folded
+     * into a {@code Sarg}, e.g. {@code @timestamp > X AND severityText IN (...)}. Falling back to
+     * {@code rel.isTimeStampType()} keeps ISO-8601 normalization and the {@code "date_time"} format
+     * hint on the range query.
+     */
     private boolean isFieldOrLiteralDateTime(LiteralExpression literal) {
       return literal.isDateTime() || (rel != null && rel.isTimeStampType());
     }
 
-    private Object endpointValue(LiteralExpression literal, boolean isTimeStamp) {
-      if (!isTimeStamp || literal.isDateTime()) {
-        return literal.value();
-      }
-      return timestampValueForPushDown(literal.value().toString());
-    }
-
     @Override
     public QueryExpression match(String query, Map<String, String> optionalArguments) {
       builder = new MatchQuery().build(getFieldReference(), query, optionalArguments);
@@ -1607,6 +1602,11 @@ public QueryExpression between(Range<?> range, boolean isTimeStamp) {
 
     private Object convertEndpointValue(Object value, boolean isTimeStamp) {
       value = sargPointValue(value);
+      // Shared normalization entry point: a Sarg endpoint can normalize to null, and
+      // timestampValueForPushDown(value.toString()) would otherwise NPE.
+      if (value == null) {
+        return null;
+      }
       return isTimeStamp ? timestampValueForPushDown(value.toString()) : value;
     }
   }
@@ -1738,16 +1738,19 @@ public static ScriptSortBuilder.ScriptSortType getScriptSortType(RelDataType rel
   }
 
   /**
-   * By default, range queries on date/time need use the format of the source to parse the literal.
-   * So we need to specify that the literal has "date_time" format
+   * Range queries on date/time fields need the source format to parse the literal, so we attach the
+   * {@code "date_time"} format. The caller resolves whether the comparison is a timestamp range
+   * from the field type (see {@link SimpleQueryExpression#isFieldOrLiteralDateTime}) rather than
+   * the literal's UDT, which {@link org.apache.calcite.rex.RexSimplify} can strip when a sibling
+   * clause is folded into a {@code Sarg}.
    *
-   * @param literal literal value
-   * @param rangeQueryBuilder query builder to optionally add {@code format} expression
-   * @return existing builder with possible {@code format} attribute
+   * @param isTimeStamp whether the comparison endpoint is a timestamp/date range endpoint
+   * @param rangeQueryBuilder query builder to optionally add the {@code format} attribute
+   * @return the same builder, with {@code format("date_time")} added when {@code isTimeStamp}
    */
   private static RangeQueryBuilder addFormatIfNecessary(
-      LiteralExpression literal, RangeQueryBuilder rangeQueryBuilder) {
-    if (literal.isDateTime()) {
+      boolean isTimeStamp, RangeQueryBuilder rangeQueryBuilder) {
+    if (isTimeStamp) {
       rangeQueryBuilder.format("date_time");
     }
     return rangeQueryBuilder;
diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/request/PredicateAnalyzerTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/request/PredicateAnalyzerTest.java
@@ -1178,6 +1178,107 @@ void gt_normalizesVarcharLiteralAgainstTimestampField() throws ExpressionNotAnal
         result.toString());
   }
 
+  // Companion stripped-VARCHAR-literal tests for the remaining range shapes (equals -> gte+lte,
+  // notEquals -> two-should bool, lte -> single range). Each must produce the same DSL as its
+  // intact-UDT counterpart, proving the field-type fallback in isFieldOrLiteralDateTime keeps
+  // ISO-8601 normalization + format("date_time") on every comparison op, not just gt. See #5481.
+  @Test
+  void equals_normalizesVarcharLiteralAgainstTimestampField()
+      throws ExpressionNotAnalyzableException {
+    RexLiteral varcharLiteral = (RexLiteral) builder.makeLiteral("1987-02-03 04:34:56");
+    RexNode call = builder.makeCall(SqlStdOperatorTable.EQUALS, field4, varcharLiteral);
+    QueryBuilder result = PredicateAnalyzer.analyze(call, schema, fieldTypes);
+
+    assertInstanceOf(RangeQueryBuilder.class, result);
+    assertEquals(
+        """
+        {
+          "range" : {
+            "d" : {
+              "from" : "1987-02-03T04:34:56.000Z",
+              "to" : "1987-02-03T04:34:56.000Z",
+              "include_lower" : true,
+              "include_upper" : true,
+              "format" : "date_time",
+              "boost" : 1.0
+            }
+          }
+        }\
+        """,
+        result.toString());
+  }
+
+  @Test
+  void notEquals_normalizesVarcharLiteralAgainstTimestampField()
+      throws ExpressionNotAnalyzableException {
+    RexLiteral varcharLiteral = (RexLiteral) builder.makeLiteral("1987-02-03 04:34:56");
+    RexNode call = builder.makeCall(SqlStdOperatorTable.NOT_EQUALS, field4, varcharLiteral);
+    QueryBuilder result = PredicateAnalyzer.analyze(call, schema, fieldTypes);
+
+    assertInstanceOf(BoolQueryBuilder.class, result);
+    assertEquals(
+        """
+        {
+          "bool" : {
+            "should" : [
+              {
+                "range" : {
+                  "d" : {
+                    "from" : "1987-02-03T04:34:56.000Z",
+                    "to" : null,
+                    "include_lower" : false,
+                    "include_upper" : true,
+                    "format" : "date_time",
+                    "boost" : 1.0
+                  }
+                }
+              },
+              {
+                "range" : {
+                  "d" : {
+                    "from" : null,
+                    "to" : "1987-02-03T04:34:56.000Z",
+                    "include_lower" : true,
+                    "include_upper" : false,
+                    "format" : "date_time",
+                    "boost" : 1.0
+                  }
+                }
+              }
+            ],
+            "adjust_pure_negative" : true,
+            "boost" : 1.0
+          }
+        }\
+        """,
+        result.toString());
+  }
+
+  @Test
+  void lte_normalizesVarcharLiteralAgainstTimestampField() throws ExpressionNotAnalyzableException {
+    RexLiteral varcharLiteral = (RexLiteral) builder.makeLiteral("1987-02-03 04:34:56");
+    RexNode call = builder.makeCall(SqlStdOperatorTable.LESS_THAN_OR_EQUAL, field4, varcharLiteral);
+    QueryBuilder result = PredicateAnalyzer.analyze(call, schema, fieldTypes);
+
+    assertInstanceOf(RangeQueryBuilder.class, result);
+    assertEquals(
+        """
+        {
+          "range" : {
+            "d" : {
+              "from" : null,
+              "to" : "1987-02-03T04:34:56.000Z",
+              "include_lower" : true,
+              "include_upper" : true,
+              "format" : "date_time",
+              "boost" : 1.0
+            }
+          }
+        }\
+        """,
+        result.toString());
+  }
+
   @Test
   void gte_generatesRangeQueryWithFormatForDateTime() throws ExpressionNotAnalyzableException {
     RexNode call =
