-
Notifications
You must be signed in to change notification settings - Fork 25.5k
ES|QL: Make _tsid available in metadata #135204
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: Make _tsid available in metadata #135204
Conversation
Add _tsid into the list of available attributes in metadata. Closes elastic#133205
ℹ️ 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?
|
Add _tsid into the list of available attributes in metadata. Closes elastic#133205
…ata-list' into feature/esql-add-tsid-into-metadata-list # Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/action/EsqlQueryResponseTests.java
Add _tsid into the list of available attributes in metadata. Closes elastic#133205
…ata-list' into feature/esql-add-tsid-into-metadata-list
test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java
Outdated
Show resolved
Hide resolved
Add an esql capability for the _tsid metadata field. in metadata. Closes elastic#133205
Fix docs Closes elastic#133205
Add EsqlQueryResponseTests round trip test Closes elastic#133205
Pinging @elastic/es-analytical-engine (Team:Analytics) |
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
Hi @leontyevdv, I've created a changelog YAML for you. |
@Param( | ||
name = "field", | ||
type = { "boolean", "date", "date_nanos", "double", "integer", "ip", "keyword", "long", "text", "version" } | ||
type = { "boolean", "date", "date_nanos", "double", "integer", "ip", "keyword", "long", "text", "version", "_tsid" } |
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 don't think we need this, is it required?
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, to use it in CountDistinct. This class is an implementation of TimeSeriesAggregateFunction that creates an instance of CountDistinct here.
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 thought it's the other way round, CountDistinctOverTime
is built on top of CountDistinct
.
CountDistinctOverTime
is expected to return 1 per time-series so it's not that useful.. Then again, it doesn't hurt.
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.
@kkrik-es , you are right, we can avoid this change. It was there due to the tests for both CountDistinct and CountDistinctOverTime use the same list of parameters. I split them and removed the _tsid data type from CountDistinctOverTime. Thanks!
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 also added CrossClusterTimeSeriesIT.
); | ||
return isType( | ||
field(), | ||
dt -> (dt.isNumeric() && dt != DataType.UNSIGNED_LONG) || dt == DataType.TSID_DATA_TYPE, |
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.
Here, it looks weird to me, @dnhatn , but I didn't find the better way to do it. We use LastOverTime substitution in the TranslateTimeSeriesAggregate and I had to add the type into the function. Wdyt?
Fix tests and docs Closes elastic#133205
…ata-list' into feature/esql-add-tsid-into-metadata-list
Thanks, Dima! The changes look good to me. I'd like to re-discuss the purpose of exposing this with Kostas. Once we expose it, it will be hard to remove, especially since we're considering a different format for |
# Conflicts: # server/src/main/resources/transport/upper_bounds/9.2.csv # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
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 discussed this with Kostas. We agreed to expose the _tsid, but to keep the scope limited. We also prefer not to document this, as it is intended for internal or debugging purposes. LGTM, thanks Dima!
# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
Add cross-cluster IT. Closes elastic#133205
Add cross-cluster IT. Improve UT. Closes elastic#133205
The changes still show up in the md files, is it possible to avoid this? |
# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/LastOverTime.java
Remove _tsid from docs. Closes elastic#133205
Remove _tsid from docs. Closes elastic#133205
# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
@kkrik-es Done! We still need the _tsid in the functions but I excluded mentions of the data type from the documentation. See https://github.com/elastic/elasticsearch/pull/135204/files#diff-5b3a7bcaf3f57ee42ca359ddd440590dae3f4ddb24071a7587659ef6ad941fe3R1058 |
Neat! I'll let Nhat stamp, this is good stuff. |
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.
Thanks for removing the docs! Great work, Dima!
Add _tsid into the list of available attributes in metadata.
Closes #133205