From c637bb25239ca2f88f60424f201094aaed9733f9 Mon Sep 17 00:00:00 2001 From: Yuanchun Shen Date: Thu, 21 Aug 2025 18:10:31 +0800 Subject: [PATCH 1/6] Support refering to implicit @timestamp field in time-based aggregations Signed-off-by: Yuanchun Shen --- .../opensearch/sql/ast/expression/Span.java | 8 +++++-- .../sql/calcite/CalciteRexNodeVisitor.java | 21 +++++++++++++++- .../sql/calcite/plan/OpenSearchConstants.java | 2 ++ .../remote/CalcitePPLAggregationIT.java | 24 +++++++++++++++++++ ppl/src/main/antlr/OpenSearchPPLParser.g4 | 2 +- .../sql/ppl/parser/AstExpressionBuilder.java | 3 ++- .../sql/ppl/utils/PPLQueryDataAnonymizer.java | 8 +++++-- 7 files changed, 61 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/ast/expression/Span.java b/core/src/main/java/org/opensearch/sql/ast/expression/Span.java index edd309b22d8..a9eb8440374 100644 --- a/core/src/main/java/org/opensearch/sql/ast/expression/Span.java +++ b/core/src/main/java/org/opensearch/sql/ast/expression/Span.java @@ -7,6 +7,7 @@ import com.google.common.collect.ImmutableList; import java.util.List; +import javax.annotation.Nullable; import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.RequiredArgsConstructor; @@ -19,13 +20,16 @@ @RequiredArgsConstructor @ToString public class Span extends UnresolvedExpression { - private final UnresolvedExpression field; + @Nullable private final UnresolvedExpression field; private final UnresolvedExpression value; private final SpanUnit unit; @Override public List getChild() { - return ImmutableList.of(field, value); + if (field == null) { + return ImmutableList.of(value); + } + return List.of(field, value); } @Override diff --git a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java index fec5d2dfa1d..e6912123892 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java @@ -46,6 +46,7 @@ import org.opensearch.sql.ast.expression.Cast; import org.opensearch.sql.ast.expression.Compare; import org.opensearch.sql.ast.expression.EqualTo; +import org.opensearch.sql.ast.expression.Field; import org.opensearch.sql.ast.expression.Function; import org.opensearch.sql.ast.expression.In; import org.opensearch.sql.ast.expression.Interval; @@ -67,6 +68,7 @@ import org.opensearch.sql.ast.expression.subquery.InSubquery; import org.opensearch.sql.ast.expression.subquery.ScalarSubquery; import org.opensearch.sql.ast.tree.UnresolvedPlan; +import org.opensearch.sql.calcite.plan.OpenSearchConstants; import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory; import org.opensearch.sql.calcite.utils.PlanUtils; import org.opensearch.sql.common.utils.StringUtils; @@ -349,7 +351,19 @@ public RexNode visitAlias(Alias node, CalcitePlanContext context) { @Override public RexNode visitSpan(Span node, CalcitePlanContext context) { - RexNode field = analyze(node.getField(), context); + RexNode field; + if (node.getField() != null) { + field = analyze(node.getField(), context); + } else { + try { + field = referenceImplicitTimestampField(context); + } catch (IllegalArgumentException e) { + throw new SemanticCheckException( + "SPAN operation requires an explicit field or an implicit '@timestamp' field, but" + + " '@timestamp' was not found in the input schema.", + e); + } + } RexNode value = analyze(node.getValue(), context); SpanUnit unit = node.getUnit(); RexBuilder rexBuilder = context.relBuilder.getRexBuilder(); @@ -359,6 +373,11 @@ public RexNode visitSpan(Span node, CalcitePlanContext context) { context.rexBuilder, BuiltinFunctionName.SPAN, field, value, unitNode); } + private RexNode referenceImplicitTimestampField(CalcitePlanContext context) { + return analyze( + new Field(new QualifiedName(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP)), context); + } + private boolean isTimeBased(SpanUnit unit) { return !(unit == NONE || unit == UNKNOWN); } diff --git a/core/src/main/java/org/opensearch/sql/calcite/plan/OpenSearchConstants.java b/core/src/main/java/org/opensearch/sql/calcite/plan/OpenSearchConstants.java index b0f02171075..2421717ca2e 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/plan/OpenSearchConstants.java +++ b/core/src/main/java/org/opensearch/sql/calcite/plan/OpenSearchConstants.java @@ -20,6 +20,8 @@ public interface OpenSearchConstants { String METADATA_FIELD_ROUTING = "_routing"; + String IMPLICIT_FIELD_TIMESTAMP = "@timestamp"; + java.util.Map METADATAFIELD_TYPE_MAP = Map.of( METADATA_FIELD_ID, ExprCoreType.STRING, 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 eb725c4eea4..90939c59266 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 @@ -14,6 +14,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; @@ -24,6 +25,8 @@ import org.junit.Ignore; 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 { @@ -38,6 +41,7 @@ public void init() throws Exception { loadIndex(Index.CALCS); loadIndex(Index.DATE_FORMATS); loadIndex(Index.DATA_TYPE_NUMERIC); + loadIndex(Index.BIG5); } @Test @@ -515,6 +519,26 @@ 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, + "SPAN operation requires an explicit field or an implicit '@timestamp' field, but" + + " '@timestamp' was not found in the input schema."); + } + @Test public void testCountDistinct() throws IOException { JSONObject actual = diff --git a/ppl/src/main/antlr/OpenSearchPPLParser.g4 b/ppl/src/main/antlr/OpenSearchPPLParser.g4 index 3f75da1245e..ae9d74f21b1 100644 --- a/ppl/src/main/antlr/OpenSearchPPLParser.g4 +++ b/ppl/src/main/antlr/OpenSearchPPLParser.g4 @@ -381,7 +381,7 @@ bySpanClause ; spanClause - : SPAN LT_PRTHS fieldExpression COMMA value = literalValue (unit = timespanUnit)? RT_PRTHS + : SPAN LT_PRTHS (fieldExpression COMMA)? value = literalValue (unit = timespanUnit)? RT_PRTHS ; sortbyClause 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 cd5e3a76d8d..f9816675c7a 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 @@ -523,7 +523,8 @@ public UnresolvedExpression visitBySpanClause(BySpanClauseContext ctx) { @Override public UnresolvedExpression visitSpanClause(SpanClauseContext ctx) { String unit = ctx.unit != null ? ctx.unit.getText() : ""; - return new Span(visit(ctx.fieldExpression()), visit(ctx.value), SpanUnit.of(unit)); + var field = ctx.fieldExpression() != null ? visit(ctx.fieldExpression()) : null; + return new Span(field, visit(ctx.value), SpanUnit.of(unit)); } @Override diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.java b/ppl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.java index 1e751cb10e7..e14d4c566ea 100644 --- a/ppl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.java +++ b/ppl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.java @@ -553,9 +553,13 @@ public String visitAggregateFunction(AggregateFunction node, String context) { @Override public String visitSpan(Span node, String context) { - String field = analyze(node.getField(), context); + String field = node.getField() != null ? analyze(node.getField(), context) : null; String value = analyze(node.getValue(), context); - return StringUtils.format("span(%s, %s %s)", field, value, node.getUnit().getName()); + if (field != null) { + return StringUtils.format("span(%s, %s %s)", field, value, node.getUnit().getName()); + } else { + return StringUtils.format("span(%s %s)", value, node.getUnit().getName()); + } } @Override From e3f904b63489d950965dd8971d34e7dbb9331dbc Mon Sep 17 00:00:00 2001 From: Yuanchun Shen Date: Wed, 27 Aug 2025 17:07:37 +0800 Subject: [PATCH 2/6] Update documentation of stats to reflect that span can be used without specifying a field Signed-off-by: Yuanchun Shen --- .../opensearch/sql/ast/expression/Span.java | 2 ++ docs/user/ppl/cmd/stats.rst | 18 ++++++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/ast/expression/Span.java b/core/src/main/java/org/opensearch/sql/ast/expression/Span.java index a9eb8440374..056cafd5c22 100644 --- a/core/src/main/java/org/opensearch/sql/ast/expression/Span.java +++ b/core/src/main/java/org/opensearch/sql/ast/expression/Span.java @@ -20,7 +20,9 @@ @RequiredArgsConstructor @ToString public class Span extends UnresolvedExpression { + /** When field is {@code null}, span implicitly refers to {@code @timestamp} field */ @Nullable private final UnresolvedExpression field; + private final UnresolvedExpression value; private final SpanUnit unit; diff --git a/docs/user/ppl/cmd/stats.rst b/docs/user/ppl/cmd/stats.rst index a2a6885c744..5acf01ac211 100644 --- a/docs/user/ppl/cmd/stats.rst +++ b/docs/user/ppl/cmd/stats.rst @@ -45,8 +45,8 @@ stats ... [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 and time type field, 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 and time type field, 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: +----------------------------+ @@ -525,3 +525,17 @@ PPL query:: | 36 | 30 | M | +-----+----------+--------+ +Example 14: 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 | + +---------+---------------------+ From 111b095f3c94daf442c8f1ce5fbf81816b964415 Mon Sep 17 00:00:00 2001 From: Yuanchun Shen Date: Thu, 18 Sep 2025 14:25:31 +0800 Subject: [PATCH 3/6] Move @timestamp reference to AST layer - Additionally refactored visitTimechartCommand to reuse spanLiteral definition Signed-off-by: Yuanchun Shen --- .../org/opensearch/sql/ast/dsl/AstDSL.java | 6 ++ .../opensearch/sql/ast/expression/Span.java | 5 +- .../sql/calcite/CalciteRexNodeVisitor.java | 14 +--- .../remote/CalcitePPLAggregationIT.java | 5 +- ppl/src/main/antlr/OpenSearchPPLParser.g4 | 11 +-- .../opensearch/sql/ppl/parser/AstBuilder.java | 40 +++------- .../sql/ppl/parser/AstExpressionBuilder.java | 73 ++++++++++++++----- 7 files changed, 77 insertions(+), 77 deletions(-) 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 c8600bb9809..3b53881aebc 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 @@ -666,4 +667,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/core/src/main/java/org/opensearch/sql/ast/expression/Span.java b/core/src/main/java/org/opensearch/sql/ast/expression/Span.java index 056cafd5c22..90bd00d85c7 100644 --- a/core/src/main/java/org/opensearch/sql/ast/expression/Span.java +++ b/core/src/main/java/org/opensearch/sql/ast/expression/Span.java @@ -7,7 +7,6 @@ import com.google.common.collect.ImmutableList; import java.util.List; -import javax.annotation.Nullable; import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.RequiredArgsConstructor; @@ -20,9 +19,7 @@ @RequiredArgsConstructor @ToString public class Span extends UnresolvedExpression { - /** When field is {@code null}, span implicitly refers to {@code @timestamp} field */ - @Nullable private final UnresolvedExpression field; - + private final UnresolvedExpression field; private final UnresolvedExpression value; private final SpanUnit unit; diff --git a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java index b6ac7a3bf3e..205f38cf867 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java @@ -357,19 +357,7 @@ public RexNode visitAlias(Alias node, CalcitePlanContext context) { @Override public RexNode visitSpan(Span node, CalcitePlanContext context) { - RexNode field; - if (node.getField() != null) { - field = analyze(node.getField(), context); - } else { - try { - field = referenceImplicitTimestampField(context); - } catch (IllegalArgumentException e) { - throw new SemanticCheckException( - "SPAN operation requires an explicit field or an implicit '@timestamp' field, but" - + " '@timestamp' was not found in the input schema.", - e); - } - } + RexNode field = analyze(node.getField(), context); RexNode value = analyze(node.getValue(), context); SpanUnit unit = node.getUnit(); RexBuilder rexBuilder = context.relBuilder.getRexBuilder(); 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 ce0d579c954..6e6a0e8388b 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 @@ -741,10 +741,7 @@ public void testSpanByImplicitTimestamp() throws IOException { executeQuery( StringUtils.format( "source=%s | stats count() by span(5m)", TEST_INDEX_DATE_FORMATS))); - verifyErrorMessageContains( - t, - "SPAN operation requires an explicit field or an implicit '@timestamp' field, but" - + " '@timestamp' was not found in the input schema."); + verifyErrorMessageContains(t, "field [@timestamp] not found"); } @Test diff --git a/ppl/src/main/antlr/OpenSearchPPLParser.g4 b/ppl/src/main/antlr/OpenSearchPPLParser.g4 index a934d911178..a1cc39df674 100644 --- a/ppl/src/main/antlr/OpenSearchPPLParser.g4 +++ b/ppl/src/main/antlr/OpenSearchPPLParser.g4 @@ -249,18 +249,13 @@ timechartCommand ; timechartParameter - : (spanClause | SPAN EQUAL spanLiteral) - | timechartArg - ; - -timechartArg : LIMIT EQUAL integerLiteral + | SPAN EQUAL spanLiteral | USEOTHER EQUAL (booleanLiteral | ident) ; spanLiteral - : integerLiteral timespanUnit - | stringLiteral + : value = literalValue (unit = timespanUnit)? ; evalCommand @@ -598,7 +593,7 @@ bySpanClause ; spanClause - : SPAN LT_PRTHS (fieldExpression COMMA)? value = literalValue (unit = timespanUnit)? RT_PRTHS + : SPAN LT_PRTHS (fieldExpression COMMA)? 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 774eb73dff6..56428abd0ff 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 @@ -62,6 +62,7 @@ import org.opensearch.sql.ast.expression.SearchAnd; import org.opensearch.sql.ast.expression.SearchExpression; import org.opensearch.sql.ast.expression.SearchGroup; +import org.opensearch.sql.ast.expression.Span; import org.opensearch.sql.ast.expression.SpanUnit; import org.opensearch.sql.ast.expression.UnresolvedArgument; import org.opensearch.sql.ast.expression.UnresolvedExpression; @@ -622,40 +623,21 @@ 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.of("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) { - // Convert span=1h to span(@timestamp, 1h) - binExpression = internalVisitExpression(paramCtx.spanLiteral()); - } 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 span) { + binExpression = span; + } 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 ae08aa7c91c..9c0cca1c158 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 @@ -612,30 +612,65 @@ public UnresolvedExpression visitBySpanClause(BySpanClauseContext ctx) { @Override public UnresolvedExpression visitSpanClause(SpanClauseContext ctx) { - String unit = ctx.unit != null ? ctx.unit.getText() : ""; - var field = ctx.fieldExpression() != null ? visit(ctx.fieldExpression()) : null; - return new Span(field, visit(ctx.value), SpanUnit.of(unit)); + // If a field is not specified in a span clause, it defaults to the implicit @timestamp field. + // E.g. span(1d) <==> span(@timestamp, 1d) + var field = + ctx.fieldExpression() != null + ? visit(ctx.fieldExpression()) + : AstDSL.referImplicitTimestampField(); + Span partialSpan = (Span) visit(ctx.spanLiteral()); + return AstDSL.span(field, partialSpan.getValue(), partialSpan.getUnit()); } - // Handle new syntax: span=1h + /** Construct a partial Span without field specified that represents a span value */ @Override public UnresolvedExpression visitSpanLiteral(OpenSearchPPLParser.SpanLiteralContext ctx) { - if (ctx.integerLiteral() != null && ctx.timespanUnit() != null) { - return new Span( - AstDSL.field("@timestamp"), - new Literal(Integer.parseInt(ctx.integerLiteral().getText()), DataType.INTEGER), - SpanUnit.of(ctx.timespanUnit().getText())); - } - - if (ctx.integerLiteral() != null) { - return new Span( - AstDSL.field("@timestamp"), - new Literal(Integer.parseInt(ctx.integerLiteral().getText()), DataType.INTEGER), - SpanUnit.of("")); + UnresolvedExpression literal = visit(ctx.literalValue()); + String unitText = ctx.timespanUnit() != null ? ctx.timespanUnit().getText() : ""; + SpanUnit unit = SpanUnit.of(unitText); + return AstDSL.span(null, literal, unit); + } + + @Override + public UnresolvedExpression visitTimechartParameter( + OpenSearchPPLParser.TimechartParameterContext ctx) { + UnresolvedExpression timechartParameter; + if (ctx.SPAN() != null) { + // Convert span=1h to span(@timestamp, 1h) + Span partialSpan = (Span) visit(ctx.spanLiteral()); + timechartParameter = + AstDSL.span( + AstDSL.referImplicitTimestampField(), partialSpan.getValue(), partialSpan.getUnit()); + } 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 new Span( - AstDSL.field("@timestamp"), new Literal(ctx.getText(), DataType.STRING), SpanUnit.of("")); + return timechartParameter; } @Override From 7d7108eab4b8939fc34c7ecf4ca540270b990a6a Mon Sep 17 00:00:00 2001 From: Yuanchun Shen Date: Thu, 18 Sep 2025 15:11:18 +0800 Subject: [PATCH 4/6] Unit test visitSpanLiteral, vistSpanClause, and visitTimechartParamter Signed-off-by: Yuanchun Shen --- .../sql/calcite/CalciteRexNodeVisitor.java | 7 - .../ppl/parser/AstExpressionBuilderTest.java | 223 ++++++++++++++++++ 2 files changed, 223 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java index 205f38cf867..f82e63302ab 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java @@ -46,7 +46,6 @@ import org.opensearch.sql.ast.expression.Cast; import org.opensearch.sql.ast.expression.Compare; import org.opensearch.sql.ast.expression.EqualTo; -import org.opensearch.sql.ast.expression.Field; import org.opensearch.sql.ast.expression.Function; import org.opensearch.sql.ast.expression.In; import org.opensearch.sql.ast.expression.Interval; @@ -68,7 +67,6 @@ import org.opensearch.sql.ast.expression.subquery.InSubquery; import org.opensearch.sql.ast.expression.subquery.ScalarSubquery; import org.opensearch.sql.ast.tree.UnresolvedPlan; -import org.opensearch.sql.calcite.plan.OpenSearchConstants; import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory; import org.opensearch.sql.calcite.utils.PlanUtils; import org.opensearch.sql.common.utils.StringUtils; @@ -367,11 +365,6 @@ public RexNode visitSpan(Span node, CalcitePlanContext context) { context.rexBuilder, BuiltinFunctionName.SPAN, field, value, unitNode); } - private RexNode referenceImplicitTimestampField(CalcitePlanContext context) { - return analyze( - new Field(new QualifiedName(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP)), context); - } - private boolean isTimeBased(SpanUnit unit) { return !(unit == NONE || unit == UNKNOWN); } 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 31678148c9c..2a8bfdf0090 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 { @@ -1360,4 +1365,222 @@ public void testMedianAggFuncExpr() { emptyList(), defaultStatsArgs())); } + + @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=2.5m count()", + Timechart.builder() + .child(relation("t")) + .binExpression( + span( + field(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP), + decimalLiteral(2.5), + 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 4a9193fb189287482e94b4e80f52bcb6f32d6d36 Mon Sep 17 00:00:00 2001 From: Yuanchun Shen Date: Wed, 24 Sep 2025 16:44:38 +0800 Subject: [PATCH 5/6] Revert changes to Span will always have a field with the current implementation Signed-off-by: Yuanchun Shen --- .../main/java/org/opensearch/sql/ast/expression/Span.java | 5 +---- .../opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.java | 8 ++------ 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/ast/expression/Span.java b/core/src/main/java/org/opensearch/sql/ast/expression/Span.java index 90bd00d85c7..edd309b22d8 100644 --- a/core/src/main/java/org/opensearch/sql/ast/expression/Span.java +++ b/core/src/main/java/org/opensearch/sql/ast/expression/Span.java @@ -25,10 +25,7 @@ public class Span extends UnresolvedExpression { @Override public List getChild() { - if (field == null) { - return ImmutableList.of(value); - } - return List.of(field, value); + return ImmutableList.of(field, value); } @Override diff --git a/ppl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.java b/ppl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.java index 267b0f37ed8..d5c55d10258 100644 --- a/ppl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.java +++ b/ppl/src/main/java/org/opensearch/sql/ppl/utils/PPLQueryDataAnonymizer.java @@ -729,13 +729,9 @@ public String visitAggregateFunction(AggregateFunction node, String context) { @Override public String visitSpan(Span node, String context) { - String field = node.getField() != null ? analyze(node.getField(), context) : null; + String field = analyze(node.getField(), context); String value = analyze(node.getValue(), context); - if (field != null) { - return StringUtils.format("span(%s, %s %s)", field, value, node.getUnit().getName()); - } else { - return StringUtils.format("span(%s %s)", value, node.getUnit().getName()); - } + return StringUtils.format("span(%s, %s %s)", field, value, node.getUnit().getName()); } @Override From edcb72f71299678b2281ce900b2e9d11543fbee7 Mon Sep 17 00:00:00 2001 From: Yuanchun Shen Date: Fri, 17 Oct 2025 16:25:35 +0800 Subject: [PATCH 6/6] Throw exception for zero span Signed-off-by: Yuanchun Shen --- .../org/opensearch/sql/ast/dsl/AstDSL.java | 8 ++- .../rest-api-spec/test/issues/4527.yml | 50 +++++++++++++++++++ .../ppl/calcite/CalcitePPLTimechartTest.java | 8 +++ 3 files changed, 64 insertions(+), 2 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 057d04c7f28..86b2343ace1 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 @@ -492,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 @@ -501,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); } 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/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);