QL: Refactor DataType for pluggability #51328

costin · 2020-01-22T18:05:54Z

Change DataType from enum to class
Break DataType enums into QL (default) and SQL types
Make data type conversion pluggable so that new types can be introduced

As part of the process:

static type conversion in QL package (such as Literal) has been
removed
several utility classes have been broken into base (QL) and extended
(SQL) parts based on type awareness
operators (+,-,/,*) are
due to extensibility, serialization of arithmetic operation has been
slightly changed and pushed down to the operator executor itself

elasticmachine · 2020-01-22T18:05:56Z

Pinging @elastic/es-search (:Search/EQL)

Change DataType from enum to class Break DataType enums into QL (default) and SQL types Make data type conversion pluggable so that new types can be introduced As part of the process: - static type conversion in QL package (such as Literal) has been removed - several utility classes have been broken into base (QL) and extended (SQL) parts based on type awareness - operators (+,-,/,*) are - due to extensibility, serialization of arithmetic operation has been slightly changed and pushed down to the operator executor itself

astefan

A first set of feedback.

astefan · 2020-01-24T14:29:40Z

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

-                (left == DataType.NULL || right == DataType.NULL)
-                    || (left.isString() && right.isString())
+                (left == NULL || right == NULL)
+                    || (isString(left) && isString(right))


Why this mix of styles? isSomething(DataType) vs. dataType.isSomething()
Is it possible to use only one?
Also, is not obvious to me why isNumeric() remains on the DataType while isString() is moved to DataTypes.

