SQL: Introduce Coalesce function #35253

costin · 2018-11-05T13:44:45Z

Add Coalesce conditional for replacing null values.

Fix #35060

P.S. This took longer than it should due to some gradle foobar. Also more tests need to be added on the having front.

Add Coalesce conditional for replacing null values Fix elastic#35060

elasticmachine · 2018-11-05T13:44:47Z

Pinging @elastic/es-search-aggs

costin · 2018-11-05T13:45:27Z

x-pack/plugin/sql/qa/src/main/resources/null.csv-spec

+//
+
+dateTimeOverNull
+SELECT YEAR(CAST(NULL AS DATE)) d;


@matriv notice that now null is supported in the CSV tests.

costin · 2018-11-05T13:45:45Z

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

@@ -114,6 +114,11 @@ public static void assertResultSetMetadata(ResultSet expected, ResultSet actual,
            if (expectedType == Types.FLOAT && expected instanceof CsvResultSet) {
                expectedType = Types.REAL;
            }
+            // csv doesn't support NULL type so skip type checking
+            if (actualType == Types.NULL && expected instanceof CsvResultSet) {


hack to get jdbc-csv to support null. /cc @matriv

astefan

Left few comments related to tests.
Also, thinking out loud, I'm wondering if wouldn't be better to add some Coalesce-specific random testing, where more nulls would be introduced in the list of values... atm, NodeSubclassTests would do this but it is probably not generating as many nulls as we would like to see for this specific function?

astefan · 2018-11-05T14:01:44Z

x-pack/plugin/sql/qa/src/main/resources/null.csv-spec

+
+
+divOfNull
+SELECT 5 / CAST(NULL AS FLOAT) + 10 AS n;


Why not SELECT 5 / NULL AS n, without casting?

Because I want to check that the a cast preserve the null value and its associated type. I've renamed the test and added a basic one without cast.

astefan · 2018-11-05T14:04:52Z

x-pack/plugin/sql/qa/src/main/resources/null.csv-spec

+
+c:i
+124
+;


Can you add a test where the first value is non-null? (all tests with COALESCE assume there is at least one null value before the first non-null)

Added (there's an optimization test that tries that but anyway).

astefan · 2018-11-05T14:11:52Z

x-pack/plugin/sql/qa/src/main/resources/null.csv-spec

+
+coalesceWithFirstNullOfNumber
+SELECT COALESCE(null, 123) AS c;
+


I would also add a test where all values are null.

it's done here: https://github.com/elastic/elasticsearch/pull/35253/files#diff-4fb3d311239a963fa6e56c5db4264130R28 no?

Hm... my bad, not sure why I haven't seen that.

matriv

Looks good! Left some comments/questions and request for more tests.

matriv · 2018-11-05T14:52:00Z

...src/main/java/org/elasticsearch/xpack/sql/expression/predicate/nulls/IsNotNullProcessor.java

@@ -15,7 +15,7 @@

    static final IsNotNullProcessor INSTANCE = new IsNotNullProcessor();

-    public static final String NAME = "inn";
+    public static final String NAME = "ninn";


why ninn? I'd prefer less cryptic name: isnotnull.

n=Null
inn=IsNotNull
which leads to
ninn=NullIsNotNull

Not that it is suppose to be readable. Since the name is saved in the serialization, we try to keep them short with some ad-hoc convention.

matriv · 2018-11-05T14:52:49Z

...ain/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/CoalesceProcessor.java

+
+public class CoalesceProcessor implements Processor {
+
+    public static final String NAME = "nco";


also here, I don't get the n

same as above:
n = null
co = coalesce

matriv · 2018-11-05T14:56:07Z

...sql/src/main/java/org/elasticsearch/xpack/sql/expression/predicate/conditional/Coalesce.java

+
+public class Coalesce extends ConditionalFunction {
+
+    private DataType dataType = DataType.NULL;


I don't like it so much to use NULL as it can be anything (depends on the arguments). I don't have a better suggestion for the moment though.

The dataType gets initialized on the first value - however if that is null (or the list is empty), the dataType remains the default, namely NULL (aka unknown) which I think it's appropriate.

yes. makes sense. thx!

matriv · 2018-11-05T14:59:01Z

.../plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/gen/pipeline/MultiPipe.java

+    }
+
+    public abstract Processor asProcessor(List<Processor> procs);
+}


don't we need equals() here (or in the parent class) to check that instances are of the same class?

No, because this check already exists. From Node#equals:

if (obj == null || getClass() != obj.getClass()) { return false; }

ah ok, overlooked that!

matriv · 2018-11-05T15:04:45Z

...ugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/FunctionRegistry.java

+        };
+        return def(function, builder, false, aliases);
+    }
+


Really really minor: can you remove the empty line or add it to the other places between the def variant and the iface?

Sure, I've added it to the rest (the source wasn't consistent).

matriv · 2018-11-05T15:21:49Z

x-pack/plugin/sql/qa/src/main/resources/null.sql-spec

+SELECT COALESCE(null, ABS(emp_no) + 1) AS c FROM test_emp ORDER BY emp_no LIMIT 5;
+
+coalesceHaving
+SELECT COALESCE(languages, 0) c FROM test_emp GROUP BY languages ORDER BY languages;


