Reject unsupported last_over_time during verification #136517
Conversation
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
dnhatn
force-pushed
the
validate-time-series
branch
from
6af2b1e to
5e82c63
Compare
dnhatn
added
the
auto-backport
| + "dimension fields, use the FROM command instead of the TS command.", | ||
| fa.sourceText(), | ||
| outer.sourceText() | ||
| "time-series aggregation inside function [{}] doesn't support type [{}]; only numeric types are supported," |
There was a problem hiding this comment.
| "time-series aggregation inside function [{}] doesn't support type [{}]; only numeric types are supported," | |
| "time-series aggregation function [{}] doesn't support type [{}]; only numeric types are supported," |
There was a problem hiding this comment.
These are now defined in documentation:
https://www.elastic.co/docs/reference/query-languages/esql/functions-operators/time-series-aggregation-functions
There was a problem hiding this comment.
I see what you mean.. It'd be nice to include the name of the time-series agg too, like
"time-series aggregation [{}] inside function [{}] doesn't support type [{}]; only numeric types are supported,"
There was a problem hiding this comment.
Thanks Kostas - I updated the error message in 03afdb2
| "1:11: cannot use dimension field [host] in a time-series aggregation function [count(host)]. " | ||
| + "Dimension fields can only be used for grouping in a BY clause. " | ||
| + "To aggregate dimension fields, use the FROM command instead of the TS command." | ||
| "1:11: time-series aggregation inside function [count(host)] doesn't support type [keyword]; " |
There was a problem hiding this comment.
| "1:11: time-series aggregation inside function [count(host)] doesn't support type [keyword]; " | |
| "1:11: time-series aggregation function [count(host)] doesn't support type [keyword]; " |
There was a problem hiding this comment.
This is somewhat misleading though.. the function that doesn't support keyword is last_over_time, not count..
Can we surface the actual time-series function and include that in the error message, e.g. count(last_over_time(host)) here? This is now documented in a note in:
https://www.elastic.co/docs/reference/query-languages/esql/commands/ts
| "Invalid call to dataType on an unresolved object \\?LASTOVERTIME", // https://github.com/elastic/elasticsearch/issues/134791 | ||
| // https://github.com/elastic/elasticsearch/issues/134793 | ||
| "class org.elasticsearch.compute.data..*Block cannot be cast to class org.elasticsearch.compute.data..*Block" | ||
| "time-series aggregation inside function * doesn't support type *" |
There was a problem hiding this comment.
I think this needs to be .* rather than *?
| ) | ||
| ); | ||
| assertThat( | ||
| error("TS test | STATS count(count_over_time(host)) BY bucket(@timestamp, 1 minute)", tsdb), |
There was a problem hiding this comment.
Should we maybe leave the old versions of these tests in as well, since we do still want these to not work, right?
There was a problem hiding this comment.
I'm not sure about this. We can restore it, but count(count_over_time(host)) doesn't work, while count(count_over_time(name)) does. This seems inconsistent since both are keyword fields, except host is a dimension field and name is not
|
Thanks Kostas! |
💔 Backport failed
You can use sqren/backport to manually backport by running |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
This change updates the validation to reject time-series aggregations that cannot be executed. Specifically, if FN(field) is rewritten to FN(last_over_time(field)), and we don't support last_over_time(field), we should throw an error during planning rather than at runtime. Closes elastic#136270 (cherry picked from commit 80b4b95)
) This change updates the validation to reject time-series aggregations that cannot be executed. Specifically, if FN(field) is rewritten to FN(last_over_time(field)), and we don't support last_over_time(field), we should throw an error during planning rather than at runtime. Closes #136270 (cherry picked from commit 80b4b95)
This change updates the validation to reject time-series aggregations that cannot be executed. Specifically, if FN(field) is rewritten to FN(last_over_time(field)), and we don't support last_over_time(field), we should throw an error during planning rather than at runtime. Closes elastic#136270
4 participants
This change updates the validation to reject time-series aggregations that cannot be executed. Specifically, if FN(field) is rewritten to FN(last_over_time(field)), and we don't support last_over_time(field), we should throw an error during planning rather than at runtime.
Closes #136270