-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[ES|QL] Enable aggregate_metric_double outside of snapshots #135213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ES|QL] Enable aggregate_metric_double outside of snapshots #135213
Conversation
1caa2f6 to
e5f891d
Compare
e5f891d to
2d7876c
Compare
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
a26322e to
77110f6
Compare
282100e to
54d4da8
Compare
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
| FROM test-* | ||
| | EVAL a = TO_AGGREGATE_METRIC_DOUBLE(1) // Temporary workaround to enable aggregate_metric_double | ||
| | WHERE k8s.pod.uid == \"947e4ced-1786-4e53-9e0c-5c447e959507\" | ||
| | WHERE k8s.pod.uid == "947e4ced-1786-4e53-9e0c-5c447e959507" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out this test (Stats from downsampled and non-downsampled index simultaneously with implicit casting) wasn't being run due to a typo in one of the capability names, so I had to make a couple of adjustments to it after changing the capabilities
| esql.query: | ||
| body: | ||
| query: "TS test-* | STATS max = max(k8s.pod.network.rx) | LIMIT 100" | ||
| query: "FROM test-* | STATS max = max(k8s.pod.network.rx) | LIMIT 100" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same test as above comment (Stats from downsampled and non-downsampled index simultaneously with implicit casting), since the behavior of TS changed to wrap fields with last_over_time before being handled with non-time-series aggregations, its results would also be different, so I modified the query to produce the same results as before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth adding back in the other query as a second test, with different output. We can do that in a follow up though. (I think we need to really go over the CSV tests during feature freeze and make sure our coverage is good)
| * @see DataType#isRepresentable(DataType) | ||
| */ | ||
| public static TypeResolution isRepresentableExceptCounters(Expression e, String operationName, ParamOrdinal paramOrd) { | ||
| return isType(e, DataType::isRepresentable, operationName, paramOrd, "any type except counter types"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this and isRepresentableExceptCountersAndSpatial?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is technically being used in MvSort (which I didn't need to modify for these tests interestingly), but it would definitely make sense to use to use isRepresentableExceptCountersAndAggregateMetricDouble there
The latter is unused indeed though
I can remove in a follow-up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind i removed because i had to fix merge conflicts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Will leave it to Mark or Nhat to stamp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
| /** | ||
| * Enable aggregate_metric_double in non-snapshot builds | ||
| */ | ||
| AGGREGATE_METRIC_DOUBLE_V0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not familiar with this pattern of deprecating capabilities and replacing them with a new one. Where did this come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the same format as what we did for the TS command so I just mimicked what was there
| "boolean, cartesian_point, cartesian_shape, datetime, date_nanos, double, geo_point, geo_shape, integer, ip, keyword, long," | ||
| + " semantic_text, text, unsigned_long or version"; | ||
| "boolean, cartesian_point, cartesian_shape, date_nanos, datetime, double, geo_point, geo_shape, geohash, geohex, geotile, " | ||
| + "integer, ip, keyword, long, text, unsigned_long or version"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be included? It looks unrelated. Likewise, in NotEqualsErrorTests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this was intentional (same with NotEqualsErrorTest); this code path wasn't being hit prior, but taking AggregateMetricDouble out of under_construction pulled it into these tests and exposed that the error message was ever so slightly different for whatever reason
| esql.query: | ||
| body: | ||
| query: "TS test-* | STATS max = max(k8s.pod.network.rx) | LIMIT 100" | ||
| query: "FROM test-* | STATS max = max(k8s.pod.network.rx) | LIMIT 100" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth adding back in the other query as a second test, with different output. We can do that in a follow up though. (I think we need to really go over the CSV tests during feature freeze and make sure our coverage is good)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks Larisa!
|
All tests passed, just status wasn't reported for one of them, so I am merging |
This commit enables aggregate_metric_double in ES|QL in non-snapshot builds
Many tests (such as the multi-value tests) were modified in order to handle lack-of-support of aggregate_metric_double and should be addressed in follow-up tasks