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 all commits
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 @@ -36,7 +36,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
75 changes: 75 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,75 @@
//
// 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
;


divOfCastedNull
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
;

divNoNull
SELECT 5 / null + 1 AS n;

n:i
null
;

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

c
null
;

coalesceFirstNotNull
SELECT COALESCE(123) AS c;

c
123
;


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).

12 changes: 12 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,12 @@
//
// Null expressions
//

coalesceField
SELECT COALESCE(null, ABS(emp_no) + 1) AS c FROM test_emp ORDER BY emp_no LIMIT 5;

coalesceHaving
SELECT COALESCE(null, ABS(MAX(emp_no)) + 1, 123) AS c FROM test_emp GROUP BY languages HAVING c > 100 ORDER BY languages LIMIT 5;

coalesceWhere
SELECT COALESCE(null, ABS(emp_no) + 1, 123) AS c FROM test_emp WHERE COALESCE(null, ABS(emp_no) + 1, 123, 321) > 100 ORDER BY emp_no NULLS FIRST LIMIT 5;
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 All @@ -347,6 +371,7 @@ static <T extends Function> FunctionDefinition def(Class<T> function,
};
return def(function, builder, true, aliases);
}

interface DatetimeUnaryFunctionBuilder<T> {
T build(Location location, Expression target, TimeZone tz);
}
Expand All @@ -373,6 +398,7 @@ static <T extends Function> FunctionDefinition def(Class<T> function,
};
return def(function, builder, false, aliases);
}

interface BinaryFunctionBuilder<T> {
T build(Location location, Expression lhs, Expression rhs);
}
Expand All @@ -391,6 +417,7 @@ private static FunctionDefinition def(Class<? extends Function> function, Functi
};
return new FunctionDefinition(primaryName, unmodifiableList(Arrays.asList(aliases)), function, datetime, realBuilder);
}

private interface FunctionBuilder {
Function build(Location location, List<Expression> children, boolean distinct, TimeZone tz);
}
Expand Down Expand Up @@ -450,6 +477,7 @@ private static <T extends Function> FunctionDefinition def(Class<T> function,
ctorRef.build(location, children.get(0), children.get(0).dataType());
return def(function, builder, false, aliases);
}

private interface CastFunctionBuilder<T> {
T build(Location location, Expression expression, DataType dataType);
}
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
Loading