-
Notifications
You must be signed in to change notification settings - Fork 25k
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 support for VERSION field type in SQL and EQL #85502
Changes from 30 commits
0e7f61a
f5c79f8
dbe5c67
c3e8803
965b6c1
7d2361d
288b5f8
9f8cb6e
3a59ec8
f449255
dfe4013
c56cf37
22936d2
2f16de6
43402e3
85697ad
0a0eaf6
feb82fe
eae32e1
49a643b
401d12e
c5040b2
9e52c2f
c9fe537
4384010
b266021
4ddef7f
aaa77b0
d8b5473
ff9139e
e31a21f
dacefd6
a92defd
9bfaf86
e59952d
89aa381
dd12a67
e644334
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
pr: 85502 | ||
summary: Add support for VERSION field type in SQL and EQL | ||
area: Query Languages | ||
type: enhancement | ||
issues: | ||
- 83375 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,6 +90,9 @@ | |
}, | ||
"bool" : { | ||
"type" : "boolean" | ||
}, | ||
"version" : { | ||
"type" : "version" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
{ | ||
"properties" : { | ||
"event" : { | ||
"properties" : { | ||
"category" : { | ||
"type" : "keyword" | ||
} | ||
} | ||
}, | ||
"@timestamp" : { | ||
"type" : "date" | ||
}, | ||
"version_number" : { | ||
"type" : "version" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,7 +35,7 @@ | |
* lexically in ASCII sort order. Numeric identifiers always have lower precedence than non-numeric identifiers. | ||
* </ul> | ||
*/ | ||
class VersionEncoder { | ||
public class VersionEncoder { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the code touches on code outside QL, please find one of the authors/team responsible for the code to review these changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, actually these changes are a leftover from the previous attempts to support Version without impacting on the Search/Painless implementation, but since we went with a more complete solution (see #85990), we don't need them anymore. |
||
|
||
public static final byte NUMERIC_MARKER_BYTE = (byte) 0x01; | ||
public static final byte PRERELEASE_SEPARATOR_BYTE = (byte) 0x02; | ||
|
@@ -175,7 +175,7 @@ static boolean legalVersionString(VersionParts versionParts) { | |
return legalMainVersion && legalPreRelease && legalBuildSuffix; | ||
} | ||
|
||
static class EncodedVersion { | ||
public static class EncodedVersion { | ||
|
||
public final boolean isLegal; | ||
public final boolean isPreRelease; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,8 @@ | |
*/ | ||
package org.elasticsearch.xpack.ql.expression.predicate.operator.comparison; | ||
|
||
import org.elasticsearch.xpack.versionfield.Version; | ||
|
||
import java.math.BigInteger; | ||
import java.util.Set; | ||
|
||
|
@@ -71,13 +73,21 @@ static Integer compare(Object l, Object r) { | |
return null; | ||
} | ||
// typical number comparison | ||
if (l instanceof Number && r instanceof Number) { | ||
return compare((Number) l, (Number) r); | ||
if (l instanceof Number lN && r instanceof Number rN) { | ||
return compare(lN, rN); | ||
} | ||
|
||
// automatic conversion for versions | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this still needed? Shouldn't by now the runtime type always be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It depends on how tolerant we want to be with automatic casts in situations like local folding (e.g In general, we do not do it for local folding (eg. for numbers IMHO this is an inconsistency: the same operation should have the same behavior, locally and in _search. So from my point of view we should leave it as it is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would tend to not make an exception for |
||
if (l instanceof Version lV && r instanceof String rStr) { | ||
return lV.compareTo(new Version(rStr)); | ||
} | ||
if (l instanceof String lStr && r instanceof Version rV) { | ||
return new Version(lStr).compareTo(rV); | ||
} | ||
|
||
if (l instanceof Comparable && r instanceof Comparable) { | ||
if (l instanceof Comparable lC && r instanceof Comparable) { | ||
try { | ||
return Integer.valueOf(((Comparable) l).compareTo(r)); | ||
return Integer.valueOf(lC.compareTo(r)); | ||
} catch (ClassCastException cce) { | ||
// when types are not compatible, cce is thrown | ||
// fall back to null | ||
|
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 a version also be the join key?
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.
Good point, adding a specific test case