[ES|QL] TS Disallow renaming into timestamp prior to implicit use #137713

limotova · 2025-11-07T07:52:47Z

This commit adds a check after translating STATS run with the TS command that the @timestamp field still exists for commands that require it (TimestampAware functions and functions that bucket on it) and errors if it is missing.

Resolves #137655

elasticsearchmachine · 2025-11-11T21:13:08Z

Pinging @elastic/es-storage-engine (Team:StorageEngine)

elasticsearchmachine · 2025-11-11T21:13:08Z

Hi @limotova, I've created a changelog YAML for you.

kkrik-es · 2025-11-12T08:36:06Z

...ugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Rate.java

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


Why was this added? Is it necessary?

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

@dnhatn wdyt?

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

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

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.

dnhatn

I've left a question, thanks Larisa!

dnhatn · 2025-11-12T18:10:08Z

.../java/org/elasticsearch/xpack/esql/optimizer/rules/logical/TranslateTimeSeriesAggregate.java

                }
            }
        }
+        if (aggregate.child().output().contains(timestamp.get()) == false) {


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

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

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

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.

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

dnhatn · 2025-11-12T18:12:08Z

...ugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Rate.java

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


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

bpintea

Lgtm

bpintea · 2025-11-13T13:01:26Z

.../java/org/elasticsearch/xpack/esql/optimizer/rules/logical/TranslateTimeSeriesAggregate.java

                }
            }
        }
+        if (aggregate.child().output().contains(timestamp.get()) == false) {


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.

bpintea · 2025-11-13T13:02:59Z

.../java/org/elasticsearch/xpack/esql/optimizer/rules/logical/TranslateTimeSeriesAggregate.java

+                        + "["
+                        + 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."


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

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)

bpintea · 2025-11-13T13:04:41Z

.../java/org/elasticsearch/xpack/esql/optimizer/rules/logical/TranslateTimeSeriesAggregate.java

                }
            }
        }
+        if (aggregate.child().output().contains(timestamp.get()) == false) {


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

bpintea · 2025-11-13T13:09:09Z

...ugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Rate.java

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


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.

[ES|QL] TS Disallow renaming timestamp prior to implicit use

d38774b

limotova changed the title ~~[ES|QL] TS Disallow renaming timestamp prior to implicit use~~ [ES|QL] TS Disallow renaming into timestamp prior to implicit use Nov 7, 2025

elasticsearchmachine added the v9.3.0 label Nov 7, 2025

limotova added 5 commits November 9, 2025 16:51

add test and rephrase

97e53c0

add timestamp to ToString of TimestampAware functions

0eb4a5b

Merge branch 'main' into disallow-renaming-timestamp

d748fe5

fix generative test message

27c236b

fix test

446cffd

limotova force-pushed the disallow-renaming-timestamp branch from 703e816 to 446cffd Compare November 11, 2025 04:23

Merge branch 'main' into disallow-renaming-timestamp

80abab0

limotova requested review from dnhatn, kkrik-es and not-napoleon November 11, 2025 21:04

limotova marked this pull request as ready for review November 11, 2025 21:11

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Nov 11, 2025

limotova added >bug :StorageEngine/ES|QL Timeseries / metrics / logsdb capabilities in ES|QL and removed needs:triage Requires assignment of a team area label labels Nov 11, 2025

elasticsearchmachine added the Team:StorageEngine label Nov 11, 2025

Update docs/changelog/137713.yaml

473184b

limotova requested a review from bpintea November 11, 2025 21:33

kkrik-es reviewed Nov 12, 2025

View reviewed changes

dnhatn reviewed Nov 12, 2025

View reviewed changes

bpintea approved these changes Nov 13, 2025

View reviewed changes

limotova added 2 commits November 13, 2025 14:22

Merge branch 'main' into disallow-renaming-timestamp

4a4c0e3

rephrase error message and add one more test query

2623e7d

limotova merged commit 3cc97b9 into elastic:main Nov 14, 2025
34 checks passed

limotova deleted the disallow-renaming-timestamp branch November 14, 2025 06:57

[ES|QL] TS Disallow renaming into timestamp prior to implicit use #137713

[ES|QL] TS Disallow renaming into timestamp prior to implicit use #137713

Uh oh!

Conversation

limotova commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Nov 11, 2025

Uh oh!

elasticsearchmachine commented Nov 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnhatn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

limotova Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpintea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

limotova commented Nov 7, 2025 •

edited

Loading

limotova Nov 13, 2025 •

edited

Loading