---------- Forwarded message ---------- From: Timothy Chen <[email protected]> Date: Wed, May 28, 2014 at 3:19 PM Subject: Re: [4/4] git commit: DRILL-665: Handle null values in case expressions (contd). To: [email protected]
Nice to see the added test cases, I didn't get to do those yet. Thanks for splitting the patch for me! Tim On Wed, May 28, 2014 at 3:13 PM, <[email protected]> wrote: > 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" > + ); > + } > } >
