-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Add field type for version strings #59773
Conversation
Pinging @elastic/es-search (:Search/Mapping) |
43d470b
to
2cd8f1d
Compare
This PR adds a new 'version' field type that allows indexing string values representing software versions similar to the ones defined in the Semantic Versioning definition (semver.org). The field behaves very similar to a 'keyword' field but allows efficient sorting and range queries that take into accound the special ordering needed for version strings. For example, the main version parts are sorted numerically (ie 2.0.0 < 11.0.0) whereas this wouldn't be possible with 'keyword' fields today. Valid version values are similar to the Semantic Versioning definition, with the notable exception that in addition to the "main" version consiting of major.minor.patch, we allow less or more than three numeric identifiers, i.e. "1.2" or "1.4.6.123.12" are treated as valid too. Relates to elastic#48878
2cd8f1d
to
343310c
Compare
9b249a1
to
08989e7
Compare
...ersionfield/src/main/java/org/elasticsearch/xpack/versionfield/VersionStringFieldMapper.java
Outdated
Show resolved
Hide resolved
...ersionfield/src/main/java/org/elasticsearch/xpack/versionfield/VersionStringFieldMapper.java
Show resolved
Hide resolved
...ersionfield/src/main/java/org/elasticsearch/xpack/versionfield/VersionStringFieldMapper.java
Show resolved
Hide resolved
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 was getting up to speed with the feature so I could help with reviews, and left some small comments. I haven't yet reviewed in detail though.
...k/plugin/versionfield/src/main/java/org/elasticsearch/xpack/versionfield/VersionEncoder.java
Outdated
Show resolved
Hide resolved
...k/plugin/versionfield/src/main/java/org/elasticsearch/xpack/versionfield/VersionEncoder.java
Outdated
Show resolved
Hide resolved
...k/plugin/versionfield/src/main/java/org/elasticsearch/xpack/versionfield/VersionEncoder.java
Outdated
Show resolved
Hide resolved
...ugin/versionfield/src/main/java/org/elasticsearch/xpack/versionfield/VersionFieldPlugin.java
Show resolved
Hide resolved
...gin/versionfield/src/test/java/org/elasticsearch/xpack/versionfield/VersionEncoderTests.java
Show resolved
Hide resolved
...ersionfield/src/main/java/org/elasticsearch/xpack/versionfield/VersionStringFieldMapper.java
Show resolved
Hide resolved
...k/plugin/versionfield/src/main/java/org/elasticsearch/xpack/versionfield/VersionEncoder.java
Outdated
Show resolved
Hide resolved
...k/plugin/versionfield/src/main/java/org/elasticsearch/xpack/versionfield/VersionEncoder.java
Show resolved
Hide resolved
...k/plugin/versionfield/src/main/java/org/elasticsearch/xpack/versionfield/VersionEncoder.java
Show resolved
Hide resolved
f1bf52f
to
67df1d3
Compare
...k/plugin/versionfield/src/main/java/org/elasticsearch/xpack/versionfield/VersionEncoder.java
Outdated
Show resolved
Hide resolved
...versionfield/src/test/java/org/elasticsearch/xpack/versionfield/VersionStringFieldTests.java
Show resolved
Hide resolved
...rsionfield/src/main/java/org/elasticsearch/xpack/versionfield/VersionFieldWildcardQuery.java
Outdated
Show resolved
Hide resolved
@mayya-sharipova thanks, I hope I adressed your last comments in ce51484 |
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.
@cbuescher Thanks for all these iterations of addressing feedback and thanks for a great field type, this PR LGTM.
I have just left small comments
.../versionfield/src/main/java/org/elasticsearch/xpack/versionfield/VersionScriptDocValues.java
Outdated
Show resolved
Hide resolved
...ersionfield/src/main/java/org/elasticsearch/xpack/versionfield/VersionStringFieldMapper.java
Outdated
Show resolved
Hide resolved
...ersionfield/src/main/java/org/elasticsearch/xpack/versionfield/VersionStringFieldMapper.java
Show resolved
Hide resolved
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 change looks good to me. I left one comment regarding the new script functions that I think should be discussed separately. I also wonder what family type this field should expose but that can also be decide in a follow up. So +1 to merge the current state since the main functionality we're after is there and consistent (accurate sort and range queries on Semver versions).
|
||
@Override | ||
public Query prefixQuery(String value, MultiTermQuery.RewriteMethod method, QueryShardContext context) { | ||
if (context.allowExpensiveQueries() == false) { |
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 you remove the check please
...in/versionfield/src/main/java/org/elasticsearch/xpack/versionfield/ValidationOnSortedDv.java
Outdated
Show resolved
Hide resolved
@mayya-sharipova @jimczi thanks for the reviews, I will work on backporting and open issues for follow ups where needed. |
This PR adds a new 'version' field type that allows indexing string values representing software versions similar to the ones defined in the Semantic Versioning definition (semver.org). The field behaves very similar to a 'keyword' field but allows efficient sorting and range queries that take into accound the special ordering needed for version strings. For example, the main version parts are sorted numerically (ie 2.0.0 < 11.0.0) whereas this wouldn't be possible with 'keyword' fields today. Valid version values are similar to the Semantic Versioning definition, with the notable exception that in addition to the "main" version consiting of major.minor.patch, we allow less or more than three numeric identifiers, i.e. "1.2" or "1.4.6.123.12" are treated as valid too. Relates to elastic#48878
This PR adds a new 'version' field type that allows indexing string values representing software versions similar to the ones defined in the Semantic Versioning definition (semver.org). The field behaves very similar to a 'keyword' field but allows efficient sorting and range queries that take into accound the special ordering needed for version strings. For example, the main version parts are sorted numerically (ie 2.0.0 < 11.0.0) whereas this wouldn't be possible with 'keyword' fields today. Valid version values are similar to the Semantic Versioning definition, with the notable exception that in addition to the "main" version consiting of major.minor.patch, we allow less or more than three numeric identifiers, i.e. "1.2" or "1.4.6.123.12" are treated as valid too. Relates to #48878
This PR adds a new 'version' field type that allows indexing string values
representing software versions similar to the ones defined in the Semantic
Versioning definition (semver.org). The field behaves very similar to a
'keyword' field but allows efficient sorting and range queries that take into
accound the special ordering needed for version strings. For example, the main
version parts are sorted numerically (ie 2.0.0 < 11.0.0) whereas this wouldn't
be possible with 'keyword' fields today.
Valid version values are similar to the Semantic Versioning definition, with the
notable exception that in addition to the "main" version consiting of
major.minor.patch, we allow less or more than three numeric identifiers, i.e.
"1.2" or "1.4.6.123.12" are treated as valid too.
Relates to #48878