ESQL: ToString/ToDatetime/ToDateNanos converters timezone support#138985
ESQL: ToString/ToDatetime/ToDateNanos converters timezone support#138985ivancea merged 37 commits intoelastic:mainfrom
Conversation
|
Hi @ivancea, I've created a changelog YAML for you. |
| @@ -7,9 +7,7 @@ | |||
|
|
|||
| package org.elasticsearch.xpack.esql.core.type; | |||
|
|
|||
| import org.elasticsearch.common.io.stream.NamedWriteable; | |||
|
|
|||
| public interface Converter extends NamedWriteable { | |||
There was a problem hiding this comment.
This is never written/read or even registered
| /** | ||
| * Converters that don't require a Configuration. | ||
| */ | ||
| private static final Map<DataType, BiFunction<Source, Expression, AbstractConvertFunction>> TYPE_TO_CONVERTER_FUNCTION = Map.ofEntries( |
There was a problem hiding this comment.
Here, I separated the conversion functions in 2 blocks:
- Simple converters that need no configuration
- Converters for functions that need the config. In the parser there's no config available, so we pass
ConfiguratioNAware.CONFIGURATION_MARKER
| @@ -241,56 +265,62 @@ public enum INTERVALS { | |||
| public static final String INVALID_INTERVAL_ERROR = | |||
| "Invalid interval value in [{}], expected integer followed by one of {} but got [{}]"; | |||
|
|
|||
| public static Converter converterFor(DataType from, DataType to) { | |||
| public static Converter converterFor(DataType from, DataType to, Configuration configuration) { | |||
There was a problem hiding this comment.
Here I directly moved the EsqlConverter enum lambdas here, as to reduce the extra indirection, and removed the enum. Current callers now directly use the functions, which are correctly typed (No cast needed).
I considered removing this, and delegating conversion to the conversion functions directly, with folding, either manually here or through the planner. But that looked like a bigger change, and I'm not sure yet
# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToString.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToStringTests.java
a808cc7 to
aed77b0
Compare
# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDateNanos.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToString.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/type/EsqlDataTypeConverter.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToStringTests.java
# Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java
luigidellaquila
left a comment
There was a problem hiding this comment.
Thanks @ivancea, LGTM
I just left a couple of minor comments
| */ | ||
|
|
||
| // Evaluators dynamically created in #factories() | ||
| Map.entry(KEYWORD, (source, fieldEval) -> null), |
There was a problem hiding this comment.
This is a bit ugly, but I guess it does the job (for supportedTypes())
There was a problem hiding this comment.
Yeah. I didn't find a simpler way, without refactoring the factories() method requiring a map like that.
I could remove those from the map and just add them dynamically, but it would make harder to know what the function actually supports
There was a problem hiding this comment.
I think it's fine as it is. This list was not intended to be dynamic, and having a all the keys defined ahead of time is a reasonable advantage. The comment is clear enough IMHO
.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/date/DateExtract.java
Outdated
Show resolved
Hide resolved
Fix release tests on ToStringTests for DateRange introduced on #138985 DateRange is snapshot-only for now, so those tests fail on release mode.
…astic#138985) Add timezone support to TO_STRING, TO_DATETIME and TO_DATENANOS. As they all are conversion functions, the change was also applied to the implicit casting rule, and some cases in the parser where a TO_STRING is needed.
…40837) Fix release tests on ToStringTests for DateRange introduced on elastic#138985 DateRange is snapshot-only for now, so those tests fail on release mode.
Add timezone support to TO_STRING, TO_DATETIME and TO_DATENANOS.
As they all are conversion functions, the change was also applied to the implicit casting rule, and some cases in the parser where a TO_STRING is needed.