Skip to content
6 changes: 6 additions & 0 deletions docs/changelog/137713.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 137713
summary: TS Disallow renaming into timestamp prior to implicit use
area: ES|QL
type: bug
issues:
- 137655
3 changes: 0 additions & 3 deletions muted-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -427,9 +427,6 @@ tests:
method: testTopNSimilarityBetweenConstantVectorAndField {functionName=v_l2_norm
similarityFunction=org.elasticsearch.xpack.esql.expression.function.vector.L2Norm$1@5b068087 elementType=byte}
issue: https://github.com/elastic/elasticsearch/issues/137625
- class: org.elasticsearch.xpack.esql.qa.single_node.GenerativeMetricsIT
method: test
issue: https://github.com/elastic/elasticsearch/issues/137655
- class: org.elasticsearch.xpack.inference.action.filter.ShardBulkInferenceActionFilterBasicLicenseIT
method: testLicenseInvalidForInference {p0=true}
issue: https://github.com/elastic/elasticsearch/issues/137690
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ public abstract class GenerativeRestTest extends ESRestTestCase implements Query
"implicit time-series aggregation function .* doesn't support type .*",
"INLINE STATS .* can only be used after STATS when used with TS command",
"cannot group by a metric field .* in a time-series aggregation",
"requires a @timestamp field of type date to be present when run with the TS command",
"Output has changed from \\[.*\\] to \\[.*\\]" // https://github.com/elastic/elasticsearch/issues/134794
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ public Delta perTimeSeriesAggregation() {

@Override
public String toString() {
return "delta(" + field() + ")";
return "delta(" + field() + ", " + timestamp() + ")";
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ public FirstOverTime perTimeSeriesAggregation() {

@Override
public String toString() {
return "first_over_time(" + field() + ")";
return "first_over_time(" + field() + ", " + timestamp() + ")";
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ public Idelta perTimeSeriesAggregation() {

@Override
public String toString() {
return "idelta(" + field() + ")";
return "idelta(" + field() + ", " + timestamp() + ")";
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public Increase perTimeSeriesAggregation() {

@Override
public String toString() {
return "increase(" + field() + ")";
return "increase(" + field() + ", " + timestamp() + ")";
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ public Irate perTimeSeriesAggregation() {

@Override
public String toString() {
return "irate(" + field() + ")";
return "irate(" + field() + ", " + timestamp() + ")";
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ public LastOverTime perTimeSeriesAggregation() {

@Override
public String toString() {
return "last_over_time(" + field() + ")";
return "last_over_time(" + field() + ", " + timestamp() + ")";
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public Rate perTimeSeriesAggregation() {

@Override
public String toString() {
return "rate(" + field() + ")";
return "rate(" + field() + ", " + timestamp() + ")";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this added? Is it necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't necessary but @bpintea recommended it in our conversation and I agreed it made sense to add

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dnhatn wdyt?

If we want this, we'll have to update all time-series aggregate functions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the benefits of adding this, but I'm not against it either

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about the benefits of adding this

When seeing the failed query (in the issue this addresses), I wanted to quickly see what's happening in a running instance with debug on I hand running, but failed, because @timestamp isn't "visible". Since it's a parameter of the function, even if implicit, I think it can help. It's a nice to have though, not a strict necessity. And can be done here or some other PR.

}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@
import org.elasticsearch.xpack.esql.core.expression.Literal;
import org.elasticsearch.xpack.esql.core.expression.MetadataAttribute;
import org.elasticsearch.xpack.esql.core.expression.NamedExpression;
import org.elasticsearch.xpack.esql.core.tree.Node;
import org.elasticsearch.xpack.esql.core.type.DataType;
import org.elasticsearch.xpack.esql.core.util.CollectionUtils;
import org.elasticsearch.xpack.esql.core.util.Holder;
import org.elasticsearch.xpack.esql.expression.function.TimestampAware;
import org.elasticsearch.xpack.esql.expression.function.aggregate.AggregateFunction;
import org.elasticsearch.xpack.esql.expression.function.aggregate.DimensionValues;
import org.elasticsearch.xpack.esql.expression.function.aggregate.LastOverTime;
Expand Down Expand Up @@ -240,6 +242,25 @@ protected LogicalPlan rule(TimeSeriesAggregate aggregate, LogicalOptimizerContex
}
}
}
if (aggregate.child().output().contains(timestamp.get()) == false) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should run this check in TimeSeriesAggregate#postAnalysisVerification instead. What about TBucket? It seems we aren't checking it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you're right; TBucket is a TimestampAware function so I thought it would be getting caught there but it doesn't end up in the list of timeSeriesAggs so it's getting skipped over. I'll see what I can do about both of those

Copy link
Contributor Author

@limotova limotova Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rewrote it (edit: or so i thought; it didnt actually work) to be checked in TimeSeriesAggregate#postAnalysisVerification but it looks like something else actually catches the TBucket case already

      stack_trace: "org.elasticsearch.xpack.esql.VerificationException: Found 1 problem\n\
        line 3:68: [tbucket(5minute)] requires the [@timestamp] field, which was either\
        \ not present in the source index, or has been dropped or renamed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, all the TimestampAware functions that are created by the Analyzer will have the UnresolvedTimestamp injected and if during reference resolution this isn't swapped out, it'll get caught into the Verifier.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I guess we're doing this to prevent @timestamp be altered at all, right? Like maybe shifted with a TZ delta.
If so, maybe we'll want to add a test for it to testTranslateMetricsAfterRenamingTimestamp(), like ... | EVAL @timestamp = @timestamp + 1 day

var timestampAwareFunctions = timeSeriesAggs.keySet()
.stream()
.filter(ts -> ts instanceof TimestampAware)
.map(Node::sourceText)
.sorted()
.toList();
if (timestampAwareFunctions.isEmpty() == false) {
int size = timestampAwareFunctions.size();
throw new IllegalArgumentException(
"Function"
+ (size > 1 ? "s " : " ")
+ "["
+ String.join(", ", timestampAwareFunctions.subList(0, Math.min(size, 3)))
+ (size > 3 ? ", ..." : "")
+ "] requires a @timestamp field of type date to be present when run with the TS command, but it was not present."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll probably want to do the plural dance with the require/s too.
Also, can @timestamp be of nanos resolution, or just date?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah @timestamp can be nanos. I'll change the phrase a bit and look into adding tests for that in a bit (we don't have an index with a date_nanos timestamp that's set up as a time series index in the CSV tests)

);
}
}
// time-series aggregates must be grouped by _tsid (and time-bucket) first and re-group by users key
List<Expression> firstPassGroupings = new ArrayList<>();
firstPassGroupings.add(tsid.get());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9670,4 +9670,37 @@ public void testFullTextFunctionOnEvalNull() {
EsRelation relation = as(filter.child(), EsRelation.class);
assertEquals("test", relation.indexPattern());
}

/*
* Renaming or shadowing the @timestamp field prior to running stats with TS command is not allowed.
*/
public void testTranslateMetricsAfterRenamingTimestamp() {
assertThat(
expectThrows(
IllegalArgumentException.class,
() -> logicalOptimizerWithLatestVersion.optimize(metricsAnalyzer.analyze(parser.createStatement("""
TS k8s |
EVAL @timestamp = region |
STATS max(network.cost), count(network.eth0.rx)
""")))
).getMessage(),
containsString("""
Functions [count(network.eth0.rx), max(network.cost)] requires a @timestamp field of type date \
to be present when run with the TS command, but it was not present.""")
);

assertThat(
expectThrows(
IllegalArgumentException.class,
() -> logicalOptimizerWithLatestVersion.optimize(metricsAnalyzer.analyze(parser.createStatement("""
TS k8s |
DISSECT event "%{@timestamp} %{network.total_bytes_in}" |
STATS ohxqxpSqEZ = avg(network.eth0.currently_connected_clients)
""")))
).getMessage(),
containsString("""
Function [avg(network.eth0.currently_connected_clients)] requires a @timestamp field of type date \
to be present when run with the TS command, but it was not present.""")
);
}
}