DRILL-665: Handle null values in case expressions (contd). 1. Added functions for converting REQUIRED holder into NULLABLE holder where the minorType is same. 2. Update in Optiq->Drill literal conversion. First check if it null type, before parsing the literal value. Parsing literal value will cause NPE if the type is NULL. 3. Changed getReturnType of IfExpression to consider the nullable types of THEN and ELSE expressions. 4. Added testcases.
Project: http://git-wip-us.apache.org/repos/asf/incubator-drill/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-drill/commit/c8a08c3e Tree: http://git-wip-us.apache.org/repos/asf/incubator-drill/tree/c8a08c3e Diff: http://git-wip-us.apache.org/repos/asf/incubator-drill/diff/c8a08c3e Branch: refs/heads/master Commit: c8a08c3e793d53ae4c445f07caa639269c8b51b7 Parents: 2cdbd60 Author: vkorukanti <[email protected]> Authored: Tue May 27 18:49:50 2014 -0700 Committer: Jacques Nadeau <[email protected]> Committed: Wed May 28 15:13:06 2014 -0700 ---------------------------------------------------------------------- .../drill/common/expression/IfExpression.java | 21 ++++- .../templates/ConvertToNullableHolder.java | 80 +++++++++++++++++++ .../exec/expr/ExpressionTreeMaterializer.java | 36 +++++++++ .../drill/exec/planner/logical/DrillOptiq.java | 82 +++++++++++++++----- .../apache/drill/jdbc/test/TestJdbcQuery.java | 48 ++++++++++++ 5 files changed, 245 insertions(+), 22 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/c8a08c3e/common/src/main/java/org/apache/drill/common/expression/IfExpression.java ---------------------------------------------------------------------- diff --git a/common/src/main/java/org/apache/drill/common/expression/IfExpression.java b/common/src/main/java/org/apache/drill/common/expression/IfExpression.java index 280952d..d1df7f7 100644 --- a/common/src/main/java/org/apache/drill/common/expression/IfExpression.java +++ b/common/src/main/java/org/apache/drill/common/expression/IfExpression.java @@ -23,7 +23,11 @@ import java.util.List; import org.apache.drill.common.expression.IfExpression.IfCondition; import org.apache.drill.common.expression.visitors.ExprVisitor; +import org.apache.drill.common.types.TypeProtos; +import org.apache.drill.common.types.TypeProtos.DataMode; import org.apache.drill.common.types.TypeProtos.MajorType; +import org.apache.drill.common.types.TypeProtos.MinorType; +import org.apache.drill.common.types.Types; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -102,7 +106,22 @@ public class IfExpression extends LogicalExpressionBase{ @Override public MajorType getMajorType() { - return this.elseExpression.getMajorType(); + // If the return type of one of the "then" expression or "else" expression is nullable, return "if" expression + // type as nullable + MajorType majorType = elseExpression.getMajorType(); + if (majorType.getMode() == DataMode.OPTIONAL) { + return majorType; + } + + for(IfCondition condition : conditions) { + if (condition.expression.getMajorType().getMode() == DataMode.OPTIONAL) { + assert condition.expression.getMajorType().getMinorType() == majorType.getMinorType(); + + return condition.expression.getMajorType(); + } + } + + return majorType; } public static Builder newBuilder(){ http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/c8a08c3e/exec/java-exec/src/main/codegen/templates/ConvertToNullableHolder.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/codegen/templates/ConvertToNullableHolder.java b/exec/java-exec/src/main/codegen/templates/ConvertToNullableHolder.java new file mode 100644 index 0000000..548d645 --- /dev/null +++ b/exec/java-exec/src/main/codegen/templates/ConvertToNullableHolder.java @@ -0,0 +1,80 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +<@pp.dropOutputFile /> +<#list vv.types as type> +<#list type.minor as minor> + +<#assign className="GConvertToNullable${minor.class}Holder" /> + +<@pp.changeOutputFile name="/org/apache/drill/exec/expr/fn/impl/${className}.java" /> + +<#include "/@includes/license.ftl" /> + +package org.apache.drill.exec.expr.fn.impl; + +import org.apache.drill.exec.expr.DrillSimpleFunc; +import org.apache.drill.exec.expr.annotations.*; +import org.apache.drill.exec.expr.holders.*; +import org.apache.drill.exec.record.RecordBatch; + +@FunctionTemplate(name = "convertToNullable${minor.class?upper_case}", scope = FunctionTemplate.FunctionScope.SIMPLE, nulls = FunctionTemplate.NullHandling.INTERNAL) +public class ${className} implements DrillSimpleFunc { + + @Param ${minor.class}Holder input; + @Output Nullable${minor.class}Holder output; + + public void setup(RecordBatch incoming) { } + + public void eval() { + output.isSet = 1; +<#if type.major != "VarLen"> + <#if (minor.class == "TimeStampTZ")> + output.value = input.value; + output.index = input.index; + <#elseif (minor.class == "Interval")> + output.months = input.months; + output.days = input.days; + output.milliSeconds = input.milliSeconds; + <#elseif (minor.class == "IntervalDay")> + output.days = input.days; + output.milliSeconds = input.milliSeconds; + <#elseif minor.class.startsWith("Decimal")> + output.scale = input.scale; + output.precision = input.precision; + <#if minor.class.startsWith("Decimal28") || minor.class.startsWith("Decimal38")> + output.sign = input.sign; + output.start = input.start; + output.buffer = input.buffer; + <#else> + output.value = input.value; + </#if> + <#elseif (type.width > 8)> + output.start = input.start; + output.buffer = input.buffer; + <#else> + output.value = input.value; + </#if> +<#else> + output.start = input.start; + output.end = input.end; + output.buffer = input.buffer; +</#if> + } +} +</#list> +</#list> \ No newline at end of file http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/c8a08c3e/exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java index 50ddb75..fca743b 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java @@ -284,9 +284,45 @@ public class ExpressionTreeMaterializer { } } + // If the type of the IF expression is nullable, apply a convertToNullable*Holder function for "THEN"/"ELSE" + // expressions whose type is not nullable. + if (IfExpression.newBuilder().setElse(newElseExpr).addConditions(conditions).build().getMajorType().getMode() + == DataMode.OPTIONAL) { + for (int i = 0; i < conditions.size(); ++i) { + IfExpression.IfCondition condition = conditions.get(i); + if (condition.expression.getMajorType().getMode() != DataMode.OPTIONAL) { + conditions.set(i, new IfExpression.IfCondition(condition.condition, + getConvertToNullableExpr(ImmutableList.of(condition.expression), + condition.expression.getMajorType().getMinorType(), registry))); + } + } + + if (newElseExpr.getMajorType().getMode() != DataMode.OPTIONAL) { + newElseExpr = getConvertToNullableExpr(ImmutableList.of(newElseExpr), + newElseExpr.getMajorType().getMinorType(), registry); + } + } + return validateNewExpr(IfExpression.newBuilder().setElse(newElseExpr).addConditions(conditions).build()); } + private LogicalExpression getConvertToNullableExpr(List<LogicalExpression> args, MinorType minorType, + FunctionImplementationRegistry registry) { + String funcName = "convertToNullable" + minorType.toString(); + FunctionCall funcCall = new FunctionCall(funcName, args, ExpressionPosition.UNKNOWN); + FunctionResolver resolver = FunctionResolverFactory.getResolver(funcCall); + + DrillFuncHolder matchedConvertToNullableFuncHolder = + resolver.getBestMatch(registry.getDrillRegistry().getMethods().get(funcName), funcCall); + + if (matchedConvertToNullableFuncHolder == null) { + logFunctionResolutionError(errorCollector, funcCall); + return NullExpression.INSTANCE; + } + + return new DrillFuncHolderExpr(funcName, matchedConvertToNullableFuncHolder, args, ExpressionPosition.UNKNOWN); + } + private LogicalExpression rewriteNullExpression(LogicalExpression expr, MajorType type) { if(expr instanceof NullExpression) { return new TypedNullConstant(type); http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/c8a08c3e/exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java index a77dd6f..8966f18 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java @@ -338,21 +338,40 @@ public class DrillOptiq { public LogicalExpression visitLiteral(RexLiteral literal) { switch(literal.getType().getSqlTypeName()){ case BIGINT: + if (isLiteralNull(literal)) { + return createNullExpr(MinorType.BIGINT); + } long l = ((BigDecimal) literal.getValue()).longValue(); - return checkNullLiteral(literal, MinorType.BIGINT, ValueExpressions.getBigInt(l)); + return ValueExpressions.getBigInt(l); case BOOLEAN: - return checkNullLiteral(literal, MinorType.BIT, ValueExpressions.getBit(((Boolean) literal.getValue()))); + if (isLiteralNull(literal)) { + return createNullExpr(MinorType.BIT); + } + return ValueExpressions.getBit(((Boolean) literal.getValue())); case CHAR: - return checkNullLiteral(literal, MinorType.VARCHAR, ValueExpressions.getChar(((NlsString) literal.getValue()).getValue())); + if (isLiteralNull(literal)) { + return createNullExpr(MinorType.VARCHAR); + } + return ValueExpressions.getChar(((NlsString)literal.getValue()).getValue()); case DOUBLE: + if (isLiteralNull(literal)){ + return createNullExpr(MinorType.FLOAT8); + } double d = ((BigDecimal) literal.getValue()).doubleValue(); - return checkNullLiteral(literal, MinorType.FLOAT8, ValueExpressions.getFloat8(d)); + return ValueExpressions.getFloat8(d); case FLOAT: + if (isLiteralNull(literal)) { + return createNullExpr(MinorType.FLOAT4); + } float f = ((BigDecimal) literal.getValue()).floatValue(); - return checkNullLiteral(literal, MinorType.FLOAT4, ValueExpressions.getFloat4(f)); + return ValueExpressions.getFloat4(f); case INTEGER: + if (isLiteralNull(literal)) { + return createNullExpr(MinorType.INT); + } int a = ((BigDecimal) literal.getValue()).intValue(); - return checkNullLiteral(literal, MinorType.INT, ValueExpressions.getInt(a)); + return ValueExpressions.getInt(a); + case DECIMAL: /* TODO: Enable using Decimal literals once we have more functions implemented for Decimal * For now continue using Double instead of decimals @@ -367,24 +386,49 @@ public class DrillOptiq { } else if (precision <= 38) { return ValueExpressions.getDecimal38((BigDecimal)literal.getValue()); } */ - + if (isLiteralNull(literal)) { + return createNullExpr(MinorType.FLOAT8); + } double dbl = ((BigDecimal) literal.getValue()).doubleValue(); logger.warn("Converting exact decimal into approximate decimal. Should be fixed once decimal is implemented."); - return checkNullLiteral(literal, MinorType.FLOAT8, ValueExpressions.getFloat8(dbl)); + return ValueExpressions.getFloat8(dbl); case VARCHAR: - return checkNullLiteral(literal, MinorType.VARCHAR, ValueExpressions.getChar(((NlsString) literal.getValue()).getValue())); + if (isLiteralNull(literal)) { + return createNullExpr(MinorType.VARCHAR); + } + return ValueExpressions.getChar(((NlsString)literal.getValue()).getValue()); case SYMBOL: - return checkNullLiteral(literal, MinorType.VARCHAR, ValueExpressions.getChar(literal.getValue().toString())); + if (isLiteralNull(literal)) { + return createNullExpr(MinorType.VARCHAR); + } + return ValueExpressions.getChar(literal.getValue().toString()); case DATE: - return checkNullLiteral(literal, MinorType.DATE, ValueExpressions.getDate((GregorianCalendar) literal.getValue())); + if (isLiteralNull(literal)) { + return createNullExpr(MinorType.DATE); + } + return (ValueExpressions.getDate((GregorianCalendar)literal.getValue())); case TIME: - return checkNullLiteral(literal, MinorType.TIME, ValueExpressions.getTime((GregorianCalendar) literal.getValue())); + if (isLiteralNull(literal)) { + return createNullExpr(MinorType.TIME); + } + return (ValueExpressions.getTime((GregorianCalendar)literal.getValue())); case TIMESTAMP: - return checkNullLiteral(literal, MinorType.TIMESTAMP, ValueExpressions.getTimeStamp((GregorianCalendar) literal.getValue())); + if (isLiteralNull(literal)) { + return createNullExpr(MinorType.TIMESTAMP); + } + return (ValueExpressions.getTimeStamp((GregorianCalendar) literal.getValue())); case INTERVAL_YEAR_MONTH: - return checkNullLiteral(literal, MinorType.INTERVALYEAR, ValueExpressions.getIntervalYear(((BigDecimal) (literal.getValue())).intValue())); + if (isLiteralNull(literal)) { + return createNullExpr(MinorType.INTERVALYEAR); + } + return (ValueExpressions.getIntervalYear(((BigDecimal) (literal.getValue())).intValue())); case INTERVAL_DAY_TIME: - return checkNullLiteral(literal, MinorType.INTERVALDAY, ValueExpressions.getIntervalDay(((BigDecimal) (literal.getValue())).longValue())); + if (isLiteralNull(literal)) { + return createNullExpr(MinorType.INTERVALDAY); + } + return (ValueExpressions.getIntervalDay(((BigDecimal) (literal.getValue())).longValue())); + case NULL: + return NullExpression.INSTANCE; case ANY: if (isLiteralNull(literal)) { return NullExpression.INSTANCE; @@ -395,12 +439,8 @@ public class DrillOptiq { } } - private static LogicalExpression checkNullLiteral(RexLiteral literal, MinorType type, LogicalExpression orExpr) { - if(isLiteralNull(literal)) { - return new TypedNullConstant(Types.optional(type)); - } else { - return orExpr; - } + private static final TypedNullConstant createNullExpr(MinorType type) { + return new TypedNullConstant(Types.optional(type)); } private static boolean isLiteralNull(RexLiteral literal) { http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/c8a08c3e/exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestJdbcQuery.java ---------------------------------------------------------------------- diff --git a/exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestJdbcQuery.java b/exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestJdbcQuery.java index 8783897..e684228 100644 --- a/exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestJdbcQuery.java +++ b/exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestJdbcQuery.java @@ -947,4 +947,52 @@ import static org.junit.Assert.fail; } }); } + + @Test + public void testCaseWithNoElse() throws Exception { + JdbcAssert.withNoDefaultSchema() + .sql("SELECT employee_id, CASE WHEN employee_id < 100 THEN first_name END from cp.`employee.json` " + + "WHERE employee_id = 99 OR employee_id = 100") + .returns( + "employee_id=99; EXPR$1=Elizabeth\n" + + "employee_id=100; EXPR$1=null\n" + ); + } + + @Test + public void testCaseWithElse() throws Exception { + JdbcAssert.withNoDefaultSchema() + .sql("SELECT employee_id, CASE WHEN employee_id < 100 THEN first_name ELSE 'Test' END from cp.`employee.json` " + + "WHERE employee_id = 99 OR employee_id = 100") + .returns( + "employee_id=99; EXPR$1=Elizabeth\n" + + "employee_id=100; EXPR$1=Test" + ); + } + + @Test + public void testCaseWith2ThensAndNoElse() throws Exception { + JdbcAssert.withNoDefaultSchema() + .sql("SELECT employee_id, CASE WHEN employee_id < 100 THEN first_name WHEN employee_id = 100 THEN last_name END " + + "from cp.`employee.json` " + + "WHERE employee_id = 99 OR employee_id = 100 OR employee_id = 101") + .returns( + "employee_id=99; EXPR$1=Elizabeth\n" + + "employee_id=100; EXPR$1=Hunt\n" + + "employee_id=101; EXPR$1=null" + ); + } + + @Test + public void testCaseWith2ThensAndElse() throws Exception { + JdbcAssert.withNoDefaultSchema() + .sql("SELECT employee_id, CASE WHEN employee_id < 100 THEN first_name WHEN employee_id = 100 THEN last_name ELSE 'Test' END " + + "from cp.`employee.json` " + + "WHERE employee_id = 99 OR employee_id = 100 OR employee_id = 101") + .returns( + "employee_id=99; EXPR$1=Elizabeth\n" + + "employee_id=100; EXPR$1=Hunt\n" + + "employee_id=101; EXPR$1=Test\n" + ); + } }
