- 
                Notifications
    
You must be signed in to change notification settings  - Fork 25.6k
 
ESQL: Update mechanism for under construction data types #135697
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
ESQL: Update mechanism for under construction data types #135697
Conversation
All the new types added in 9.2 will be considered supported by a remote node if its transport version is at least `INDEX_SOURCE`. We cannot add a new transport version and use that because it would break some queries already working on Serverless.
Rely on the SupportedVersion class, instead.
| 
           Hi @alex-spies, I've created a changelog YAML for you.  | 
    
| 
           Pinging @elastic/es-analytical-engine (Team:Analytics)  | 
    
| return new SupportedVersion() { | ||
| @Override | ||
| public boolean supports(TransportVersion version) { | ||
| return version.supports(supportedVersion) || Build.current().isSnapshot(); | 
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 is a subtle change that considers released types supported on snapshot builds, always.
Similarly, under construction types are also always considered supported on snapshot builds.
I thought about making a distinction between "created at version" and "supported from version", but that wouldn't really add anything as we already have capabilities to control the behavior of bwc tests in snapshot builds.
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'm good with just having the supported from version.
It does feel weird to say that released types are always supported on snapshots though.
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 to explain that this is so that we don't break a bunch of existing tests when finally enabling a 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.
Added some javadoc.
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 is an interesting compromise that makes tests flow more nicely. We don't discard any backwards compatibility tests that we wrote while working on the new type. That's good. But those tests might be guaranteeing more backwards compatibility than we actually offer in production. That's ok. And it makes it explicit when we disable that backward compatibility. All good.
| return new SupportedVersion() { | ||
| @Override | ||
| public boolean supports(TransportVersion version) { | ||
| return version.supports(supportedVersion) || Build.current().isSnapshot(); | 
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'm good with just having the supported from version.
It does feel weird to say that released types are always supported on snapshots though.
| private static String[] removeUnderConstruction(String[] types) { | ||
| for (Map.Entry<DataType, FeatureFlag> underConstruction : DataType.UNDER_CONSTRUCTION.entrySet()) { | ||
| if (underConstruction.getValue().isEnabled() == false) { | ||
| types = Arrays.stream(types).filter(t -> underConstruction.getKey().typeName().equals(t) == false).toArray(String[]::new); | 
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 suppose you could pass the feature flag into the under construction SupportVersion variant and use the enabled/disabled-ness of the feature flag. I'm wondering whether a createdVersion AND a featureFlag might be the right thing here. It's more work when you make a new data type, but it feels a little more true.
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 guess you could have a feature-flag based override to enable under-construction types outside of SNAPSHOT; e.g. so that folks toying around with in-development data types could enable them on their local cluster (and live with the fact that queries might break in unexpected ways, esp. if connected to a remote cluster that may not enable the same feature flag).
That said, we currently have no under construction types anymore, so that would complicate the system for no gain. But I could leave a comment in place so that future-us will know how to do that.
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.
Added a comment.
| * EsqlDataTypeConverter#commonType, individual function type checking, the | ||
| * verifier rules, or other places. We suggest starting with CSV tests and | ||
| * seeing where they fail.</li> | ||
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.
Extra
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.
Oh right.
I think it's fine to remove just that and not wait on the whole CI to run again before merging - that's what I'm gonna do.
| return new SupportedVersion() { | ||
| @Override | ||
| public boolean supports(TransportVersion version) { | ||
| return version.supports(supportedVersion) || Build.current().isSnapshot(); | 
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 is an interesting compromise that makes tests flow more nicely. We don't discard any backwards compatibility tests that we wrote while working on the new type. That's good. But those tests might be guaranteeing more backwards compatibility than we actually offer in production. That's ok. And it makes it explicit when we disable that backward compatibility. All good.
Close #135193
Follow-up to #135215
Fix the test expectations for our
AllSupportedFieldsITs according to #135215 (comment).Additionally, clean up our mechanism and docs for "under construction" data types:
CreatedVersionfor data types toSupportedVersion. For release builds, it doesn't matter when serialization code for a type was introduced; it matters only when it was actually released, and this is generally (much) later than when the serialization code for the type was written.SupportedVersionnon-optional in the data type builder, so that we don't accidentally mark a new data type as "supported on all versions" in the future.Point 1. prevents having to update existing capabilities when a new type is released (which was required for #135604 and #135213) and thus gives a little bit of extra safety by keeping bwc tests intact (even if they technically test versions that don't officially support a type, yet).
Point 4. is lining up our data type system for #131108.
Some context on under-construction data types
We have data types that are considered under construction until they are released. Older nodes may not know these types at all (not able to deserialize), or they may know them but not actually support them.
Previously, we had a feature flag-based mechanism that would determine if a type is supported or not. Unfortunately, we do not check the feature flags on remote nodes and clusters, so this mechanism alone doesn't work; moreover, it leads to broken queries when the coordinator considers a type supported, but a remote node can't even deserialize it. The remote node will then not participate in simple queries like
FROM logs-*and be counted as partial failure, even though it participated just fine in the past, when the data type was considered unsupported.That was fixed for the
aggregate_metric_doubleanddense_vectortypes, specifically, in the workaround #135215; these types now require the query to contain functions or commands that are only available when the types are actually released, otherwise we still consider these types as unsupported.Of course, that's not a long term solution. Long term, we want to consider types as supported if the minimum transport version of all participating nodes is at least the version when the type was declared released. This is #131108. (See the ongoing work in #135633).