-
Couldn't load subscription status.
- Fork 25.6k
ESQL: Enable new data types with created version #136327
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
Conversation
Enables `dense_vector` and `aggregate_metric_double` if all nodes in all of the target clusters support it. Relates to elastic#135193
ℹ️ 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?
|
| if (Build.current().isSnapshot()) { | ||
| // We only test behavior in release builds. Snapshot builds will have data types enabled that are still under construction. | ||
| return List.of(); | ||
| } |
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.
Moved this.
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Hi @nik9000, I've created a changelog YAML for you. |
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.
Nice, thanks @nik9000 ! I have only minor comments, and a suggestion for a follow-up (more tests!) that we can tackle separately.
| } | ||
|
|
||
| @Before | ||
| public void onlyOnSnapshot() throws IOException { |
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.
The name seems to suggest the opposite of the actual method body?
| public void onlyOnSnapshot() throws IOException { | |
| public void onlyOnProductionBuilds() throws IOException { |
| * Tests fetching {@code dense_vector} if possible. Uses the {@code dense_vector_agg_metric_double_if_fns} | ||
| * work around if required. | ||
| */ | ||
| public final void testFetchDenseVector() throws IOException { |
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.
We could use a similar test for aggregate metric double, no?
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.
👍
| } | ||
|
|
||
| @Before | ||
| public void onlyOnSnapshot() throws IOException { |
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.
Interesting! I thought that running tests in release mode will use release builds of all bwc versions as well. Is that not the case?
Can we add a comment to explain this? Also, should we double check our release tests whether they always should use release builds of bwc versions?
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'll add a comment, yeah. If you run:
./gradlew -p x-pack/plugin/esql/qa/server/multi-clusters/ clean v9.1.5#newToOld -Dbuild.snapshot=false -Dlicense.key="x-pack/license-tools/src/test/resources/public.key" -Dtests.jvm.argline="-Dbuild.snapshot=false" -Dtests.class='*All*'
while 9.1.5 is the tip of the 9.1 branch then we'll compile 9.1 and run it as a snapshot.
I'll ask around if this is intended.
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'll ask around if this is intended.
Well, not "intended", but not something we're likely to fix right now.
...qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/AllSupportedFieldsTestCase.java
Show resolved
Hide resolved
...qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/AllSupportedFieldsTestCase.java
Show resolved
Hide resolved
| * production but tests need more control | ||
| * @param useAggregateMetricDoubleWhenNotSupported does the query itself force us to use {@code aggregate_metric_double} fields | ||
| * even if the remotes don't report that they support the type? This exists because | ||
| * some remotes <strong>do</strong> support {@code aggregate_metric_double} without |
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.
We can be more precise here: the support is precisely on 9.2.0; we can link to the data type's supported transport version so that hackers can use git blame to confirm that.
Being precise allows us to refactor and clean this in 9.5, when we don't have to expect a mixed cluster state with 9.2.0 (or only during a rolling upgrade)
| * reporting that they do. And, for a while, we used the query itself to opt into | ||
| * reading these fields. | ||
| * @param useDenseVectorWhenNotSupported does the query itself force us to use {@code dense_vector} fields even if the remotes don't | ||
| * report that they support the type? This exists because some remotes <strong>do</strong> |
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 here.
| boolean useDenseVectorWhenNotSupported | ||
| ) { | ||
| /** | ||
| * The {@link #minTransportVersion}, but if any remote didn't tell us the version we assume |
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.
Can we say that, if a remote (cluster) didn't tell us the min version, it's at most on 9.1, because if all nodes where on 9.2.0+, the min transport version would be part of the field caps response?
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 could add that to the comment, but this is lowest version it could possibly be, not the highest. At least, that's what I think it should be.
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.
What you say is correct! I meant that it'd just be helpful to understand that the min transport version was added to the FC response in 9.2.0. Which means that if we didn't receive it, there is at least one cluster with at least one node that is on a previous 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.
Cool! I've pushed some updated comments.
| boolean typeSupported = type.supportedVersion() | ||
| .supports(fieldsInfo.effectiveMinTransportVersion(), fieldsInfo.currentBuildIsSnapshot) | ||
| || switch (type) { | ||
| case AGGREGATE_METRIC_DOUBLE -> fieldsInfo.useAggregateMetricDoubleWhenNotSupported; | ||
| case DENSE_VECTOR -> fieldsInfo.useDenseVectorWhenNotSupported; | ||
| default -> false; | ||
| }; | ||
| if (false == typeSupported) { | ||
| type = UNSUPPORTED; | ||
| } |
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.
Yeah, this is the main change and looks right!
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 change effectively adds support to agg metric double and dense vector fields in lookup indices and enrich policies.
Let's add a test case for using all kinds of types as lookup/enrich fields, right? (Not match fields, we have separate tests for that.) In a follow-up that is, this should be fine to merge without.
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.
👍
|
Follow ups we need:
|
💔 Backport failed
You can use sqren/backport to manually backport by running |
I'd made a mistake in elastic#136327 when writing the test for fetching fields that the release of 9.2.0 revealed. This fixes it and adds one more test that we needed from elastic#136327.
Enables `dense_vector` and `aggregate_metric_double` if all nodes in all of the target clusters support it. Relates to elastic#135193
|
Backport: #137073 |
Enables
dense_vectorandaggregate_metric_doubleif all nodes in all of the target clusters support it.Relates to #135193