Skip to content
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

SQL: Introduce Coalesce function #35253

Merged
merged 3 commits into from
Nov 6, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,18 @@
import org.elasticsearch.test.rest.ESRestTestCase;
import org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase;

import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue;

import java.net.URISyntaxException;
import java.nio.file.Files;
import java.nio.file.Path;

import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue;

/**
* Integration test for the rest sql action. The one that speaks json directly to a
* user rather than to the JDBC driver or CLI.
*/
public class RestSqlIT extends RestSqlTestCase {
static final boolean SSL_ENABLED = Booleans.parseBoolean(System.getProperty("tests.ssl.enabled"));
static final boolean SSL_ENABLED = Booleans.parseBoolean(System.getProperty("tests.ssl.enabled"), false);

static Settings securitySettings() {
String token = basicAuthHeaderValue("test_admin", new SecureString("x-pack-test-password".toCharArray()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ public void testShowFunctions() throws IOException {
while (aggregateFunction.matcher(line).matches()) {
line = readLine();
}
Pattern conditionalFunction = Pattern.compile("\\s*[A-Z0-9_~]+\\s*\\|\\s*CONDITIONAL\\s*");
while (conditionalFunction.matcher(line).matches()) {
line = readLine();
}
Pattern scalarFunction = Pattern.compile("\\s*[A-Z0-9_~]+\\s*\\|\\s*SCALAR\\s*");
while (scalarFunction.matcher(line).matches()) {
line = readLine();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public static List<Object[]> readScriptSpec() throws Exception {
tests.addAll(readScriptSpec("/columns.csv-spec", parser));
tests.addAll(readScriptSpec("/datetime.csv-spec", parser));
tests.addAll(readScriptSpec("/alias.csv-spec", parser));
tests.addAll(readScriptSpec("/nulls.csv-spec", parser));
tests.addAll(readScriptSpec("/null.csv-spec", parser));
tests.addAll(readScriptSpec("/nested.csv-spec", parser));
tests.addAll(readScriptSpec("/functions.csv-spec", parser));
tests.addAll(readScriptSpec("/math.csv-spec", parser));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

expectedType = Types.NULL;
}

// when lenient is used, an int is equivalent to a short, etc...
assertEquals("Different column type for column [" + expectedName + "] (" + JDBCType.valueOf(expectedType) + " != "
+ JDBCType.valueOf(actualType) + ")", expectedType, actualType);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public static List<Object[]> readScriptSpec() throws Exception {
tests.addAll(readScriptSpec("/arithmetic.sql-spec", parser));
tests.addAll(readScriptSpec("/string-functions.sql-spec", parser));
tests.addAll(readScriptSpec("/case-functions.sql-spec", parser));
tests.addAll(readScriptSpec("/null.sql-spec", parser));
return tests;
}

Expand Down
1 change: 1 addition & 0 deletions x-pack/plugin/sql/qa/src/main/resources/command.csv-spec
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ SKEWNESS |AGGREGATE
STDDEV_POP |AGGREGATE
SUM_OF_SQUARES |AGGREGATE
VAR_POP |AGGREGATE
COALESCE |CONDITIONAL
DAY |SCALAR
DAYNAME |SCALAR
DAYOFMONTH |SCALAR
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugin/sql/qa/src/main/resources/docs.csv-spec
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ SKEWNESS |AGGREGATE
STDDEV_POP |AGGREGATE
SUM_OF_SQUARES |AGGREGATE
VAR_POP |AGGREGATE
COALESCE |CONDITIONAL
DAY |SCALAR
DAYNAME |SCALAR
DAYOFMONTH |SCALAR
Expand Down
60 changes: 60 additions & 0 deletions x-pack/plugin/sql/qa/src/main/resources/null.csv-spec
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
//
// Null expressions
//

dateTimeOverNull
SELECT YEAR(CAST(NULL AS DATE)) d;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


d:i
null
;

addOfNull
SELECT CAST(NULL AS INT) + CAST(NULL AS FLOAT) AS n;

n:d
null
;


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


n:d
null
;

coalesceJustWithNull
SELECT COALESCE(null, null, null) AS c;

c
null
;

coalesceWithFirstNullOfString
SELECT COALESCE(null, 'first') AS c;

c:s
first
;

coalesceWithFirstNullOfNumber
SELECT COALESCE(null, 123) AS c;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

c:i
123
;

coalesceMixed
SELECT COALESCE(null, 123, null, 321) AS c;

c:i
123
;

coalesceScalar
SELECT COALESCE(null, ABS(123) + 1) AS c;

c:i
124
;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

9 changes: 9 additions & 0 deletions x-pack/plugin/sql/qa/src/main/resources/null.sql-spec
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
//
// Null expressions
//

coalesceField
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

25 changes: 0 additions & 25 deletions x-pack/plugin/sql/qa/src/main/resources/nulls.csv-spec

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import java.util.Locale;
import java.util.function.Predicate;

import static java.lang.String.format;
import static java.util.Collections.emptyList;
import static java.util.Collections.emptyMap;

Expand Down Expand Up @@ -103,6 +104,10 @@ public static String name(Expression e) {
return e instanceof NamedExpression ? ((NamedExpression) e).name() : e.nodeName();
}

public static boolean isNull(Expression e) {
return e.dataType() == DataType.NULL || (e.foldable() && e.fold() == null);
}

public static List<String> names(Collection<? extends Expression> e) {
List<String> names = new ArrayList<>(e.size());
for (Expression ex : e) {
Expand Down Expand Up @@ -137,6 +142,14 @@ public static Pipe pipe(Expression e) {
throw new SqlIllegalArgumentException("Cannot create pipe for {}", e);
}

public static List<Pipe> pipe(List<Expression> expressions) {
List<Pipe> pipes = new ArrayList<>(expressions.size());
for (Expression e : expressions) {
pipes.add(pipe(e));
}
return pipes;
}

public static TypeResolution typeMustBeBoolean(Expression e, String operationName, ParamOrdinal paramOrd) {
return typeMustBe(e, dt -> dt == DataType.BOOLEAN, operationName, paramOrd, "boolean");
}
Expand All @@ -161,27 +174,18 @@ public static TypeResolution typeMustBeNumericOrDate(Expression e, String operat
return typeMustBe(e, dt -> dt.isNumeric() || dt == DataType.DATE, operationName, paramOrd, "numeric", "date");
}

private static TypeResolution typeMustBe(Expression e,
public static TypeResolution typeMustBe(Expression e,
Predicate<DataType> predicate,
String operationName,
ParamOrdinal pOrd,
ParamOrdinal paramOrd,
String... acceptedTypes) {

return predicate.test(e.dataType()) || DataTypes.isNull(e.dataType())?
TypeResolution.TYPE_RESOLVED :
new TypeResolution(incorrectTypeErrorMessage(e, operationName, pOrd, acceptedTypes));

}

private static String incorrectTypeErrorMessage(Expression e,
String operationName,
ParamOrdinal paramOrd,
String... acceptedTypes) {
return String.format(Locale.ROOT, "[%s]%s argument must be [%s], found value [%s] type [%s]",
operationName,
paramOrd == null || paramOrd == ParamOrdinal.DEFAULT ? "" : " " + paramOrd.name().toLowerCase(Locale.ROOT),
Strings.arrayToDelimitedString(acceptedTypes, " or "),
Expressions.name(e),
e.dataType().esType);
}
}
new TypeResolution(format(Locale.ROOT, "[%s]%s argument must be [%s], found value [%s] type [%s]",
operationName,
paramOrd == null || paramOrd == ParamOrdinal.DEFAULT ? "" : " " + paramOrd.name().toLowerCase(Locale.ROOT),
Strings.arrayToDelimitedString(acceptedTypes, " or "),
Expressions.name(e),
e.dataType().esType));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
import org.elasticsearch.xpack.sql.expression.function.scalar.string.Space;
import org.elasticsearch.xpack.sql.expression.function.scalar.string.Substring;
import org.elasticsearch.xpack.sql.expression.function.scalar.string.UCase;
import org.elasticsearch.xpack.sql.expression.predicate.conditional.Coalesce;
import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.Mod;
import org.elasticsearch.xpack.sql.parser.ParsingException;
import org.elasticsearch.xpack.sql.tree.Location;
Expand Down Expand Up @@ -142,6 +143,8 @@ private void defineDefaultFunctions() {
def(Skewness.class, Skewness::new),
def(Kurtosis.class, Kurtosis::new));
// Scalar functions
// conditional
addToMap(def(Coalesce.class, Coalesce::new));
// Date
addToMap(def(DayName.class, DayName::new, "DAYNAME"),
def(DayOfMonth.class, DayOfMonth::new, "DAYOFMONTH", "DAY", "DOM"),
Expand Down Expand Up @@ -310,6 +313,26 @@ static <T extends Function> FunctionDefinition def(Class<T> function,
return def(function, builder, false, aliases);
}

/**
* Build a {@linkplain FunctionDefinition} for multi-arg function that
* is not aware of time zone and does not support {@code DISTINCT}.
*/
@SuppressWarnings("overloads") // These are ambiguous if you aren't using ctor references but we always do
static <T extends Function> FunctionDefinition def(Class<T> function,
MultiFunctionBuilder<T> ctorRef, String... aliases) {
FunctionBuilder builder = (location, children, distinct, tz) -> {
if (distinct) {
throw new IllegalArgumentException("does not support DISTINCT yet it was specified");
}
return ctorRef.build(location, children);
};
return def(function, builder, false, aliases);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

interface MultiFunctionBuilder<T> {
T build(Location location, List<Expression> children);
}

/**
* Build a {@linkplain FunctionDefinition} for a unary function that is not
* aware of time zone but does support {@code DISTINCT}.
Expand All @@ -325,6 +348,7 @@ static <T extends Function> FunctionDefinition def(Class<T> function,
};
return def(function, builder, false, aliases);
}

interface DistinctAwareUnaryFunctionBuilder<T> {
T build(Location location, Expression target, boolean distinct);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@
import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
import org.elasticsearch.xpack.sql.expression.function.aggregate.AggregateFunction;
import org.elasticsearch.xpack.sql.expression.function.scalar.ScalarFunction;
import org.elasticsearch.xpack.sql.expression.predicate.conditional.ConditionalFunction;


public enum FunctionType {

AGGREGATE(AggregateFunction.class),
CONDITIONAL(ConditionalFunction.class),
SCALAR(ScalarFunction.class),
SCORE(Score.class);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,10 @@
import org.elasticsearch.xpack.sql.expression.gen.processor.ConstantProcessor;
import org.elasticsearch.xpack.sql.expression.gen.processor.HitExtractorProcessor;
import org.elasticsearch.xpack.sql.expression.gen.processor.Processor;
import org.elasticsearch.xpack.sql.expression.predicate.IsNotNullProcessor;
import org.elasticsearch.xpack.sql.expression.predicate.conditional.CoalesceProcessor;
import org.elasticsearch.xpack.sql.expression.predicate.logical.BinaryLogicProcessor;
import org.elasticsearch.xpack.sql.expression.predicate.logical.NotProcessor;
import org.elasticsearch.xpack.sql.expression.predicate.nulls.IsNotNullProcessor;
import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.BinaryArithmeticProcessor;
import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.UnaryArithmeticProcessor;
import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.BinaryComparisonProcessor;
Expand Down Expand Up @@ -58,6 +59,7 @@ public static List<NamedWriteableRegistry.Entry> getNamedWriteables() {
entries.add(new Entry(Processor.class, BinaryLogicProcessor.NAME, BinaryLogicProcessor::new));
entries.add(new Entry(Processor.class, NotProcessor.NAME, NotProcessor::new));
// null
entries.add(new Entry(Processor.class, CoalesceProcessor.NAME, CoalesceProcessor::new));
entries.add(new Entry(Processor.class, IsNotNullProcessor.NAME, IsNotNullProcessor::new));

// arithmetic
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,10 @@
import org.elasticsearch.xpack.sql.expression.function.scalar.string.ReplaceFunctionProcessor;
import org.elasticsearch.xpack.sql.expression.function.scalar.string.StringProcessor.StringOperation;
import org.elasticsearch.xpack.sql.expression.function.scalar.string.SubstringFunctionProcessor;
import org.elasticsearch.xpack.sql.expression.predicate.IsNotNullProcessor;
import org.elasticsearch.xpack.sql.expression.predicate.logical.BinaryLogicProcessor.BinaryLogicOperation;
import org.elasticsearch.xpack.sql.expression.predicate.conditional.CoalesceProcessor;
import org.elasticsearch.xpack.sql.expression.predicate.logical.NotProcessor;
import org.elasticsearch.xpack.sql.expression.predicate.nulls.IsNotNullProcessor;
import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.BinaryArithmeticProcessor.BinaryArithmeticOperation;
import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.UnaryArithmeticProcessor.UnaryArithmeticOperation;
import org.elasticsearch.xpack.sql.expression.predicate.operator.comparison.BinaryComparisonProcessor.BinaryComparisonOperation;
Expand Down Expand Up @@ -119,6 +120,13 @@ public static Boolean in(Object value, List<Object> values) {
return InProcessor.apply(value, values);
}

//
// Null
//
public static Object coalesce(List<Object> expressions) {
return CoalesceProcessor.apply(expressions);
}

//
// Regex
//
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

package org.elasticsearch.xpack.sql.expression.gen.pipeline;

import org.elasticsearch.xpack.sql.expression.Expression;
import org.elasticsearch.xpack.sql.expression.gen.processor.Processor;
import org.elasticsearch.xpack.sql.tree.Location;

import java.util.ArrayList;
import java.util.List;

public abstract class MultiPipe extends Pipe {

protected MultiPipe(Location location, Expression expression, List<Pipe> children) {
super(location, expression, children);
}

@Override
public Processor asProcessor() {
List<Processor> procs = new ArrayList<>();
for (Pipe pipe : children()) {
procs.add(pipe.asProcessor());
}

return asProcessor(procs);
}

public abstract Processor asProcessor(List<Processor> procs);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok, overlooked that!

Loading