Add support for VERSION field type in SQL and EQL by luigidellaquila · Pull Request #85502 · elastic/elasticsearch

+    /**
+     * Does the provided {@code version} support the version type (PR#85502)?
+     */
+    public static boolean supportsVersionFieldType(Version version) {


supportsVersionType()?

bpintea · 2022-05-30T20:19:00Z

...in/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTypesTests.java

        }
    }

+    public void testVersionFieldFiltering() {


testVersionTypeFiltering()?

bpintea · 2022-05-30T20:26:39Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/gen/script/Scripts.java


    public static ScriptTemplate nullSafeSort(ScriptTemplate script) {
-        String methodName = script.outputType().isNumeric() ? "nullSafeSortNumeric" : "nullSafeSortString";
+        String methodName;


Iis this change needed?

Just a leftover, fixed

bpintea · 2022-05-31T07:45:28Z

x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/EsType.java

    TIME(Types.TIME),
    DATETIME(Types.TIMESTAMP),
    IP(Types.VARCHAR),
+    VERSION(Types.VARCHAR),


Not appending into this enum might raise some bwc concerns (see #65145 (comment))

Good point, fixed

bpintea · 2022-05-31T08:33:44Z

.../src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java

            // since its later processing will be type dependent. (ex.: negation of UL is only "safe" for 0 values)
            return convert(values, UNSIGNED_LONG);
        }
+        if (dataType == VERSION && values instanceof String) {


The type check is safe, but wondering if necessary, since the Version converter will eventually do the same check.

👍 also seems redundant to me

bpintea · 2022-05-31T08:47:55Z

.../plugin/sql/qa/jdbc/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/ResultSetTestCase.java

+        // some higher versions
+        for (int i = 0; i < randomInt(10); i++) {
+            index("test", "" + (docId++), builder -> {
+                String versionVal = (2 + randomInt(50)) + "." + randomInt(50) + "." + randomInt(50);


Curious if there's any reason not to allow a major of 0 here.

Nothing very specific, I just needed a random dataset that did not collide with my WHERE conditions.
Looking at it now, probably it's a bit overkill to test a result set, but still it validates that the query returns correct results in a fairly randomized environment.

bpintea · 2022-05-31T08:52:10Z

.../plugin/sql/qa/jdbc/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/ResultSetTestCase.java

+        // bad version value
+        query = "SELECT name, version from test where version = 'foo'";
+        doWithQuery(query, results -> {
+            results.next();
+            assertEquals("version foo", results.getString("name"));
+            assertEquals("foo", results.getString("version"));
+            assertFalse(results.next());
+        });
+    }


Does this second test add anything to the first one? JDBC has no Version type knowledge so it can only convert to string, so the content doesn't really matter.

Since there is this distinction between valid and invalid versions, I thought it would be good to have both in the result set test. Anyway, I agree that knowing the implementation details this test is probably not crucial. I'll refactor it a bit to make it shorter.

bpintea · 2022-05-31T09:04:12Z

...gin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/FieldAttributeTests.java

        }
    }

+    public void testVersionFieldVersionCompatibility() {


testVersionTypeVersionCompatibilty()? SELECT '1.2.3'::version'... makes use of no field to test.

bpintea · 2022-05-31T09:09:45Z

.../sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumnsTests.java

        }
    }

+    public void testVersionFieldFiltering() {


testVersionTypeFiltering()?

bpintea · 2022-05-31T09:14:02Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/type/DataTypes.java

    // ip
    public static final DataType IP               = new DataType("ip",                45,                false, false, true);
+    // version
+    public static final DataType VERSION          = new DataType("version",           Integer.MAX_VALUE, false, false, true);


Giving VERSION a size of Integer.MAX_VALUE seems wrong, though I guess that's the theoretical max. Not sure if ES type enforces any limit? It seems there's only a recommendation, but no actual limit: https://semver.org/#does-semver-have-a-size-limit-on-the-version-string

Technically, any string can be indexed as a Version, so the limit is the same as a KEYWORD.

Luegg

👍

Luegg · 2022-06-01T11:50:12Z

x-pack/plugin/eql/qa/common/src/main/resources/test_extra.toml

+[[queries]]
+name = "sequenceWithVersionConcat"
+query = '''
+  sequence by transID


can a version also be the join key?

Good point, adding a specific test case

Luegg · 2022-06-01T11:53:17Z

...in/java/org/elasticsearch/xpack/ql/expression/predicate/operator/comparison/Comparisons.java

+            return compare(lN, rN);
+        }
+
+        // automatic conversion for versions


is this still needed? Shouldn't by now the runtime type always be Version?

It depends on how tolerant we want to be with automatic casts in situations like local folding (e.g WHERE '1.2.0' < '1.11.0'::version) and function evaluation (eg. IIF(version > '1.1', 1, 0)).

In general, we do not do it for local folding (eg. for numbers '2' > 1 returns false) but we do it for field queries (eg. id > '3' will evaluate to true even if id is numeric, same with version > 1.2).

IMHO this is an inconsistency: the same operation should have the same behavior, locally and in _search.
Since local folding is just an optimization or a fallback in most of the cases, so I tend to consider the automatic cast as the expected behavior.

So from my point of view we should leave it as it is

I would tend to not make an exception for Version. Since we have this distinction between local execution and queries it should at least be consistent across types.

Luegg · 2022-06-01T12:02:35Z

.../src/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java

            // since its later processing will be type dependent. (ex.: negation of UL is only "safe" for 0 values)
            return convert(values, UNSIGNED_LONG);
        }
+        if (dataType == VERSION && values instanceof String) {


👍 also seems redundant to me

luigidellaquila · 2022-06-01T14:17:05Z

@elasticmachine update branch

costin · 2022-06-08T09:58:20Z

...plugin/mapper-version/src/main/java/org/elasticsearch/xpack/versionfield/VersionEncoder.java

 * </ul>
 */
-class VersionEncoder {
+public class VersionEncoder {


Since the code touches on code outside QL, please find one of the authors/team responsible for the code to review these changes.
If only constants are being used, it's fine to copy them.

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.

costin · 2022-06-08T10:01:02Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/type/DataType.java

    }

+    public ScriptSortBuilder.ScriptSortType scriptSortType() {
+        if (isNumeric()) {


nit: return isNumeric() ? ScriptSortType.NUMBER : this == DataTypes.VERSION ? ScriptSortType.VERSION : ScriptSortType.STRING

costin · 2022-06-08T10:05:37Z

...plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java

     */
    public static XContentBuilder value(XContentBuilder builder, Mode mode, SqlVersion sqlVersion, Object value) throws IOException {
-        if (value instanceof ZonedDateTime zdt) {
+        if (value instanceof org.elasticsearch.xpack.versionfield.Version) {


Version is not a widely used type hence why it should be the else if not at the main if.

costin

Requesting changes regarding the impact on mapper-version project.

…sion_data_type_in_ql' into enhancement/support_version_data_type_in_ql

luigidellaquila · 2022-06-08T11:12:00Z

@elasticmachine update branch

…sion_data_type_in_ql' into enhancement/support_version_data_type_in_ql

costin

Thanks for reviewing the feedback. LGTM!

luigidellaquila · 2022-06-08T18:07:30Z

@elasticmachine update branch

This reverts commit 79b0078.

luigidellaquila · 2022-06-09T07:53:49Z

Had to revert due to the following:

va.lang.IllegalArgumentException: can not write type [class org.elasticsearch.xpack.versionfield.Version]
        at org.elasticsearch.server@8.4.0-SNAPSHOT/org.elasticsearch.common.io.stream.StreamOutput.writeGenericValue(StreamOutput.java:828)
        at org.elasticsearch.xpack.ql.expression.gen.processor.ConstantProcessor.writeTo(ConstantProcessor.java:68)
        at org.elasticsearch.server@8.4.0-SNAPSHOT/org.elasticsearch.common.io.stream.StreamOutput.writeNamedWriteable(StreamOutput.java:1097)
        at org.elasticsearch.server@8.4.0-SNAPSHOT/org.elasticsearch.common.io.stream.StreamOutput.writeNamedWriteableList(StreamOutput.java:1203)
        at org.elasticsearch.xpack.sql.expression.predicate.conditional.ConditionalProcessor.writeTo(ConditionalProcessor.java:73)
        at org.elasticsearch.server@8.4.0-SNAPSHOT/org.elasticsearch.common.io.stream.StreamOutput.writeNamedWriteable(StreamOutput.java:1097)
        at org.elasticsearch.xpack.ql.execution.search.extractor.ComputingExtractor.writeTo(ComputingExtractor.java:56)
        at org.elasticsearch.server@8.4.0-SNAPSHOT/org.elasticsearch.common.io.stream.StreamOutput.writeNamedWriteable(StreamOutput.java:1097)
        at org.elasticsearch.server@8.4.0-SNAPSHOT/org.elasticsearch.common.io.stream.StreamOutput.writeNamedWriteableList(StreamOutput.java:1203)
        at org.elasticsearch.xpack.sql.execution.search.SearchHitCursor.writeTo(SearchHitCursor.java:80)
        at org.elasticsearch.server@8.4.0-SNAPSHOT/org.elasticsearch.common.io.stream.StreamOutput.writeNamedWriteable(StreamOutput.java:1097)
        at org.elasticsearch.xpack.sql.session.Cursors.encodeToString(Cursors.java:82)
        at org.elasticsearch.xpack.sql.session.Cursors.encodeToString(Cursors.java:73)
        at org.elasticsearch.xpack.sql.plugin.TransportSqlQueryAction.createResponse(TransportSqlQueryAction.java:225)

The problem seems to be related to how Version objects are translated to strings to be serialized, so it should be deterministic, but strangely it's not (before merging the build was green and I didn't notice this error before during the tests)

Add support for VERSION field type in SQL and EQL

0e7f61a

luigidellaquila added >enhancement :Analytics/SQL SQL querying :Analytics/EQL EQL querying Team:QL (Deprecated) Meta label for query languages team labels Mar 30, 2022

luigidellaquila requested review from Luegg, astefan, bpintea and costin March 30, 2022 15:43

elasticsearchmachine added the v8.2.0 label Mar 30, 2022

Update docs/changelog/85502.yaml

f5c79f8

luigidellaquila commented Mar 30, 2022

View reviewed changes

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/type/DataTypeConverter.java Outdated Show resolved Hide resolved

luigidellaquila marked this pull request as draft March 30, 2022 16:04

luigidellaquila added 2 commits March 30, 2022 18:49

Add SQL csv-spec tests for VERSION fields

dbe5c67

Merge remote-tracking branch 'luigidellaquila/enhancement/support_ver…

c3e8803

…sion_data_type_in_ql' into enhancement/support_version_data_type_in_ql

salvatore-campagna added v8.3.0 and removed v8.2.0 labels Mar 30, 2022

Fix test cases

965b6c1

elasticmachine and others added 3 commits March 31, 2022 17:39

Merge branch 'master' into enhancement/support_version_data_type_in_ql

7d2361d

Fix test to consider v 8.3 as version of introduction of the feature

288b5f8

Fix frozen csv test cases

9f8cb6e

Merge branch 'master' into enhancement/support_version_data_type_in_ql

3a59ec8

elasticmachine and others added 2 commits March 31, 2022 19:33

Merge branch 'master' into enhancement/support_version_data_type_in_ql

f449255

Fix further tests

dfe4013

craigtaverner added v8.4.0 and removed v8.3.0 labels May 25, 2022

luigidellaquila added 2 commits May 26, 2022 12:01

Merge branch 'master' into enhancement/support_version_data_type_in_ql

b266021

Update version for bwc checks (to v 8.4.0)

4ddef7f

Merge branch 'master' into enhancement/support_version_data_type_in_ql

aaa77b0

bpintea mentioned this pull request May 31, 2022

Add Version type support elastic/elasticsearch-sql-odbc#311

Closed

bpintea approved these changes May 31, 2022

View reviewed changes

Luegg approved these changes Jun 1, 2022

View reviewed changes

Implement review suggestions

d8b5473

Merge branch 'master' into enhancement/support_version_data_type_in_ql

ff9139e

costin requested changes Jun 8, 2022

View reviewed changes

costin approved these changes Jun 8, 2022

View reviewed changes

costin requested changes Jun 8, 2022

View reviewed changes

luigidellaquila added 3 commits June 8, 2022 12:41

Revert unnescessary changes to mapper-version

e31a21f

Merge remote-tracking branch 'luigidellaquila/enhancement/support_ver…

dacefd6

…sion_data_type_in_ql' into enhancement/support_version_data_type_in_ql

Implement review suggestions

a92defd

elasticmachine and others added 3 commits June 8, 2022 21:12

Merge branch 'master' into enhancement/support_version_data_type_in_ql

9bfaf86

Merge remote-tracking branch 'luigidellaquila/enhancement/support_ver…

e59952d

…sion_data_type_in_ql' into enhancement/support_version_data_type_in_ql

Fix compile problems after merge from master

89aa381

costin approved these changes Jun 8, 2022

View reviewed changes

Fix failing test case (due to refactoring on master)

dd12a67

Merge branch 'master' into enhancement/support_version_data_type_in_ql

e644334

luigidellaquila merged commit 79b0078 into elastic:master Jun 9, 2022

luigidellaquila added a commit that referenced this pull request Jun 9, 2022

Revert "Add support for VERSION field type in SQL and EQL (#85502)"

3dc4176

This reverts commit 79b0078.

luigidellaquila mentioned this pull request Jun 13, 2022

Add support for VERSION field type in SQL and EQL #87590

Merged

Conversation

luigidellaquila commented Mar 30, 2022

Uh oh!

elasticmachine commented Mar 30, 2022

Uh oh!

elasticsearchmachine commented Mar 30, 2022

Uh oh!

Uh oh!

luigidellaquila commented Mar 31, 2022

Uh oh!

luigidellaquila commented Mar 31, 2022

Uh oh!

luigidellaquila commented Mar 31, 2022

Uh oh!

luigidellaquila commented Mar 31, 2022

Uh oh!

luigidellaquila commented May 30, 2022

Uh oh!

bpintea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Luegg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luigidellaquila commented Jun 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment