Support metrics counter types in ESQL #107877

dnhatn · 2024-04-24T21:20:46Z

This commit adds support for numeric metrics counter fields in ES|QL. These counter types, including counter_long, counter_integer, and counter_double, are different from their parent types. Users will have limited interaction with these counter types, restricted to:

Retrieving values without any processing
Casting to their root type (e.g., to_long(a_long_counter))
Using them in the metrics rate aggregation

These restrictions are intentional to prevent misuse. If users want to use them as numeric values, explicit casting to their root types is required.

elasticsearchmachine · 2024-04-25T06:25:06Z

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

elasticsearchmachine · 2024-04-25T06:25:29Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

elasticsearchmachine · 2024-04-25T06:25:30Z

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

astefan

Looks good in general, but there are edge cases that might need some refining:

stats min(tx) -> argument of [min(tx)] must be [datetime or numeric except unsigned_long], found value [min(tx)] type [counter_long]. It's good we have a verification exception, but its message is not entirely true
tests are needed in VerifierTests for error messages when using these types in various places in queries
it seems I cannot aggregate on these types, but I can group by them (ie min(time) by counter). Is this expected?
count(counter) seems to also work...

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/40_tsdb.yml

nik9000

Left some questions. Probably good to go but I'll recheck after you answer.

docs/reference/esql/functions/kibana/definition/to_long.json

.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDouble.java

...ck/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/type/EsqlDataTypes.java

nik9000 · 2024-04-25T13:56:35Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/type/EsqlDataTypes.java

@@ -212,7 +219,8 @@ public static boolean isRepresentable(DataType t) {
            && t != FLOAT
            && t != SCALED_FLOAT
            && t != SOURCE
-            && t != HALF_FLOAT;
+            && t != HALF_FLOAT
+            && isCounterType(t) == false;


I think counters are representable though. They can be loaded into blocks now.

I disabled this mainly to avoid the map in AbstractFunctionTestCase #107877 (comment). Would you be okay if I enable this in a follow-up?

👍

Delaying is fine.

...ck/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/action/EsqlQueryResponseTests.java

nik9000 · 2024-04-25T13:58:23Z

...src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractFunctionTestCase.java

+                DataTypes.NULL
+            ),
+            "boolean or counter_double or datetime or numeric or string"
+        ),


I've grown to hate this Map. I made it, but I dislike it....

You've done the right thing to it, but I'm sorry you had to.

There's another version of errorsForCasesWithoutExamples which takes another parameter that outputs a matcher for hte expected type. You could use that rather than changing the map.

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/40_tsdb.yml

dnhatn · 2024-04-25T17:35:55Z

count(counter) seems to also work...

Should we allow counter_int and counter_long in all of these too?

Good question. Some usages, such as count(counter) or to_string(counter), seem to make sense. Should we support them all, or still require casting in these cases?

dnhatn · 2024-04-25T17:59:07Z

@astefan I pushed 5b09bdf to add verification tests and restrict usages in count, aggregation, and grouping.

astefan · 2024-04-25T17:59:50Z

count(counter) seems to also work...

Should we allow counter_int and counter_long in all of these too?

Good question. Some usages, such as count(counter) or to_string(counter), seem to make sense. Should we support them all, or still require casting in these cases?

I would be cautious and start with requiring casting everywhere, there may be edge cases that we couldn't think of atm and it may be difficult to remove the support once it's merged. If we get demand for this, we'll analyze these requests and adapt at that time.

dnhatn · 2024-04-25T18:02:45Z

I would be cautious and start with requiring casting everywhere, there may be edge cases that we couldn't think of atm and it may be difficult to remove the support once it's merged. If we get demand for this, we'll analyze these requests and adapt at that time.

Thanks @astefan. We are on the same page.

costin

This looks Nhat. Outside some superficial nits, the main comment I have is around CSV tests with counters in various places such as:

filtering (WHERE counter > 10)
expressions both named and anonymous + scalar functions (EVAL X = counter % 3 > 2 | WHERE counter_long > counter_double)
using it in agg functions MAX(counter), MIN(counter % 2)

Thanks!

...c/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/NumericAggregate.java

.../src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/convert/ToDouble.java

costin · 2024-04-26T06:20:16Z

...ck/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/LocalExecutionPlanner.java