there is no having condition here. can we add one?

Also, can you add a test when COALESCE is used in the WHERE filter (no aggregations).

matriv

LGTM!

astefan

LGTM

Add Coalesce conditional for replacing null values Fix #35060 (cherry picked from commit 75e9a63)

…-agg * master: (528 commits) Register Azure max_retries setting (elastic#35286) add version 6.4.4 [Docs] Add painless context details for bucket_script (elastic#35142) Upgrade jline to 3.8.2 (elastic#35288) SQL: new SQL CLI logo (elastic#35261) Logger: Merge ESLoggerFactory into Loggers (elastic#35146) Docs: Add section about range query for range type (elastic#35222) [ILM] change remove-policy-from-index http method from DELETE to POST (elastic#35268) [CCR] Forgot missing return statement, SQL: Fix null handling for AND and OR in SELECT (elastic#35277) [TEST] Mute ChangePolicyForIndexIT#testChangePolicyForIndex Serialize ignore_throttled also to 6.6 after backport Check for java 11 in buildSrc (elastic#35260) [TEST] increase await timeout in RemoteClusterConnectionTests Add missing up-to-date configuration (elastic#35255) Adapt Lucene BWC version SQL: Introduce Coalesce function (elastic#35253) Upgrade to lucene-8.0.0-snapshot-31d7dfe6b1 (elastic#35224) Fix failing ICU tests (elastic#35207) Prevent throttled indices to be searched through wildcards by default (elastic#34354) ...

Follows: elastic#35253

Follows: #35253

SQL: Introduce Coalesce function

7225f38

Add Coalesce conditional for replacing null values Fix elastic#35060

costin added >enhancement v7.0.0 :Analytics/SQL SQL querying v6.6.0 labels Nov 5, 2018

costin requested review from astefan and matriv November 5, 2018 13:44

costin commented Nov 5, 2018

View reviewed changes

astefan reviewed Nov 5, 2018

View reviewed changes

matriv reviewed Nov 5, 2018

View reviewed changes

Address feedback

65605e8

costin force-pushed the fix-35060 branch from 3b7c720 to 65605e8 Compare November 5, 2018 21:06

matriv approved these changes Nov 5, 2018

View reviewed changes

astefan approved these changes Nov 6, 2018

View reviewed changes

Merge branch 'master' into fix-35060

193fbd6

costin merged commit 75e9a63 into elastic:master Nov 6, 2018

costin deleted the fix-35060 branch November 6, 2018 11:22

costin added a commit that referenced this pull request Nov 6, 2018

SQL: Introduce Coalesce function (#35253)

509a45c

Add Coalesce conditional for replacing null values Fix #35060 (cherry picked from commit 75e9a63)

matriv added a commit to matriv/elasticsearch that referenced this pull request Nov 20, 2018

SQL: [docs] Add documentation for COALESCE

6967afb

Follows: elastic#35253

matriv mentioned this pull request Nov 20, 2018

SQL: [docs] Add documentation for COALESCE #35740

Merged

matriv pushed a commit that referenced this pull request Nov 21, 2018

SQL: [docs] Add documentation for COALESCE (#35740)

b1818db

Follows: #35253

matriv pushed a commit that referenced this pull request Nov 21, 2018

SQL: [docs] Add documentation for COALESCE (#35740)

2f19570

Follows: #35253

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

astefan mentioned this pull request Jun 3, 2019

SQL: consider adding conditional expression support #32984

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Introduce Coalesce function #35253

SQL: Introduce Coalesce function #35253

costin commented Nov 5, 2018

elasticmachine commented Nov 5, 2018

costin Nov 5, 2018

costin Nov 5, 2018

astefan left a comment

astefan Nov 5, 2018

costin Nov 5, 2018

astefan Nov 5, 2018

costin Nov 5, 2018

astefan Nov 5, 2018

matriv Nov 5, 2018

astefan Nov 5, 2018

matriv left a comment

matriv Nov 5, 2018

costin Nov 5, 2018 •

edited

Loading

matriv Nov 5, 2018

costin Nov 5, 2018

matriv Nov 5, 2018

costin Nov 5, 2018

matriv Nov 5, 2018

matriv Nov 5, 2018

costin Nov 5, 2018

matriv Nov 5, 2018

matriv Nov 5, 2018

costin Nov 5, 2018

matriv Nov 5, 2018

matriv Nov 5, 2018

costin Nov 5, 2018

matriv left a comment

astefan left a comment

+              c:i
+
+              ;


		coalesceWithFirstNullOfNumber
		SELECT COALESCE(null, 123) AS c;


		public class CoalesceProcessor implements Processor {

		public static final String NAME = "nco";


		public class Coalesce extends ConditionalFunction {

		private DataType dataType = DataType.NULL;

SQL: Introduce Coalesce function #35253

SQL: Introduce Coalesce function #35253

Conversation

costin commented Nov 5, 2018

elasticmachine commented Nov 5, 2018

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

matriv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin Nov 5, 2018 • edited Loading

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

matriv left a comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

costin Nov 5, 2018 •

edited

Loading