From 699fd7aee904c5949c4a232dc1defb8ffd700a27 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Tue, 21 Oct 2025 02:50:25 +0000 Subject: [PATCH 1/2] Support refering to implicit `@timestamp` field in span (#4138) * Support refering to implicit @timestamp field in time-based aggregations Signed-off-by: Yuanchun Shen * Update documentation of stats to reflect that span can be used without specifying a field Signed-off-by: Yuanchun Shen * Move @timestamp reference to AST layer - Additionally refactored visitTimechartCommand to reuse spanLiteral definition Signed-off-by: Yuanchun Shen * Unit test visitSpanLiteral, vistSpanClause, and visitTimechartParamter Signed-off-by: Yuanchun Shen * Revert changes to Span will always have a field with the current implementation Signed-off-by: Yuanchun Shen * Throw exception for zero span Signed-off-by: Yuanchun Shen --------- Signed-off-by: Yuanchun Shen (cherry picked from commit c30d5d01c36b7c959d7b2a290c319c4a8f0e8367) Signed-off-by: github-actions[bot] --- .../org/opensearch/sql/ast/dsl/AstDSL.java | 14 +- docs/user/ppl/cmd/stats.rst | 21 +- .../remote/CalcitePPLAggregationIT.java | 21 ++ .../rest-api-spec/test/issues/4527.yml | 50 ++++ ppl/src/main/antlr/OpenSearchPPLParser.g4 | 8 +- .../opensearch/sql/ppl/parser/AstBuilder.java | 39 +--- .../sql/ppl/parser/AstExpressionBuilder.java | 43 +++- .../ppl/calcite/CalcitePPLTimechartTest.java | 8 + .../ppl/parser/AstExpressionBuilderTest.java | 221 ++++++++++++++++++ 9 files changed, 384 insertions(+), 41 deletions(-) create mode 100644 integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4527.yml diff --git a/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java b/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java index 9cdd2046df2..a0f5b4a4369 100644 --- a/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java +++ b/core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java @@ -80,6 +80,7 @@ import org.opensearch.sql.ast.tree.Trendline; import org.opensearch.sql.ast.tree.UnresolvedPlan; import org.opensearch.sql.ast.tree.Values; +import org.opensearch.sql.calcite.plan.OpenSearchConstants; /** Class of static methods to create specific node instances. */ @UtilityClass @@ -491,8 +492,8 @@ public static Span spanFromSpanLengthLiteral( UnresolvedExpression field, Literal spanLengthLiteral) { if (spanLengthLiteral.getType() == DataType.STRING) { String spanText = spanLengthLiteral.getValue().toString(); - String valueStr = spanText.replaceAll("[^0-9]", ""); - String unitStr = spanText.replaceAll("[0-9]", ""); + String valueStr = spanText.replaceAll("[^0-9-]", ""); + String unitStr = spanText.replaceAll("[0-9-]", ""); if (valueStr.isEmpty()) { // No numeric value found, use the literal as-is @@ -500,6 +501,10 @@ public static Span spanFromSpanLengthLiteral( } else { // Parse numeric value and unit Integer value = Integer.parseInt(valueStr); + if (value <= 0) { + throw new IllegalArgumentException( + String.format("Zero or negative time interval not supported: %s", spanText)); + } SpanUnit unit = unitStr.isEmpty() ? SpanUnit.NONE : SpanUnit.of(unitStr); return span(field, intLiteral(value), unit); } @@ -714,4 +719,9 @@ public static Bin bin(UnresolvedExpression field, Argument... arguments) { return DefaultBin.builder().field(field).alias(alias).build(); } } + + /** Get a reference to the implicit timestamp field {@code @timestamp} */ + public static Field referImplicitTimestampField() { + return AstDSL.field(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP); + } } diff --git a/docs/user/ppl/cmd/stats.rst b/docs/user/ppl/cmd/stats.rst index 8541f86288c..10aa40e5169 100644 --- a/docs/user/ppl/cmd/stats.rst +++ b/docs/user/ppl/cmd/stats.rst @@ -58,8 +58,8 @@ stats [bucket_nullable=bool] ... [by-clause] * span-expression: optional, at most one. - * Syntax: span(field_expr, interval_expr) - * Description: The unit of the interval expression is the natural unit by default. **If the field is a date/time type field, the aggregation results always ignore null bucket**. And the interval is in date/time units, you will need to specify the unit in the interval expression. For example, to split the field ``age`` into buckets by 10 years, it looks like ``span(age, 10)``. And here is another example of time span, the span to split a ``timestamp`` field into hourly intervals, it looks like ``span(timestamp, 1h)``. + * Syntax: span([field_expr,] interval_expr) + * Description: The unit of the interval expression is the natural unit by default. If ``field_expr`` is omitted, span will use the implicit ``@timestamp`` field. An error will be thrown if this field doesn't exist. **If the field is a date/time type field, the aggregation results always ignore null bucket**. And the interval is in date/time units, you will need to specify the unit in the interval expression. For example, to split the field ``age`` into buckets by 10 years, it looks like ``span(age, 10)``. And here is another example of time span, the span to split a ``timestamp`` field into hourly intervals, it looks like ``span(timestamp, 1h)``. * Available time unit: +----------------------------+ @@ -578,7 +578,7 @@ Description Version: 3.3.0 (Calcite engine only) -Usage: LIST(expr). Collects all values from the specified expression into an array. Values are converted to strings, nulls are filtered, and duplicates are preserved. +Usage: LIST(expr). Collects all values from the specified expression into an array. Values are converted to strings, nulls are filtered, and duplicates are preserved. The function returns up to 100 values with no guaranteed ordering. * expr: The field expression to collect values from. @@ -976,3 +976,18 @@ PPL query:: | 1 | 2025-01-01 | 2 | +-----+------------+--------+ + +Example 18: Calculate the count by the implicit @timestamp field +================================================================ + +This example demonstrates that if you omit the field parameter in the span function, it will automatically use the implicit ``@timestamp`` field. + +PPL query:: + + PPL> source=big5 | stats count() by span(1month) + fetched rows / total rows = 1/1 + +---------+---------------------+ + | count() | span(1month) | + |---------+---------------------| + | 1 | 2023-01-01 00:00:00 | + +---------+---------------------+ diff --git a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLAggregationIT.java b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLAggregationIT.java index 4c4500abf56..69add59c7f4 100644 --- a/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLAggregationIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLAggregationIT.java @@ -16,6 +16,7 @@ import static org.opensearch.sql.util.MatcherUtils.rows; import static org.opensearch.sql.util.MatcherUtils.schema; import static org.opensearch.sql.util.MatcherUtils.verifyDataRows; +import static org.opensearch.sql.util.MatcherUtils.verifyErrorMessageContains; import static org.opensearch.sql.util.MatcherUtils.verifySchema; import static org.opensearch.sql.util.MatcherUtils.verifySchemaInOrder; @@ -25,6 +26,8 @@ import org.json.JSONObject; import org.junit.jupiter.api.Test; import org.opensearch.client.Request; +import org.opensearch.sql.common.utils.StringUtils; +import org.opensearch.sql.exception.SemanticCheckException; import org.opensearch.sql.ppl.PPLIntegTestCase; public class CalcitePPLAggregationIT extends PPLIntegTestCase { @@ -41,6 +44,7 @@ public void init() throws Exception { loadIndex(Index.CALCS); loadIndex(Index.DATE_FORMATS); loadIndex(Index.DATA_TYPE_NUMERIC); + loadIndex(Index.BIG5); loadIndex(Index.LOGS); loadIndex(Index.TELEMETRY); loadIndex(Index.TIME_TEST_DATA); @@ -729,6 +733,23 @@ public void testCountBySpanForCustomFormats() throws IOException { verifyDataRows(actual, rows(1, "00:00:00"), rows(1, "12:00:00")); } + // Only available in v3 with Calcite + @Test + public void testSpanByImplicitTimestamp() throws IOException { + JSONObject result = executeQuery("source=big5 | stats count() by span(1d) as span"); + verifySchema(result, schema("count()", "bigint"), schema("span", "timestamp")); + verifyDataRows(result, rows(1, "2023-01-02 00:00:00")); + + Throwable t = + assertThrowsWithReplace( + SemanticCheckException.class, + () -> + executeQuery( + StringUtils.format( + "source=%s | stats count() by span(5m)", TEST_INDEX_DATE_FORMATS))); + verifyErrorMessageContains(t, "Field [@timestamp] not found"); + } + @Test public void testCountDistinct() throws IOException { JSONObject actual = diff --git a/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4527.yml b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4527.yml new file mode 100644 index 00000000000..048f300098d --- /dev/null +++ b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4527.yml @@ -0,0 +1,50 @@ +setup: + - do: + query.settings: + body: + transient: + plugins.calcite.enabled : true + - do: + indices.create: + index: test_timechart_span_validation + body: + mappings: + properties: + "@timestamp": + type: date_nanos + packets: + type: long + - do: + bulk: + index: test_timechart_span_validation + refresh: true + body: + - '{"index": {}}' + - '{"@timestamp": "2024-01-15T10:30:04.567890123Z", "packets": 100}' + - '{"index": {}}' + - '{"@timestamp": "2024-01-15T10:31:04.567890123Z", "packets": 150}' + - '{"index": {}}' + - '{"@timestamp": "2024-01-15T10:32:04.567890123Z", "packets": 120}' + +--- +teardown: + - do: + query.settings: + body: + transient: + plugins.calcite.enabled : false + +--- +"timechart with zero span should return validation error": + - skip: + features: + - headers + - allowed_warnings + - do: + catch: bad_request + headers: + Content-Type: 'application/json' + ppl: + body: + query: source=test_timechart_span_validation | timechart span=0m per_second(packets) + - match: {"$body": "/Zero\\s+or\\s+negative\\s+time\\s+interval\\s+not\\s+supported/"} diff --git a/ppl/src/main/antlr/OpenSearchPPLParser.g4 b/ppl/src/main/antlr/OpenSearchPPLParser.g4 index 62e2a21204b..9467ffc130d 100644 --- a/ppl/src/main/antlr/OpenSearchPPLParser.g4 +++ b/ppl/src/main/antlr/OpenSearchPPLParser.g4 @@ -257,12 +257,8 @@ timechartCommand ; timechartParameter - : (spanClause | SPAN EQUAL spanLiteral) - | timechartArg - ; - -timechartArg : LIMIT EQUAL integerLiteral + | SPAN EQUAL spanLiteral | USEOTHER EQUAL (booleanLiteral | ident) ; @@ -621,7 +617,7 @@ bySpanClause ; spanClause - : SPAN LT_PRTHS fieldExpression COMMA value = spanLiteral RT_PRTHS + : SPAN LT_PRTHS (fieldExpression COMMA)? value = spanLiteral RT_PRTHS ; sortbyClause diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java index 6d4f5382f5a..87267ff886d 100644 --- a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java +++ b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java @@ -601,39 +601,20 @@ public UnresolvedPlan visitReverseCommand(OpenSearchPPLParser.ReverseCommandCont @Override public UnresolvedPlan visitTimechartCommand(OpenSearchPPLParser.TimechartCommandContext ctx) { UnresolvedExpression binExpression = - AstDSL.span(AstDSL.field("@timestamp"), AstDSL.intLiteral(1), SpanUnit.of("m")); + AstDSL.span(AstDSL.referImplicitTimestampField(), AstDSL.intLiteral(1), SpanUnit.m); Integer limit = 10; Boolean useOther = true; // Process timechart parameters for (OpenSearchPPLParser.TimechartParameterContext paramCtx : ctx.timechartParameter()) { - if (paramCtx.spanClause() != null) { - binExpression = internalVisitExpression(paramCtx.spanClause()); - } else if (paramCtx.spanLiteral() != null) { - Literal literal = (Literal) internalVisitExpression(paramCtx.spanLiteral()); - binExpression = AstDSL.spanFromSpanLengthLiteral(AstDSL.field("@timestamp"), literal); - } else if (paramCtx.timechartArg() != null) { - OpenSearchPPLParser.TimechartArgContext argCtx = paramCtx.timechartArg(); - if (argCtx.LIMIT() != null && argCtx.integerLiteral() != null) { - limit = Integer.parseInt(argCtx.integerLiteral().getText()); - if (limit < 0) { - throw new IllegalArgumentException("Limit must be a non-negative number"); - } - } else if (argCtx.USEOTHER() != null) { - if (argCtx.booleanLiteral() != null) { - useOther = Boolean.parseBoolean(argCtx.booleanLiteral().getText()); - } else if (argCtx.ident() != null) { - String useOtherValue = argCtx.ident().getText().toLowerCase(); - if ("true".equals(useOtherValue) || "t".equals(useOtherValue)) { - useOther = true; - } else if ("false".equals(useOtherValue) || "f".equals(useOtherValue)) { - useOther = false; - } else { - throw new IllegalArgumentException( - "Invalid useOther value: " - + argCtx.ident().getText() - + ". Expected true/false or t/f"); - } - } + UnresolvedExpression param = internalVisitExpression(paramCtx); + if (param instanceof Span) { + binExpression = param; + } else if (param instanceof Literal literal) { + if (DataType.BOOLEAN.equals(literal.getType())) { + useOther = (Boolean) literal.getValue(); + } else if (DataType.INTEGER.equals(literal.getType()) + || DataType.LONG.equals(literal.getType())) { + limit = (Integer) literal.getValue(); } } } diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java index a889415792d..6b63b6ba3ca 100644 --- a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java +++ b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java @@ -636,7 +636,7 @@ public UnresolvedExpression visitSpanClause(SpanClauseContext ctx) { if (ctx.fieldExpression() != null) { fieldExpression = visit(ctx.fieldExpression()); } else { - fieldExpression = AstDSL.field("@timestamp"); + fieldExpression = AstDSL.referImplicitTimestampField(); } Literal literal = (Literal) visit(ctx.value); return AstDSL.spanFromSpanLengthLiteral(fieldExpression, literal); @@ -934,6 +934,47 @@ public UnresolvedExpression visitTimeModifierValue( return AstDSL.stringLiteral(osDateMathExpression); } + @Override + public UnresolvedExpression visitTimechartParameter( + OpenSearchPPLParser.TimechartParameterContext ctx) { + UnresolvedExpression timechartParameter; + if (ctx.SPAN() != null) { + // Convert span=1h to span(@timestamp, 1h) + Literal spanLiteral = (Literal) visit(ctx.spanLiteral()); + timechartParameter = + AstDSL.spanFromSpanLengthLiteral(AstDSL.referImplicitTimestampField(), spanLiteral); + } else if (ctx.LIMIT() != null) { + Literal limit = (Literal) visit(ctx.integerLiteral()); + if ((Integer) limit.getValue() < 0) { + throw new IllegalArgumentException("Limit must be a non-negative number"); + } + timechartParameter = limit; + } else if (ctx.USEOTHER() != null) { + UnresolvedExpression useOther; + if (ctx.booleanLiteral() != null) { + useOther = visit(ctx.booleanLiteral()); + } else if (ctx.ident() != null) { + QualifiedName ident = visitIdentifiers(List.of(ctx.ident())); + String useOtherValue = ident.toString(); + if ("true".equalsIgnoreCase(useOtherValue) || "t".equalsIgnoreCase(useOtherValue)) { + useOther = AstDSL.booleanLiteral(true); + } else if ("false".equalsIgnoreCase(useOtherValue) || "f".equalsIgnoreCase(useOtherValue)) { + useOther = AstDSL.booleanLiteral(false); + } else { + throw new IllegalArgumentException( + "Invalid useOther value: " + ctx.ident().getText() + ". Expected true/false or t/f"); + } + } else { + throw new IllegalArgumentException("value for useOther must be a boolean or identifier"); + } + timechartParameter = useOther; + } else { + throw new IllegalArgumentException( + String.format("A parameter of timechart must be a span, limit or useOther, got %s", ctx)); + } + return timechartParameter; + } + /** * Process time range expressions (EARLIEST='value' or LATEST='value') It creates a Comparison * filter like @timestamp >= timeModifierValue diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLTimechartTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLTimechartTest.java index 356a2b0cede..6e03447e243 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLTimechartTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLTimechartTest.java @@ -6,6 +6,7 @@ package org.opensearch.sql.ppl.calcite; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThrows; import com.google.common.collect.ImmutableList; import java.util.List; @@ -342,6 +343,13 @@ public void testTimechartWithUseOtherBeforeLimit() { assertNotNull(plan); } + @Test + public void testTimechartUsingZeroSpanShouldThrow() { + String ppl = "source=events | timechart span=0h limit=5 count() by host"; + Throwable t = assertThrows(IllegalArgumentException.class, () -> parsePPL(ppl)); + verifyErrorMessageContains(t, "Zero or negative time interval not supported: 0h"); + } + private UnresolvedPlan parsePPL(String query) { PPLSyntaxParser parser = new PPLSyntaxParser(); AstBuilder astBuilder = new AstBuilder(query); diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java index 55204e61043..837c56df4b4 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/parser/AstExpressionBuilderTest.java @@ -10,6 +10,7 @@ import static org.opensearch.sql.ast.dsl.AstDSL.agg; import static org.opensearch.sql.ast.dsl.AstDSL.aggregate; import static org.opensearch.sql.ast.dsl.AstDSL.alias; +import static org.opensearch.sql.ast.dsl.AstDSL.allFields; import static org.opensearch.sql.ast.dsl.AstDSL.and; import static org.opensearch.sql.ast.dsl.AstDSL.argument; import static org.opensearch.sql.ast.dsl.AstDSL.booleanLiteral; @@ -42,6 +43,7 @@ import static org.opensearch.sql.ast.dsl.AstDSL.relation; import static org.opensearch.sql.ast.dsl.AstDSL.search; import static org.opensearch.sql.ast.dsl.AstDSL.sort; +import static org.opensearch.sql.ast.dsl.AstDSL.span; import static org.opensearch.sql.ast.dsl.AstDSL.stringLiteral; import static org.opensearch.sql.ast.dsl.AstDSL.unresolvedArg; import static org.opensearch.sql.ast.dsl.AstDSL.when; @@ -58,6 +60,9 @@ import org.opensearch.sql.ast.expression.AllFields; import org.opensearch.sql.ast.expression.DataType; import org.opensearch.sql.ast.expression.RelevanceFieldList; +import org.opensearch.sql.ast.expression.SpanUnit; +import org.opensearch.sql.ast.tree.Timechart; +import org.opensearch.sql.calcite.plan.OpenSearchConstants; import org.opensearch.sql.common.antlr.SyntaxCheckException; public class AstExpressionBuilderTest extends AstBuilderTest { @@ -1383,4 +1388,220 @@ public void testTimeModifierEarliestWithStringValue() { "source=t earliest='2025-12-10 14:00:00'", search(relation("t"), "@timestamp:>=2025\\-12\\-10T14\\:00\\:00Z")); } + + @Test + public void testTimechartSpanParameter() { + assertEqual( + "source=t | timechart span=30m count()", + Timechart.builder() + .child(relation("t")) + .binExpression( + span( + field(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP), + intLiteral(30), + SpanUnit.m)) + .aggregateFunction(aggregate("count", allFields())) + .limit(10) + .useOther(true) + .build()); + } + + @Test + public void testTimechartLimitParameter() { + assertEqual( + "source=t | timechart limit=100 count()", + Timechart.builder() + .child(relation("t")) + .binExpression( + span( + field(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP), intLiteral(1), SpanUnit.m)) + .aggregateFunction(aggregate("count", allFields())) + .limit(100) + .useOther(true) + .build()); + } + + @Test + public void testTimechartNegativeLimitParameter() { + assertThrows( + IllegalArgumentException.class, + () -> assertEqual("source=t | timechart limit=-1 count()", (Node) null)); + } + + @Test + public void testTimechartUseOtherWithBooleanLiteral() { + assertEqual( + "source=t | timechart useother=true count()", + Timechart.builder() + .child(relation("t")) + .binExpression( + span( + field(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP), intLiteral(1), SpanUnit.m)) + .aggregateFunction(aggregate("count", allFields())) + .limit(10) + .useOther(true) + .build()); + + assertEqual( + "source=t | timechart useother=false count()", + Timechart.builder() + .child(relation("t")) + .binExpression( + span( + field(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP), intLiteral(1), SpanUnit.m)) + .aggregateFunction(aggregate("count", allFields())) + .limit(10) + .useOther(false) + .build()); + } + + @Test + public void testTimechartUseOtherWithIdentifier() { + assertEqual( + "source=t | timechart useother=t count()", + Timechart.builder() + .child(relation("t")) + .binExpression( + span( + field(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP), intLiteral(1), SpanUnit.m)) + .aggregateFunction(aggregate("count", allFields())) + .limit(10) + .useOther(true) + .build()); + + assertEqual( + "source=t | timechart useother=f count()", + Timechart.builder() + .child(relation("t")) + .binExpression( + span( + field(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP), intLiteral(1), SpanUnit.m)) + .aggregateFunction(aggregate("count", allFields())) + .limit(10) + .useOther(false) + .build()); + + assertEqual( + "source=t | timechart useother=TRUE count()", + Timechart.builder() + .child(relation("t")) + .binExpression( + span( + field(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP), intLiteral(1), SpanUnit.m)) + .aggregateFunction(aggregate("count", allFields())) + .limit(10) + .useOther(true) + .build()); + + assertEqual( + "source=t | timechart useother=FALSE count()", + Timechart.builder() + .child(relation("t")) + .binExpression( + span( + field(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP), intLiteral(1), SpanUnit.m)) + .aggregateFunction(aggregate("count", allFields())) + .limit(10) + .useOther(false) + .build()); + } + + @Test + public void testTimechartInvalidUseOtherValue() { + assertThrows( + IllegalArgumentException.class, + () -> assertEqual("source=t | timechart useother=invalid count()", (Node) null)); + } + + @Test + public void testTimechartInvalidParameter() { + assertThrows( + SyntaxCheckException.class, + () -> assertEqual("source=t | timechart invalidparam=value count()", (Node) null)); + } + + @Test + public void testVisitSpanClause() { + // Test span clause with explicit field + assertEqual( + "source=t | stats count() by span(timestamp, 1h)", + agg( + relation("t"), + exprList(alias("count()", aggregate("count", AllFields.of()))), + emptyList(), + emptyList(), + alias("span(timestamp,1h)", span(field("timestamp"), intLiteral(1), SpanUnit.H)), + defaultStatsArgs())); + + // Test span clause with different time unit + assertEqual( + "source=t | stats count() by span(timestamp, 5d)", + agg( + relation("t"), + exprList(alias("count()", aggregate("count", AllFields.of()))), + emptyList(), + emptyList(), + alias("span(timestamp,5d)", span(field("timestamp"), intLiteral(5), SpanUnit.D)), + defaultStatsArgs())); + + // Test span clause with implicit @timestamp field + assertEqual( + "source=t | stats count() by span(1m)", + agg( + relation("t"), + exprList(alias("count()", aggregate("count", AllFields.of()))), + emptyList(), + emptyList(), + alias( + "span(1m)", + span( + field(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP), + intLiteral(1), + SpanUnit.m)), + defaultStatsArgs())); + } + + @Test + public void testVisitSpanLiteral() { + // Test span literal with integer value and hour unit + assertEqual( + "source=t | timechart span=1h count()", + Timechart.builder() + .child(relation("t")) + .binExpression( + span( + field(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP), intLiteral(1), SpanUnit.H)) + .aggregateFunction(aggregate("count", allFields())) + .limit(10) + .useOther(true) + .build()); + + // Test span literal with decimal value and minute unit + assertEqual( + "source=t | timechart span=2m count()", + Timechart.builder() + .child(relation("t")) + .binExpression( + span( + field(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP), intLiteral(2), SpanUnit.m)) + .aggregateFunction(aggregate("count", allFields())) + .limit(10) + .useOther(true) + .build()); + + // Test span literal without unit (should use NONE unit) + assertEqual( + "source=t | timechart span=10 count()", + Timechart.builder() + .child(relation("t")) + .binExpression( + span( + field(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP), + intLiteral(10), + SpanUnit.NONE)) + .aggregateFunction(aggregate("count", allFields())) + .limit(10) + .useOther(true) + .build()); + } } From 222d808548990559c678fab51e286a0e395442d4 Mon Sep 17 00:00:00 2001 From: Yuanchun Shen Date: Tue, 21 Oct 2025 14:16:48 +0800 Subject: [PATCH 2/2] Change pattern variable to cast Signed-off-by: Yuanchun Shen --- .../main/java/org/opensearch/sql/ppl/parser/AstBuilder.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java index 87267ff886d..4383f5cd75b 100644 --- a/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java +++ b/ppl/src/main/java/org/opensearch/sql/ppl/parser/AstBuilder.java @@ -609,7 +609,8 @@ public UnresolvedPlan visitTimechartCommand(OpenSearchPPLParser.TimechartCommand UnresolvedExpression param = internalVisitExpression(paramCtx); if (param instanceof Span) { binExpression = param; - } else if (param instanceof Literal literal) { + } else if (param instanceof Literal) { + Literal literal = (Literal) param; if (DataType.BOOLEAN.equals(literal.getType())) { useOther = (Boolean) literal.getValue(); } else if (DataType.INTEGER.equals(literal.getType())