DataType contains only methods that can work on its properties (such as whether it's numeric or not) while everything else is on DataTypes.
From the usage point of view it would be cleaner to have all methods inside one place, namely DataTypes (as new types can have new properties such as interval) but in case of numeric properties these are parts of the DataType itself.
This can be revisited in the future so that these properties are fully externalized (and essentially hard-coded just like isString).

astefan · 2020-01-24T15:06:03Z

...in/java/org/elasticsearch/xpack/ql/expression/predicate/operator/arithmetic/Arithmetics.java

-        SUB
+    public interface NumericArithmetic extends BiFunction<Number, Number, Number> {
+        default Object wrap(Object l, Object r) {
+            if (!(l instanceof Number)) {


== false instead of ! ?

I've moved the code as it was without doing any style touches. I'd rather do that separately (similar to organize imports) to avoid the extra noise.
I'm sure there are multiple places where the != style is being used.

astefan · 2020-01-24T15:25:06Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/index/IndexResolver.java

-            case TEXT:
+        DataType esType = typeRegistry.fromEs(typeName);
+
+        if (esType == TEXT) {
                return new TextEsField(fieldName, props, false, isAlias);


Indentation issue.

astefan · 2020-01-24T15:25:37Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/index/IndexResolver.java

                return new UnsupportedEsField(fieldName, typeName, null, props);
-            default:
+        }
                return new EsField(fieldName, esType, props, isAggregateable, isAlias);
        }


Indentation issue.

Still an indentation issue.

astefan · 2020-01-24T15:32:18Z

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

+
+    DataType fromJava(Object value);
+
+    boolean isUnsupported(DataType type);


SqlDataTypeRegistry has DataTypes.isUnsupported(type); for this method implementation, DefaultDataTypeRegistry has return type == UNSUPPORTED;. Can this interface provide a default implementation and remove the specific ones (that seem to be the same) from the implementing classes?

A default implementation would tie the contract (the interface) to an DataType instance (UNSUPPORTED).
Delegating to *DataTypes is cleaner and makes each interface implementation clearer in design - it also makes them fully independent (if needed).
I replaced the == check with a call to DataTypes.isUnsupported to maintain the delegation pattern.

astefan · 2020-01-24T16:01:20Z

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

-    public int getPrecision() {
-        return esDataType.defaultPrecision;
-    }
+    //    public int getPrecision() {


Why is this method now available only for Keywords?

Because it's used only there. Having it all over does not provide any value.
Same as DateEsField which initially was suppose to maintain the date format however that's not the case so essentially it's just another EsField.

These APIs can be cleaned as not all properties are needed (including precision) and can be gathered into the module that needs them.

astefan · 2020-01-24T16:02:25Z

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

        this.precision = precision;
        this.normalized = normalized;
    }

-    @Override
    public int getPrecision() {


What does the precision mean in the context of a generic QL keyword field?

Being a string, it represents the number of characters - namely ignore_above.

astefan

LGTM. I left few minor comments.

astefan · 2020-01-25T15:33:00Z

x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/type/TypesTests.java

@@ -81,7 +79,7 @@ public void testDateField() {
        EsField field = mapping.get("date");
        assertThat(field.getDataType(), is(DATETIME));
        assertThat(field.isAggregatable(), is(true));
-        assertThat(field.getPrecision(), is(3));
+        //assertThat(field.getPrecision(), is(3));


Leftover comment.

astefan · 2020-01-27T12:05:54Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/index/IndexResolver.java

                return new UnsupportedEsField(fieldName, typeName, null, props);
-            default:
+        }
                return new EsField(fieldName, esType, props, isAggregateable, isAlias);
        }


Still an indentation issue.

astefan · 2020-01-27T12:07:20Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/index/IndexResolver.java

+            return new TextEsField(fieldName, props, false, isAlias);
+        }
+        if (esType == KEYWORD) {
+            int length = Short.MAX_VALUE;


I think you can remove length and use its value directly in the KeywordEsField consructor.
I would say that normalized can also be removed, and still keep the TODO in there?

astefan · 2020-01-27T12:11:46Z

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

-//FIXME: Taken from sql-proto.
-//Ideally it should be shared but the dependencies across projects and and SQL-client make it tricky.
-//Maybe a gradle task would fix that...
+//NB: Taken from sql-proto.


Remove this line?

I think it's good to have it as information on where the code comes from (though it's not big enough to require some sort of automatic copying over).

astefan · 2020-01-27T12:14:52Z

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

-                case KEYWORD:
-                    int length = intSetting(content.get("ignore_above"), esDataType.defaultPrecision);
+            } else if (esDataType == KEYWORD) {
+                    int length = intSetting(content.get("ignore_above"), Short.MAX_VALUE);


Indentation issues.

astefan · 2020-01-27T12:20:18Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/SqlDataTypes.java

+    private static final Map<String, DataType> SQL_TO_ES;
+
+    static {
+        Map<String, DataType> sqlToEs = new HashMap<>(mapSize(45));


Why the use of mapSize() method? For situations where a new type is added and the Map initialization is forgotten?

To allocate a map high enough so that, while adding items, it doesn't require reallocation.
In this case the map is a constant and we know before hand its size hence the 'optimization' to use from the start a size high enough that everything fits yet small enough to not waste space.

astefan · 2020-01-27T13:23:19Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java

@@ -1010,7 +1010,7 @@ private Expression propagate(And and) {
                                    if (comp != null) {
                                        // var cannot be equal to two different values at the same time
                                        if (comp != 0) {
-                                            return new Literal(and.source(), Boolean.FALSE, DataType.BOOLEAN);
+                                        return new Literal(and.source(), Boolean.FALSE, DataTypes.BOOLEAN);


Indentation issue. Also, check lines L1030-L1039, L1091, L1006, and L1111.

matriv

LGTM, Nice work! Left some minor mostly formatting related comments.

matriv · 2020-01-23T23:31:27Z

.../plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTypes.java

                .map(t -> asList(t.toString(),
-                        t.sqlType().getVendorTypeNumber(),
-                        DataTypes.precision(t),
+                        sqlType(t).getVendorTypeNumber(), precision(t),


minor: I would leave one arg per line as it was.

matriv · 2020-01-23T23:31:35Z

.../plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTypes.java

                        //no fixed precision scale SQL_FALSE
                        Boolean.FALSE,
                        // not auto-incremented
                        Boolean.FALSE,
                        null,
-                        DataTypes.metaSqlMinimumScale(t),
-                        DataTypes.metaSqlMaximumScale(t),
+                        metaSqlMinimumScale(t), metaSqlMaximumScale(t),


matriv · 2020-01-23T23:31:43Z

.../plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTypes.java

                        // SQL_DATA_TYPE - ODBC wants this to be not null
-                        DataTypes.metaSqlDataType(t),
-                        DataTypes.metaSqlDateTimeSub(t),
+                        metaSqlDataType(t), metaSqlDateTimeSub(t),


matriv · 2020-01-27T14:06:57Z

...n/ql/src/main/java/org/elasticsearch/xpack/ql/expression/function/scalar/ScalarFunction.java

+    //
+    // Script generation
+    //
+


minor: extra empty line

matriv · 2020-01-27T14:07:04Z

...n/ql/src/main/java/org/elasticsearch/xpack/ql/expression/function/scalar/ScalarFunction.java

+        throw new QlIllegalArgumentException("Cannot evaluate script for expression {}", exp);
+    }
+
+


minor: extra empty line

matriv · 2020-01-27T14:07:45Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/types/SqlTypesTests.java

+
+public class SqlTypesTests extends ESTestCase {
+
+


minor: extra empty line

matriv · 2020-01-27T14:07:51Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/types/SqlTypesTests.java

+        EsField gs = mapping.get("site");
+        assertThat(gs.getDataType().typeName(), is("geo_shape"));
+    }
+


minor: extra empty line

matriv · 2020-01-27T14:11:30Z

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

        return new Tuple<>(cmd, session);
    }

    public void testSysTypes() {
        Command cmd = sql("SYS TYPES").v1();

        List<String> names = asList("BYTE", "LONG", "BINARY", "NULL", "INTEGER", "SHORT", "HALF_FLOAT", "FLOAT", "DOUBLE", "SCALED_FLOAT",
-                "KEYWORD", "TEXT", "IP", "BOOLEAN", "DATE", "TIME", "DATETIME",
+                "IP", "KEYWORD", "TEXT", "BOOLEAN", "DATE", "TIME", "DATETIME",


Could you please share the logic in this re-arrangement?

Added a comment - reproducing here for the record:

ordered per SQL type id (numeric) and then (for the same id) alphabetically

Thank you, I reviewed the PR over a couple of days and I forgot some stuff.

matriv · 2020-01-27T14:29:16Z

x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/type/TypesTests.java

@@ -110,7 +108,7 @@ public void testDocValueField() {
        assertThat(mapping.size(), is(1));
        EsField field = mapping.get("session_id");
        assertThat(field, instanceOf(KeywordEsField.class));
-        assertThat(field.getPrecision(), is(15));
+        //assertThat(field.getPrecision(), is(15));


matriv · 2020-01-27T14:33:05Z

...g/elasticsearch/xpack/sql/expression/function/scalar/math/BinaryOptionalNumericFunction.java

@@ -88,7 +89,7 @@ public Expression replaceChildren(List<Expression> newChildren) {
    @Override
    public ScriptTemplate asScript() {
        ScriptTemplate leftScript = asScript(left);
-        ScriptTemplate rightScript = asOptionalScript(right);
+        ScriptTemplate rightScript = asScript(right == null ? Literal.NULL : right);


Personal preference: I kinda liked more the method asOptionaScript

I removed it since it was not used anywhere outside the base class.
The script generation needs improvement anyway as it has evolved beyond the initial requirements.

costin

Left comment

* Introduce reusable QL plugin for SQL and EQL (#50815) Extract reusable functionality from SQL into its own dedicated project QL. Implemented as a plugin, it provides common components across SQL and the upcoming EQL. While this commit is fairly large, for the most part it's just a big file move from sql package to the newly introduced ql. (cherry picked from commit ec1ac0d) * SQL: Fix incomplete registration of geo NamedWritables (cherry picked from commit e295763) * QL: Extend NodeSubclass to read classes from jars (#50866) As the test classes are spread across more than one project, the Gradle classpath contains not just folders but also jars. This commit allows the test class to explore the archive content and load matching classes from said source. (cherry picked from commit 25ad749) * QL: Remove implicit conversion inside Literal (#50962) Literal constructor makes an implicit conversion for each value given which turns out has some subtle side-effects. Improve MathProcessors to preserve numeric type where possible Fix bug on issue compatibility between date and intervals Preserve the source when folding inside the Optimizer (cherry picked from commit 9b73e22) * QL: Refactor DataType for pluggability (#51328) Change DataType from enum to class Break DataType enums into QL (default) and SQL types Make data type conversion pluggable so that new types can be introduced As part of the process: - static type conversion in QL package (such as Literal) has been removed - several utility classes have been broken into base (QL) and extended (SQL) parts based on type awareness - operators (+,-,/,*) are - due to extensibility, serialization of arithmetic operation has been slightly changed and pushed down to the operator executor itself (cherry picked from commit aebda81) * Compilation fixes for 7.x

costin added :Analytics/SQL SQL querying :Analytics/EQL EQL querying labels Jan 22, 2020

costin requested review from astefan, bpintea and matriv January 22, 2020 18:05

costin mentioned this pull request Jan 22, 2020

QL: Make DataType pluggable - WIP #51218

Closed

costin added the >refactoring label Jan 22, 2020

costin force-pushed the ql/datatype-refactor branch from 0cb5288 to 8bff48b Compare January 22, 2020 18:17

Fix uncaught test failure

153e6b9

This was referenced Jan 23, 2020

Extract common/reusable components from SQL for EQL #49773

Closed

Build EQL Query Parser and planner on top of reusable xQL components #49997

Closed

astefan reviewed Jan 24, 2020

View reviewed changes

costin added 2 commits January 24, 2020 20:34

Polish

457e91d

Merge branch 'master' into ql/datatype-refactor

dc2f108

astefan approved these changes Jan 27, 2020

View reviewed changes

Polish

6d6a8d1

matriv approved these changes Jan 27, 2020

View reviewed changes

costin commented Jan 27, 2020

View reviewed changes

costin merged commit aebda81 into elastic:master Jan 27, 2020

costin deleted the ql/datatype-refactor branch January 27, 2020 15:33

costin mentioned this pull request Jan 28, 2020

QL: Backport project to 7.x #51497

Merged


		DataType fromJava(Object value);

		boolean isUnsupported(DataType type);

		throw new QlIllegalArgumentException("Cannot evaluate script for expression {}", exp);
		}

QL: Refactor DataType for pluggability #51328

QL: Refactor DataType for pluggability #51328

Conversation

costin commented Jan 22, 2020

elasticmachine commented Jan 22, 2020

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment