From 3a902ef7677e942f3c55b90ea44f303111aa416e Mon Sep 17 00:00:00 2001 From: Yury-Fridlyand Date: Mon, 9 Jan 2023 10:08:25 -0800 Subject: [PATCH] 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 --- .../value/OpenSearchExprValueFactory.java | 33 ++++++++++--------- .../dsl/BucketAggregationBuilder.java | 10 ++++++ .../value/OpenSearchExprValueFactoryTest.java | 3 -- .../dsl/BucketAggregationBuilderTest.java | 27 +++++++++++++++ 4 files changed, 54 insertions(+), 19 deletions(-) diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java index 15d2073b11..2536121e91 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java @@ -175,26 +175,27 @@ private Optional 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/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilder.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilder.java index 8ef8a5e4a8..1a6a82be96 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilder.java +++ b/opensearch/src/main/java/org/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/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java index 21d76d9a91..8d5552d6a8 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java +++ b/opensearch/src/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/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilderTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilderTest.java index 9f21a145ed..e5f3c8eab6 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/script/aggregation/dsl/BucketAggregationBuilderTest.java +++ b/opensearch/src/test/java/org/opensearch/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> groupByExpressions) {