ESQL: TRange timezone support#139911
Conversation
…expected values in tests
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Hi @ivancea, I've created a changelog YAML for you. |
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
| value = literal.fold(FoldContext.small()); | ||
| } | ||
|
|
||
| if (value instanceof Instant instantValue) { |
There was a problem hiding this comment.
This was only being reached by the tests, which were inserting Instants as params. This should never happen in production code, so I instead fixed the tests and removed this from here
| String projectRouting | ||
| ) { | ||
| this.zoneId = zi.normalized(); | ||
| this.now = ZonedDateTime.now(Clock.tick(Clock.system(zoneId), Duration.ofNanos(1))); |
There was a problem hiding this comment.
This line was moved as-is to EsqlSession, which is the only place where a productive Configuration is created.
For other usages in tests, I used either Instant.now() or randomInstant()
| DataType.DATE_PERIOD, | ||
| Period.ofDays(1), | ||
| timestampDataType, | ||
| parse("2020-02-03T10:11:12.123456780Z"), |
There was a problem hiding this comment.
For easier reviewing, those parse() dates are, in this order:
- The row timestamp to be evaluated
- The "now" configuration value (Which is the "end" of the range, and the reference point for the date period maths
- An expected start date, to double-check that the "now - period" math is correct
| Clock fixedClock = Clock.fixed(fixedNow.toInstant(), fixedNow.getZone().normalized()); | ||
| ZonedDateTime fixedNow = ZonedDateTime.now(Clock.tick(fixedClock, Duration.ofNanos(1))); | ||
|
|
||
| Configuration spyConfig = Mockito.spy(configuration); | ||
| Mockito.doReturn(fixedNow).when(spyConfig).now(); |
There was a problem hiding this comment.
No need to mock the "now" anymore!
|
I'm open to move the configuration change to another PR. It's just that this PR tests depend on it, and the changes were quite straightforward. Open to opinions |
luigidellaquila
left a comment
There was a problem hiding this comment.
Thanks @ivancea, the general behavior is correct, so it LGTM
I only have a concern about how we declare support for keyword parameters, see comments below
| | date | date | boolean | | ||
| | date_nanos | date_nanos | boolean | | ||
| | date_period | | boolean | | ||
| | keyword | keyword | boolean | |
There was a problem hiding this comment.
This is a side effect of relying on implicit casting.
If we don't have tests for the KEYWORD type we'll lose this information. I'm not sure we want it.
| **Parameters** | ||
|
|
||
| `start_time_or_offset` | ||
| : Offset from NOW for the single parameter mode. Start time for two parameter mode. In two parameter mode, the start time value can be a date string, date, date_nanos or epoch milliseconds. |
| "params" : [ | ||
| { | ||
| "name" : "start_time_or_offset", | ||
| "type" : "keyword", |
There was a problem hiding this comment.
Will Kibana report an error now in case we pass a string?
I think it would be a problem, but I wouldn't consider this as a blocker for this specific issue, ie. every time we accept a date we should also accept a (properly formatted) string
There was a problem hiding this comment.
Answering both comments here:
My train of thought is that implicit casting works for other functions too, and those functions don't accept keywords (for dates, I mean). I tested Kibana with DATE_DIFF("day", "2020-10-10", 1::date), and it works correctly.
While testing this, however, I just found that we only implicit-cast literals, so this fails in both Kibana and esql, and would fail also in TRANGE after this change: DATE_DIFF("day", "2020-10-10", 1::string)
So, quite a shame, but I'll revert this change, and apply the timezone to the custom string conversion, as removing this now is a breaking change. I'll track it in a different issue, as I think it's worth doing for standardization
There was a problem hiding this comment.
Restored the keyword handling in this PR, and created an issue here: #140386
Also, added the tests for the edge case that would be breaking, so we don't forget it
# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java
…i-project-tests * upstream/main: (23 commits) Fix `testAckListenerReceivesNacksIfPublicationTimesOut` (elastic#140514) Reduce priority of clear-cache tasks (elastic#139685) Add docs and tests about `StreamOutput` to memory (elastic#140365) ES|QL - dense_vector support for COUNT, PRESENT, ABSENT aggregator functions (elastic#139914) Add release notes for v9.2.4 release (elastic#140487) Add release notes for v9.1.10 release (elastic#140488) Add conncectors release notes for 9.1.10, 9.2.4 (elastic#140499) Add parameter support in PromQL query durations (elastic#139873) Improve testing of STS credentials reloading (elastic#140114) Fix zstd native binary publishing script to support newer versions (elastic#140485) Add FlattenedFieldBinaryVsSortedSetDocValuesSyntheticSourceIT (elastic#140489) Store fallback match only text fields in binary doc values (elastic#140189) [DiskBBQ] Use the new merge executor for intra-merge parallelism (elastic#139942) ESQL: introduce support for mapping-unavailable fields (elastic#140463) Add ESNextOSQVectorsScorerTests (elastic#140436) Disable high cardinality tests on release builds (elastic#140503) ESQL: TRange timezone support (elastic#139911) Directly compressing `StreamOutput` (elastic#140502) ES|QL - fix dense vector enrich bug (elastic#139774) Use CrossProjectModeDecider in RemoteClusterService (elastic#140481) ...
- Added timezone support to `TRange(<date_period>)`. Now a `TRange(-1d)` will honor DST - Changed the `Configuration.now` field to be a constructor param. Currently, it was created inside, which made testing harder - Also changed it to be an `Instant` instead of a ZonedDateTime. It was also converted back to an instant, and there's no meaning in a zone here, as it's an absolute point in time. Plus there's a separated "zoneId" field.
TRange(<date_period>). Now aTRange(-1d)will honor DSTConfiguration.nowfield to be a constructor param. Currently, it was created inside, which made testing harderInstantinstead of a ZonedDateTime. It was also converted back to an instant, and there's no meaning in a zone here, as it's an absolute point in time. Plus there's a separated "zoneId" field.