@@ -344,7 +344,8 @@ private PhysicalOperation planTopN(TopNExec topNExec, LocalExecutionPlannerConte
                case "version" -> TopNEncoder.VERSION;
                case "boolean", "null", "byte", "short", "integer", "long", "double", "float", "half_float", "datetime", "date_period",
                    "time_duration", "object", "nested", "scaled_float", "unsigned_long", "_doc" -> TopNEncoder.DEFAULT_SORTABLE;
-                case "geo_point", "cartesian_point", "geo_shape", "cartesian_shape" -> TopNEncoder.DEFAULT_UNSORTABLE;
+                case "geo_point", "cartesian_point", "geo_shape", "cartesian_shape", "counter_long", "counter_integer", "counter_double" ->


Separate from this PR but it makes sense to centralize these strings in one class to avoid bugs due to typos.

astefan

Apart from Costin's comments (especially the additional tests), it's LGTM.

nik9000 · 2024-04-26T11:54:23Z

Outside some superficial nits, the main comment I have is around CSV tests with counters in various places such as:
* filtering (WHERE counter > 10)

* expressions both named and anonymous  + scalar functions (EVAL X = counter % 3 > 2 | WHERE counter_long > counter_double)

* using it in agg functions MAX(counter), MIN(counter % 2)

I think all of those should fail unless you wrap them in to_integer or whatever. Which we may as well test paranoia. Tests are cheap and run fast and make sure future me doesn't break them. Future me is a menace.

dnhatn · 2024-04-26T19:15:38Z

Thank you all for your reviews. I'll have to delay the CSV tests because I'm going to build a CSV dataset for the tsdb suite. Also, the CSV test infra doesn't support error checking yet. The YAML tests should have good coverage.

We should widen the numeric root types before converting them into counter types. Relates #107877

elasticsearchmachine added the v8.15.0 label Apr 24, 2024

dnhatn force-pushed the enable-counter-type branch from f1e3827 to 87dcd03 Compare April 24, 2024 21:30

dnhatn changed the title ~~Support counter type in ESQL~~ Support metrics counter types in ESQL Apr 24, 2024

dnhatn force-pushed the enable-counter-type branch 11 times, most recently from 84cd4ee to 68efa9a Compare April 25, 2024 05:23

Support counter type in ESQL

f1981e8

dnhatn force-pushed the enable-counter-type branch from 68efa9a to f1981e8 Compare April 25, 2024 05:25

dnhatn added :Analytics/ES|QL AKA ESQL :StorageEngine/TSDB You know, for Metrics >enhancement labels Apr 25, 2024

dnhatn requested review from nik9000, martijnvg, kkrik-es and astefan April 25, 2024 06:24

Update docs/changelog/107877.yaml

d3aff8d

dnhatn marked this pull request as ready for review April 25, 2024 06:25

elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:StorageEngine labels Apr 25, 2024

astefan reviewed Apr 25, 2024

View reviewed changes

kkrik-es reviewed Apr 25, 2024

View reviewed changes

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/40_tsdb.yml Show resolved Hide resolved

astefan requested a review from bpintea April 25, 2024 13:55

nik9000 reviewed Apr 25, 2024

View reviewed changes

dnhatn added 6 commits April 25, 2024 09:09

fix topn

06b3078

rename

0222b14

more tests

aa3e42e

response

5c2b08f

changelog

8ec3e66

javadoc

472fbd2

dnhatn requested a review from nik9000 April 25, 2024 16:29

strict validation

5b09bdf

dnhatn requested a review from astefan April 25, 2024 17:59

dnhatn added 2 commits April 25, 2024 11:03

stylecheck

8491d32

stylecheck

96bb7b7

costin reviewed Apr 26, 2024

View reviewed changes

astefan approved these changes Apr 26, 2024

View reviewed changes

dnhatn added 3 commits April 26, 2024 10:15

compact message

165385e

allow widen to_double and to_long

84733ff

Merge branch 'main' into enable-counter-type

29f71f9

dnhatn merged commit 22aad7b into elastic:main Apr 26, 2024
14 checks passed

dnhatn deleted the enable-counter-type branch April 26, 2024 19:15

dnhatn mentioned this pull request Apr 29, 2024

Widen numeric counter types #108036

Merged

dnhatn added a commit that referenced this pull request Apr 29, 2024

Widen numeric counter types (#108036)

b6ccb84

We should widen the numeric root types before converting them into counter types. Relates #107877

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support metrics counter types in ESQL #107877

Support metrics counter types in ESQL #107877

dnhatn commented Apr 24, 2024 •

edited

elasticsearchmachine commented Apr 25, 2024

elasticsearchmachine commented Apr 25, 2024

elasticsearchmachine commented Apr 25, 2024

astefan left a comment

nik9000 left a comment

nik9000 Apr 25, 2024

dnhatn Apr 25, 2024

nik9000 Apr 25, 2024

nik9000 Apr 25, 2024

nik9000 Apr 25, 2024

nik9000 Apr 25, 2024

dnhatn commented Apr 25, 2024

dnhatn commented Apr 25, 2024

astefan commented Apr 25, 2024

dnhatn commented Apr 25, 2024

costin left a comment

costin Apr 26, 2024

astefan left a comment

nik9000 commented Apr 26, 2024

dnhatn commented Apr 26, 2024

Support metrics counter types in ESQL #107877

Support metrics counter types in ESQL #107877

Conversation

dnhatn commented Apr 24, 2024 • edited

elasticsearchmachine commented Apr 25, 2024

elasticsearchmachine commented Apr 25, 2024

elasticsearchmachine commented Apr 25, 2024

astefan left a comment

Choose a reason for hiding this comment

nik9000 left a comment

Choose a reason for hiding this comment

nik9000 Apr 25, 2024

Choose a reason for hiding this comment

dnhatn Apr 25, 2024

Choose a reason for hiding this comment

nik9000 Apr 25, 2024

Choose a reason for hiding this comment

nik9000 Apr 25, 2024

Choose a reason for hiding this comment

nik9000 Apr 25, 2024

Choose a reason for hiding this comment

nik9000 Apr 25, 2024

Choose a reason for hiding this comment

dnhatn commented Apr 25, 2024

dnhatn commented Apr 25, 2024

astefan commented Apr 25, 2024

dnhatn commented Apr 25, 2024

costin left a comment

Choose a reason for hiding this comment

costin Apr 26, 2024

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

nik9000 commented Apr 26, 2024

dnhatn commented Apr 26, 2024

dnhatn commented Apr 24, 2024 •

edited