ESQL: Mark counter fields as unsupported

[nik9000]

This marks TSDB's counter fields as unsupported by ESQL. We'll support them eventually, but in the short term we were just treating them like their underlying numeric type and that's not great. They have a conceptually different meaning that we'd like to respect one day. So, for now, we'll mark them unsupported.

[elasticsearchmachine]

Pinging @elastic/es-ql (Team:QL)

[elasticsearchmachine]

Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL)

[elasticsearchmachine]

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

[nik9000]

@astefan, I'm not sure of a good way to unit test this thing. Do you have any ideas?

[astefan]

I would try to make this a bit more generic, ie instead of specifically looking at TimeSeriesParams.MetricType I'd try to use the FieldCapabilities instance itself from where I could extract whatever information necessary to decide the supportability of a data type. Another suggestion is to look into defining a second fromEs method that receives this FieldCapabilities parameter and keep the original one unchanged.

[astefan]

When the IndexResolver has been created the field_caps API didn't return such additional information and I am not seeing any less intrusive way to support this other than what you suggested.

[nik9000]

I'd be worried about keeping the original one unchanged because it's kind of dangerous, at least in ESQL it is. But I'm happy to move the whole field caps response there.

[costin]

I'm stumbling over the same issue - how about encoding the TIME_SERIES_METRIC_PARAM inside the name along with its value, as a suffix for example?
This convention avoids having to extend the static method and can handle in the future more parameter or special types outside the metric ones without having to extend the method yet again.

For example when dealing with a timeseries metric type, append that to the typeName itself and then check in EsqlDataType if the typeName ends up TIME_SERIES_METRIC_PARAM-COUNTER.

[nik9000]

Adding it to the string feels easy to break. It'd be even more likely for folks to accidentally not use the metric param.

[nik9000]

I looked at taking the whole FieldCapabilities object but there is a place we don't have it - in the mapping walking code. I'm not sure it's worth trying to get it there when adding the smaller parameter works.

[costin]

How about adding a generic object as field context instead of typing it to TimeSeriesParams.MetricType and doing an instanceof check?
It is not OOP-ish but in this case we're looking at a bag properties for fields vs just the name.
For example how about DataType fromEs(String typeName, Object typeContext)
and then in ESQL do:

if (typeContext ==TimeSeriesParams.MetricType.COUNTER)) {
  ...
}

I'd like to avoid us to add a new param for each special type on the method signature and instead use a "bucket" type approach.
If you're on the fence, we can merge the PR and revisit this in the future.

[nik9000]

I get where your coming from. My preference would be to merge as is and then, when we got to add the next "funny" parameter, we do something like:

record FromEs(String typeName, TimeSeriesParams.MetricType metricType, TheNewThing newThing) {}

DataType fromEs(FromEs fromEs) {

If it feels sensible then. I don't feel like we need something so fancy for two fields. But three or our we really may.

[astefan]

Regarding unit testing, there is TypesTests class in the ql module. But this is not an ideal test for the IndexResolver logic, since it's not using its methods :-(, it's a series of tests more for the data types themselves. There's also, probably better choice, IndexResolverTests in the SQL module, where FieldCapabilities instances are manually created and the result of indexresolver logic asserted.

[nik9000]

IndexResolverTests

I saw that! I thought "oh, I can copy this into the ql module and make changes. But that doesn't quite work. I'll push more in that direction.

[martijnvg]

+1 for not allowing counter fields in esql for now.

[costin]

Added a comment with my preference, I'll leave it up to you on how to move forward.

[nik9000]

@astefan could you have a look at this one again? I'm not sure about pushing the whole FieldCapabilities instance in - it's kind of not possible in some branches - though I wonder if that's testing branches that are only used in SQL.