ESQL: Move more of EsqlDataTypes into DataType #109921
Conversation
This further consolidates our type resolution stuff into `DataType`. Slowly, slowly, there will be one place to do this stuff for ESQL.
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| @@ -27,32 +27,9 @@ | |||
| import static java.util.stream.Collectors.toUnmodifiableMap; | |||
|
|
|||
| public enum DataType { | |||
| UNSUPPORTED("UNSUPPORTED", null, 0, false, false, false), | |||
There was a problem hiding this comment.
I went to add three more parameters and couldn't read anything. So I shifted it to a sort of named parameters with defaults thing.
There was a problem hiding this comment.
If we're going to use a builder, I'd prefer if the constructor didn't take any parameters and everything used a named parameter function. I do think the builder is a good idea.
| assumeTrue("Expected type must be representable to build an evaluator", EsqlDataTypes.isRepresentable(testCase.expectedType())); | ||
| logger.info( | ||
| "Test Values: " + testCase.getData().stream().map(TestCaseSupplier.TypedData::toString).collect(Collectors.joining(",")) | ||
| ); |
There was a problem hiding this comment.
I moved this because isRepresentable had been getting a null value for a long time and that was returning null. With this change you can't send null to isRepresentable. Which, to be honest, we never should have been sending. It's kind of like asking "can you represent unknown in a block?". And the answer is "no, of course not, that's silly".
| * If this is a "small" numeric type this contains the type ESQL will | ||
| * widen it into, otherwise this is {@code null}. | ||
| */ | ||
| private final DataType widenSmallNumeric; |
There was a problem hiding this comment.
This doesn't seem a property of the type, IMO, but a type's user preference.
If we add, say, float through support, we'd no longer widen the values to double.
There was a problem hiding this comment.
I figured we'd remove that widening in that case. It feels a little simpler to stick it here, but I'm not tightly tied to it.
There was a problem hiding this comment.
I merged this on Friday so Mark could build on this. I covered some of the followups in #110105. But not this one. I kind of like keeping this where it is....
There was a problem hiding this comment.
Was a minor note, it can stay as is 👍
| @@ -27,32 +27,9 @@ | |||
| import static java.util.stream.Collectors.toUnmodifiableMap; | |||
|
|
|||
| public enum DataType { | |||
| UNSUPPORTED("UNSUPPORTED", null, 0, false, false, false), | |||
There was a problem hiding this comment.
If we're going to use a builder, I'd prefer if the constructor didn't take any parameters and everything used a named parameter function. I do think the builder is a good idea.
| return EsqlDataTypes.fromName(typeName); | ||
| } | ||
| DataType type = EsqlDataTypes.fromName(typeName); | ||
| return metricType == TimeSeriesParams.MetricType.COUNTER ? type.widenSmallNumeric().counter() : type; |
There was a problem hiding this comment.
It's not obvious to me what this is doing, or why it's equivalent to the old code. Please add a comment.
There was a problem hiding this comment.
I added this to #110105.
|
@not-napoleon is going to build on top of this one so I'm going to merge even though we've got some feedback on this. I'll iterate on the feedback in a separate PR on Monday. |
This moves a few more methods from `EsqlDataTypes` into `DataType` and makes some of the followup changes recommended in elastic#109921.
This moves a few more methods from `EsqlDataTypes` into `DataType` and makes some of the followup changes recommended in #109921.
This further consolidates our type resolution stuff into
DataType. Slowly, slowly, there will be one place to do this stuff for ESQL.