ESQL Fix bug when renaming @timestamp in TS queries

[not-napoleon]

Resolves #134994

This PR addresses the issue with queries like

TS k8s
| RENAME `@timestamp` AS newTs 
| STATS maxRate = max(rate(network.total_cost))  BY time_bucket = bucket(newTs, 1hour)

Here, the rate aggregation function has an implied argument of @timestamp, but that field no longer exists when we get to the to the stats command. To solve this, we introduce a new specialization to the ResolveRefs rule that finds the timestamp alias and plumbs it into the TimeSeriesAggregation. In addition to fixing the references for the implied arguments, it also sends the reference on as part of the TimeSeriesAggregation for other use in the TranslateTimeSeriesAggregate rule.

[elasticsearchmachine]

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

[elasticsearchmachine]

Hi @not-napoleon, I've created a changelog YAML for you.

[not-napoleon]

I consider it a happy bonus of this PR that we are no longer fishing out the timestamp as part of the translate rule. I wonder if we should do something similar for the TSID. It just seems wrong to be doing this here.

[not-napoleon]

I had to ask @fang-xing-esql how this worked, so I've written down what I learned from that conversation.

[not-napoleon]

I didn't add a randomized timestamp to the serialization tests because I expect that it won't be serialized.

[dnhatn]

Thank you Mark! The alias trick to get this working is clever. I tested the PR with several queries:

1. TS hosts | RENAME @timestamp AS new_timestamp | STATS BY TBucket(1 minute) — works
2. FROM hosts | RENAME @timestamp AS new_timestamp | STATS BY TBucket(1 minute) — doesn't work (the fix applies to TS only)
3. TS hosts | EVAL new_timestamp = @timestamp | DROP @timestamp | STATS BY TBucket(1 minute) — works
4. TS hosts | DROP @timestamp | STATS BY TBucket(1 minute) — doesn't work (no alias)

I also noticed that users can rename or drop _tsid as well:

TS hosts | RENAME _tisd AS tsid | STATS sum(rate(request_count))

Thank you for your significant effort put into handling this case, but I wonder if we should provide a clearer error message rather than allowing the query to work in this way. What do you think? I'd also like to hear @kkrik-es's thoughts.

[not-napoleon]

Hey, thanks for taking a look.

2. `FROM hosts | RENAME @timestamp AS new_timestamp | STATS BY TBucket(1 minute)` — doesn't work (the fix applies to TS only)

I think we need to fix this one. Otherwise we're leaking TS behavior out into non-TS mode. I should be able to do that after this merges (or if we decide we don't want this fix, I can cherry pick the bits I need)

4. `TS hosts | DROP @timestamp | STATS BY TBucket(1 minute)` — doesn't work (no alias)

This one seems more like something we shouldn't support. Renaming the timestamp kind of makes sense - the user might reference it elsewhere in the query - but in this case they're dropping a field that would only be included in the output if they included it themselves. I think we could error or silently ignore this safely.

Another option would be to move the drop to after the stats. I don't love that, because it would have to be done in the analyzer phase, and that's not generally supposed to be moving nodes around as I understand it.

I also noticed that users can rename or drop _tsid as well:

TS hosts | RENAME _tisd AS tsid | STATS sum(rate(request_count))

Yeah, I saw that we treat _tsid basically the same way. It should be easy to fix it the same way, but I wanted to get some feedback from the planner devs to validate that this is the right way to fix it before I applied that fix more widely.

Thank you for your significant effort put into handling this case, but I wonder if we should provide a clearer error message rather than allowing the query to work in this way. What do you think? I'd also like to hear @kkrik-es's thoughts.

Happy to help :) FWIW, I think we should fix this one. I'd like to hear from @alex-spies or @fang-xing-esql , but I think resolving the timestamp (and eventually _tsid) in the ResolveRefs step is long term more stable and safe than what we're doing now where we have a kind of parallel resolution step as part of TranslateTimeSeriesAggregate in the substitutions step.

[dnhatn]

I don't think there is a use case for such queries; rather, we are fixing the generative queries. We could emit an error message like TS requires @timestamp and _tsid, but @timestamp/_tsid was renamed or dropped. However, I'm fine with fixing it if we find a good use case for these queries. Thanks again for handling this - it's tricky.

[fang-xing-esql]

If rate or tbucket requires @timestamp implicitly, I wonder if it is a good idea to make sure @timestamp is not changed by the children plans, otherwise we error out and return @timestamp is required by those functions?

It is an alternative option, it seems chasing down how the @timestamp was changed is tricky. And if the @timestamp is changed in a more complicated way, like eval x = @timestamp + 1 day | drop @timestamp, do we expect rate or tbucket to pick x up automatically?

If we really want @timestamp to work for the situations where its children plan renamed it, the first thing that I was thinking of to try is rewriting the rename @timestamp as x to eval x = @timestamp to preserve @timestamp in the plan, I kind of doubt this is a good idea, just an option that I can think of.

[dnhatn]

Thanks, Fang, for chiming in. For time-series queries to work properly, the timestamp and _tsid fields should not be manipulated; otherwise, the results could be incorrect or cause runtime errors.

[kkrik-es]

So, sounds like it's tricky to get aliases to work consistently. We should def catch the case where @timestamp or _tsid are dropped and complain. Excluding these, if aliases are hard to get them to work, I think it's fine to throw an error when renaming. I agree with Nhat that it's really a corner case, esp in the presence of TBUCKET and TRANGE; we should try to be as accommodating as possible but let's not go too far with this.

[not-napoleon]

Closing in favor of #136231