Change how datetime values are processed in terms bucket aggregation.

* Revert recent changes in `OpenSearchExprValueFactory`. * Update `BucketAggregationBuilder` to specify how to interpret datetime values. Signed-off-by: Yury-Fridlyand <[email protected]>
opensearch-project · Jan 9, 2023 · 3a902ef · 3a902ef
1 parent af13d83
commit 3a902ef
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 19 deletions.
diff --git a/...ch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java b/...ch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java
@@ -175,26 +175,27 @@ private Optional<ExprType> type(String field) {
    * https://www.elastic.co/guide/en/elasticsearch/reference/current/date_nanos.html
    * The customized date_format is not supported.
    */
-  private ExprValue parseTimestamp(Content value) {
-    if (value.isString()) {
-      // value may contain epoch millis as a string, trying to extract it
-      try {
-        return parseTimestamp(new ObjectContent(Long.parseLong(value.stringValue())));
-      } catch (NumberFormatException ignored) { /* nothing to do, try another format */ }
-      try {
-        return new ExprTimestampValue(
-            // Using OpenSearch DateFormatters for now.
-            DateFormatters.from(DATE_TIME_FORMATTER.parse(value.stringValue())).toInstant());
-      } catch (DateTimeParseException e) {
-        throw new IllegalStateException(String.format(
-            "Construct ExprTimestampValue from \"%s\" failed, unsupported date format.",
-            value.stringValue()), e);
-      }
+  private ExprValue constructTimestamp(String value) {
+    try {
+      return new ExprTimestampValue(
+          // Using OpenSearch DateFormatters for now.
+          DateFormatters.from(DATE_TIME_FORMATTER.parse(value)).toInstant());
+    } catch (DateTimeParseException e) {
+      throw new IllegalStateException(
+          String.format(
+              "Construct ExprTimestampValue from \"%s\" failed, unsupported date format.", value),
+          e);
     }
+  }
+
+  private ExprValue parseTimestamp(Content value) {
     if (value.isNumber()) {
       return new ExprTimestampValue(Instant.ofEpochMilli(value.longValue()));
+    } else if (value.isString()) {
+      return constructTimestamp(value.stringValue());
+    } else {
+      return new ExprTimestampValue((Instant) value.objectValue());
     }
-    return new ExprTimestampValue((Instant) value.objectValue());
   }
 
   private ExprValue parseStruct(Content content, String prefix) {

diff --git a/...rg/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilder.java b/...rg/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilder.java
@@ -5,6 +5,11 @@
 
 package org.opensearch.sql.opensearch.storage.script.aggregation.dsl;
 
+import static org.opensearch.sql.data.type.ExprCoreType.DATE;
+import static org.opensearch.sql.data.type.ExprCoreType.DATETIME;
+import static org.opensearch.sql.data.type.ExprCoreType.TIME;
+import static org.opensearch.sql.data.type.ExprCoreType.TIMESTAMP;
+
 import com.google.common.collect.ImmutableList;
 import java.util.List;
 import org.apache.commons.lang3.tuple.Triple;
@@ -14,6 +19,7 @@
 import org.opensearch.search.aggregations.bucket.composite.TermsValuesSourceBuilder;
 import org.opensearch.search.aggregations.bucket.histogram.DateHistogramInterval;
 import org.opensearch.search.aggregations.bucket.missing.MissingOrder;
+import org.opensearch.search.aggregations.support.ValueType;
 import org.opensearch.search.sort.SortOrder;
 import org.opensearch.sql.ast.expression.SpanUnit;
 import org.opensearch.sql.expression.NamedExpression;
@@ -64,6 +70,10 @@ private CompositeValuesSourceBuilder<?> buildCompositeValuesSourceBuilder(
               .missingBucket(true)
               .missingOrder(missingOrder)
               .order(sortOrder);
+      // Time types values are converted to LONG in ExpressionAggregationScript::execute
+      if (List.of(TIMESTAMP, TIME, DATE, DATETIME).contains(expr.getDelegated().type())) {
+        sourceBuilder.userValuetypeHint(ValueType.LONG);
+      }
       return helper.build(expr.getDelegated(), sourceBuilder::field, sourceBuilder::script);
     }
   }

diff --git a/...rc/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java b/...rc/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java
@@ -197,9 +197,6 @@ public void constructDate() {
     assertEquals(
         new ExprTimestampValue(Instant.ofEpochMilli(1420070400001L)),
         constructFromObject("timestampV", 1420070400001L));
-    assertEquals(
-        new ExprTimestampValue(Instant.ofEpochMilli(1420070400001L)),
-        constructFromObject("timestampV", "1420070400001"));
     assertEquals(
         new ExprTimestampValue(Instant.ofEpochMilli(1420070400001L)),
         constructFromObject("timestampV", Instant.ofEpochMilli(1420070400001L)));

diff --git a/...pensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilderTest.java b/...pensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilderTest.java
@@ -9,8 +9,12 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.mockito.Mockito.when;
 import static org.opensearch.common.xcontent.ToXContent.EMPTY_PARAMS;
+import static org.opensearch.sql.data.type.ExprCoreType.DATE;
+import static org.opensearch.sql.data.type.ExprCoreType.DATETIME;
 import static org.opensearch.sql.data.type.ExprCoreType.INTEGER;
 import static org.opensearch.sql.data.type.ExprCoreType.STRING;
+import static org.opensearch.sql.data.type.ExprCoreType.TIME;
+import static org.opensearch.sql.data.type.ExprCoreType.TIMESTAMP;
 import static org.opensearch.sql.expression.DSL.named;
 import static org.opensearch.sql.expression.DSL.ref;
 import static org.opensearch.sql.opensearch.data.type.OpenSearchDataType.OPENSEARCH_TEXT_KEYWORD;
@@ -24,6 +28,9 @@
 import org.junit.jupiter.api.DisplayNameGenerator;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.EnumSource;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
 import org.opensearch.common.bytes.BytesReference;
@@ -33,6 +40,8 @@
 import org.opensearch.search.aggregations.bucket.composite.CompositeValuesSourceBuilder;
 import org.opensearch.search.aggregations.bucket.missing.MissingOrder;
 import org.opensearch.search.sort.SortOrder;
+import org.opensearch.sql.data.type.ExprCoreType;
+import org.opensearch.sql.data.type.ExprType;
 import org.opensearch.sql.expression.DSL;
 import org.opensearch.sql.expression.NamedExpression;
 import org.opensearch.sql.expression.parse.ParseExpression;
@@ -106,6 +115,24 @@ void should_build_bucket_with_parse_expression() {
                 asc(named("name", parseExpression)))));
   }
 
+  @ParameterizedTest(name = "{0}")
+  @EnumSource(value = ExprCoreType.class, names = {"TIMESTAMP", "TIME", "DATE", "DATETIME"})
+  void terms_bucket_for_datetime_types_uses_long(ExprType dataType) {
+    assertEquals(
+        "{\n"
+            + "  \"terms\" : {\n"
+            + "    \"field\" : \"date\",\n"
+            + "    \"missing_bucket\" : true,\n"
+            + "    \"value_type\" : \"long\",\n"
+            + "    \"missing_order\" : \"first\",\n"
+            + "    \"order\" : \"asc\"\n"
+            + "  }\n"
+            + "}",
+        buildQuery(
+            Arrays.asList(
+                asc(named("date", ref("date", dataType))))));
+  }
+
   @SneakyThrows
   private String buildQuery(
       List<Triple<NamedExpression, SortOrder, MissingOrder>> groupByExpressions) {