DRILL-1180: Add casts to case expression to ensure all branches have same output type
Project: http://git-wip-us.apache.org/repos/asf/incubator-drill/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-drill/commit/846e291d Tree: http://git-wip-us.apache.org/repos/asf/incubator-drill/tree/846e291d Diff: http://git-wip-us.apache.org/repos/asf/incubator-drill/diff/846e291d Branch: refs/heads/master Commit: 846e291d5f6966a9fdf2b590c597b79b3205bb68 Parents: b6410ff Author: Mehant Baid <[email protected]> Authored: Fri Jul 25 19:39:30 2014 -0700 Committer: Mehant Baid <[email protected]> Committed: Sat Jul 26 02:15:15 2014 -0700 ---------------------------------------------------------------------- .../drill/common/expression/parser/ExprParser.g | 8 +- .../expression/ExpressionStringBuilder.java | 19 ++- .../drill/common/expression/IfExpression.java | 44 ++----- .../expression/visitors/AggregateChecker.java | 5 +- .../visitors/ConditionalExprOptimizer.java | 14 +- .../expression/visitors/ConstantChecker.java | 8 +- .../visitors/ExpressionValidator.java | 53 ++++---- .../drill/exec/expr/EvaluationVisitor.java | 41 +++--- .../exec/expr/ExpressionTreeMaterializer.java | 130 ++++++++++--------- .../drill/exec/planner/logical/DrillOptiq.java | 2 +- .../drill/exec/resolver/TypeCastRules.java | 45 ++++++- .../record/ExpressionTreeMaterializerTest.java | 35 +++-- 12 files changed, 208 insertions(+), 196 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/846e291d/common/src/main/antlr3/org/apache/drill/common/expression/parser/ExprParser.g ---------------------------------------------------------------------- diff --git a/common/src/main/antlr3/org/apache/drill/common/expression/parser/ExprParser.g b/common/src/main/antlr3/org/apache/drill/common/expression/parser/ExprParser.g index 5afaae4..dafb1d6 100644 --- a/common/src/main/antlr3/org/apache/drill/common/expression/parser/ExprParser.g +++ b/common/src/main/antlr3/org/apache/drill/common/expression/parser/ExprParser.g @@ -150,7 +150,7 @@ ifStatement returns [LogicalExpression e] @after { $e = s.build(); } - : i1=ifStat {s.addCondition($i1.i); s.setPosition(pos($i1.start)); } (elseIfStat { s.addCondition($elseIfStat.i); } )* Else expression { s.setElse($expression.e); }End + : i1=ifStat {s.setIfCondition($i1.i); s.setPosition(pos($i1.start)); } (elseIfStat { s.setIfCondition($elseIfStat.i); } )* Else expression { s.setElse($expression.e); }End ; ifStat returns [IfExpression.IfCondition i] @@ -167,11 +167,11 @@ caseStatement returns [LogicalExpression e] @after { $e = s.build(); } - : Case (caseWhenStat {s.addCondition($caseWhenStat.i); }) + caseElseStat { s.setElse($caseElseStat.e); } End + : Case (caseWhenStat {s.setIfCondition($caseWhenStat.e); }) + caseElseStat { s.setElse($caseElseStat.e); } End ; -caseWhenStat returns [IfExpression.IfCondition i] - : When e1=expression Then e2=expression {$i = new IfExpression.IfCondition($e1.e, $e2.e); } +caseWhenStat returns [IfExpression.IfCondition e] + : When e1=expression Then e2=expression {$e = new IfExpression.IfCondition($e1.e, $e2.e); } ; caseElseStat returns [LogicalExpression e] http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/846e291d/common/src/main/java/org/apache/drill/common/expression/ExpressionStringBuilder.java ---------------------------------------------------------------------- diff --git a/common/src/main/java/org/apache/drill/common/expression/ExpressionStringBuilder.java b/common/src/main/java/org/apache/drill/common/expression/ExpressionStringBuilder.java index edc1a53..0e778c5 100644 --- a/common/src/main/java/org/apache/drill/common/expression/ExpressionStringBuilder.java +++ b/common/src/main/java/org/apache/drill/common/expression/ExpressionStringBuilder.java @@ -89,17 +89,16 @@ public class ExpressionStringBuilder extends AbstractExprVisitor<Void, StringBui @Override public Void visitIfExpression(IfExpression ifExpr, StringBuilder sb) throws RuntimeException { - ImmutableList<IfCondition> conditions = ifExpr.conditions; + + // serialize the if expression sb.append(" ( "); - for(int i =0; i < conditions.size(); i++){ - IfCondition c = conditions.get(i); - if(i !=0) sb.append(" else "); - sb.append("if ("); - c.condition.accept(this, sb); - sb.append(" ) then ("); - c.expression.accept(this, sb); - sb.append(" ) "); - } + IfCondition c = ifExpr.ifCondition; + sb.append("if ("); + c.condition.accept(this, sb); + sb.append(" ) then ("); + c.expression.accept(this, sb); + sb.append(" ) "); + sb.append(" else ("); ifExpr.elseExpression.accept(this, sb); sb.append(" ) "); http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/846e291d/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 8dc220f..f16d8fe 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 @@ -41,12 +41,12 @@ import com.google.common.collect.UnmodifiableIterator; public class IfExpression extends LogicalExpressionBase{ static final Logger logger = LoggerFactory.getLogger(IfExpression.class); - public final ImmutableList<IfCondition> conditions; + public final IfCondition ifCondition; public final LogicalExpression elseExpression; - private IfExpression(ExpressionPosition pos, List<IfCondition> conditions, LogicalExpression elseExpression){ + private IfExpression(ExpressionPosition pos, IfCondition conditions, LogicalExpression elseExpression){ super(pos); - this.conditions = ImmutableList.copyOf(conditions); + this.ifCondition = conditions; this.elseExpression = elseExpression; } @@ -69,7 +69,7 @@ public class IfExpression extends LogicalExpressionBase{ } public static class Builder{ - List<IfCondition> conditions = new ArrayList<IfCondition>(); + IfCondition conditions; private LogicalExpression elseExpression; private ExpressionPosition pos = ExpressionPosition.UNKNOWN; @@ -78,27 +78,19 @@ public class IfExpression extends LogicalExpressionBase{ return this; } - public Builder addCondition(IfCondition condition){ - conditions.add(condition); + public Builder setElse(LogicalExpression elseExpression) { + this.elseExpression = elseExpression; return this; } - public Builder addConditions(Iterable<IfCondition> conditions) { - for (IfCondition condition : conditions) { - addCondition(condition); - } + public Builder setIfCondition(IfCondition conditions) { + this.conditions = conditions; return this; } - public Builder setElse(LogicalExpression elseExpression) { - this.elseExpression = elseExpression; - return this; - } - public IfExpression build(){ Preconditions.checkNotNull(pos); Preconditions.checkNotNull(conditions); - Preconditions.checkNotNull(conditions); return new IfExpression(pos, conditions, elseExpression); } @@ -113,12 +105,10 @@ public class IfExpression extends LogicalExpressionBase{ return majorType; } - for(IfCondition condition : conditions) { - if (condition.expression.getMajorType().getMode() == DataMode.OPTIONAL) { - assert condition.expression.getMajorType().getMinorType() == majorType.getMinorType(); + if (ifCondition.expression.getMajorType().getMode() == DataMode.OPTIONAL) { + assert ifCondition.expression.getMajorType().getMinorType() == majorType.getMinorType(); - return condition.expression.getMajorType(); - } + return ifCondition.expression.getMajorType(); } return majorType; @@ -128,20 +118,12 @@ public class IfExpression extends LogicalExpressionBase{ return new Builder(); } - - public Iterable<IfCondition> conditionIterable(){ - - return ImmutableList.copyOf(conditions); - } - @Override public Iterator<LogicalExpression> iterator() { List<LogicalExpression> children = Lists.newLinkedList(); - for(IfCondition ic : conditions){ - children.add(ic.condition); - children.add(ic.expression); - } + children.add(ifCondition.condition); + children.add(ifCondition.expression); children.add(this.elseExpression); return children.iterator(); } http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/846e291d/common/src/main/java/org/apache/drill/common/expression/visitors/AggregateChecker.java ---------------------------------------------------------------------- diff --git a/common/src/main/java/org/apache/drill/common/expression/visitors/AggregateChecker.java b/common/src/main/java/org/apache/drill/common/expression/visitors/AggregateChecker.java index 81457b5..4c75459 100644 --- a/common/src/main/java/org/apache/drill/common/expression/visitors/AggregateChecker.java +++ b/common/src/main/java/org/apache/drill/common/expression/visitors/AggregateChecker.java @@ -91,9 +91,8 @@ public final class AggregateChecker implements ExprVisitor<Boolean, ErrorCollect @Override public Boolean visitIfExpression(IfExpression ifExpr, ErrorCollector errors) { - for(IfCondition c : ifExpr.conditions){ - if(c.condition.accept(this, errors) || c.expression.accept(this, errors)) return true; - } + IfCondition c = ifExpr.ifCondition; + if(c.condition.accept(this, errors) || c.expression.accept(this, errors)) return true; return ifExpr.elseExpression.accept(this, errors); } http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/846e291d/common/src/main/java/org/apache/drill/common/expression/visitors/ConditionalExprOptimizer.java ---------------------------------------------------------------------- diff --git a/common/src/main/java/org/apache/drill/common/expression/visitors/ConditionalExprOptimizer.java b/common/src/main/java/org/apache/drill/common/expression/visitors/ConditionalExprOptimizer.java index cdf8729..1a4b399 100644 --- a/common/src/main/java/org/apache/drill/common/expression/visitors/ConditionalExprOptimizer.java +++ b/common/src/main/java/org/apache/drill/common/expression/visitors/ConditionalExprOptimizer.java @@ -81,18 +81,14 @@ public class ConditionalExprOptimizer extends AbstractExprVisitor<LogicalExpress @Override public LogicalExpression visitIfExpression(IfExpression ifExpr, Void value) throws RuntimeException{ - List<IfExpression.IfCondition> conditions = Lists.newArrayList(ifExpr.conditions); LogicalExpression newElseExpr = ifExpr.elseExpression.accept(this, value); + IfCondition conditions = ifExpr.ifCondition; - for (int i = 0; i < conditions.size(); ++i) { - IfExpression.IfCondition condition = conditions.get(i); + LogicalExpression newCondition = conditions.condition.accept(this, value); + LogicalExpression newExpr = conditions.expression.accept(this, value); + conditions = new IfExpression.IfCondition(newCondition, newExpr); - LogicalExpression newCondition = condition.condition.accept(this, value); - LogicalExpression newExpr = condition.expression.accept(this, value); - conditions.set(i, new IfExpression.IfCondition(newCondition, newExpr)); - } - - return IfExpression.newBuilder().setElse(newElseExpr).addConditions(conditions).build(); + return IfExpression.newBuilder().setElse(newElseExpr).setIfCondition(conditions).build(); } @Override http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/846e291d/common/src/main/java/org/apache/drill/common/expression/visitors/ConstantChecker.java ---------------------------------------------------------------------- diff --git a/common/src/main/java/org/apache/drill/common/expression/visitors/ConstantChecker.java b/common/src/main/java/org/apache/drill/common/expression/visitors/ConstantChecker.java index c73102a..a2a0d7f 100644 --- a/common/src/main/java/org/apache/drill/common/expression/visitors/ConstantChecker.java +++ b/common/src/main/java/org/apache/drill/common/expression/visitors/ConstantChecker.java @@ -90,10 +90,10 @@ final class ConstantChecker implements ExprVisitor<Boolean, ErrorCollector, Runt @Override public Boolean visitIfExpression(IfExpression ifExpr, ErrorCollector errors) { - for (IfCondition c : ifExpr.conditions) { - if (!c.condition.accept(this, errors) || !c.expression.accept(this, errors)) - return false; - } + IfCondition c = ifExpr.ifCondition; + if (!c.condition.accept(this, errors) || !c.expression.accept(this, errors)) + return false; + if (!ifExpr.elseExpression.accept(this, errors)) return false; return true; } http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/846e291d/common/src/main/java/org/apache/drill/common/expression/visitors/ExpressionValidator.java ---------------------------------------------------------------------- diff --git a/common/src/main/java/org/apache/drill/common/expression/visitors/ExpressionValidator.java b/common/src/main/java/org/apache/drill/common/expression/visitors/ExpressionValidator.java index 57f93d3..cfcf371 100644 --- a/common/src/main/java/org/apache/drill/common/expression/visitors/ExpressionValidator.java +++ b/common/src/main/java/org/apache/drill/common/expression/visitors/ExpressionValidator.java @@ -95,39 +95,34 @@ public class ExpressionValidator implements ExprVisitor<Void, ErrorCollector, Ru @Override public Void visitIfExpression(IfExpression ifExpr, ErrorCollector errors) throws RuntimeException { // confirm that all conditions are required boolean values. - int i = 0; - for (IfCondition c : ifExpr.conditions) { - MajorType mt = c.condition.getMajorType(); - if ( mt.getMinorType() != MinorType.BIT) { - errors - .addGeneralError( - c.condition.getPosition(), - String - .format( - "Failure composing If Expression. All conditions must return a boolean type. Condition %d was of Type %s.", - i, mt.getMinorType())); - } - i++; + IfCondition cond = ifExpr.ifCondition; + MajorType majorType = cond.condition.getMajorType(); + if ( majorType + .getMinorType() != MinorType.BIT) { + errors + .addGeneralError( + cond.condition.getPosition(), + String + .format( + "Failure composing If Expression. All conditions must return a boolean type. Condition was of Type %s.", + majorType.getMinorType())); } // confirm that all outcomes are the same type. final MajorType mt = ifExpr.elseExpression.getMajorType(); - i = 0; - for (IfCondition c : ifExpr.conditions) { - MajorType innerT = c.expression.getMajorType(); - if ((innerT.getMode() == DataMode.REPEATED && mt.getMode() != DataMode.REPEATED) || // - ((innerT.getMinorType() != mt.getMinorType()) && - (innerT.getMode() != DataMode.OPTIONAL && mt.getMode() != DataMode.OPTIONAL && - (innerT.getMinorType() != MinorType.NULL && mt.getMinorType() != MinorType.NULL)))) { - errors - .addGeneralError( - c.condition.getPosition(), - String - .format( - "Failure composing If Expression. All expressions must return the same MinorType as the else expression. The %d if condition returned type type %s but the else expression was of type %s", - i, innerT, mt)); - } - i++; + cond = ifExpr.ifCondition; + MajorType innerT = cond.expression.getMajorType(); + if ((innerT.getMode() == DataMode.REPEATED && mt.getMode() != DataMode.REPEATED) || // + ((innerT.getMinorType() != mt.getMinorType()) && + (innerT.getMode() != DataMode.OPTIONAL && mt.getMode() != DataMode.OPTIONAL && + (innerT.getMinorType() != MinorType.NULL && mt.getMinorType() != MinorType.NULL)))) { + errors + .addGeneralError( + cond.condition.getPosition(), + String + .format( + "Failure composing If Expression. All expressions must return the same MinorType as the else expression. The if condition returned type type %s but the else expression was of type %s", + innerT, mt)); } return null; } http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/846e291d/exec/java-exec/src/main/java/org/apache/drill/exec/expr/EvaluationVisitor.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/EvaluationVisitor.java b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/EvaluationVisitor.java index 8bfa149..73ce45e 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/expr/EvaluationVisitor.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/expr/EvaluationVisitor.java @@ -136,32 +136,31 @@ public class EvaluationVisitor { JConditional jc = null; JBlock conditionalBlock = new JBlock(false, false); - for (IfCondition c : ifExpr.conditions) { - HoldingContainer holdingContainer = c.condition.accept(this, generator); - if (jc == null) { - if (holdingContainer.isOptional()) { - jc = conditionalBlock._if(holdingContainer.getIsSet().eq(JExpr.lit(1)).cand(holdingContainer.getValue().eq(JExpr.lit(1)))); - } else { - jc = conditionalBlock._if(holdingContainer.getValue().eq(JExpr.lit(1))); - } + IfCondition c = ifExpr.ifCondition; + + HoldingContainer holdingContainer = c.condition.accept(this, generator); + if (jc == null) { + if (holdingContainer.isOptional()) { + jc = conditionalBlock._if(holdingContainer.getIsSet().eq(JExpr.lit(1)).cand(holdingContainer.getValue().eq(JExpr.lit(1)))); } else { - if (holdingContainer.isOptional()) { - jc = jc._else()._if(holdingContainer.getIsSet().eq(JExpr.lit(1)).cand(holdingContainer.getValue().eq(JExpr.lit(1)))); - } else { - jc = jc._else()._if(holdingContainer.getValue().eq(JExpr.lit(1))); - } + jc = conditionalBlock._if(holdingContainer.getValue().eq(JExpr.lit(1))); } - - HoldingContainer thenExpr = c.expression.accept(this, generator); - if (thenExpr.isOptional()) { - JConditional newCond = jc._then()._if(thenExpr.getIsSet().ne(JExpr.lit(0))); - JBlock b = newCond._then(); - b.assign(output.getHolder(), thenExpr.getHolder()); - //b.assign(output.getIsSet(), thenExpr.getIsSet()); + } else { + if (holdingContainer.isOptional()) { + jc = jc._else()._if(holdingContainer.getIsSet().eq(JExpr.lit(1)).cand(holdingContainer.getValue().eq(JExpr.lit(1)))); } else { - jc._then().assign(output.getHolder(), thenExpr.getHolder()); + jc = jc._else()._if(holdingContainer.getValue().eq(JExpr.lit(1))); } + } + HoldingContainer thenExpr = c.expression.accept(this, generator); + if (thenExpr.isOptional()) { + JConditional newCond = jc._then()._if(thenExpr.getIsSet().ne(JExpr.lit(0))); + JBlock b = newCond._then(); + b.assign(output.getHolder(), thenExpr.getHolder()); + //b.assign(output.getIsSet(), thenExpr.getIsSet()); + } else { + jc._then().assign(output.getHolder(), thenExpr.getHolder()); } HoldingContainer elseExpr = ifExpr.elseExpression.accept(this, generator); http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/846e291d/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 18cd894..60bc78c 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 @@ -17,6 +17,9 @@ */ package org.apache.drill.exec.expr; +import java.lang.reflect.Array; +import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import com.google.common.base.Function; @@ -70,10 +73,12 @@ import org.apache.drill.exec.expr.fn.DrillFuncHolder; import org.apache.drill.exec.expr.fn.FunctionImplementationRegistry; import org.apache.drill.exec.record.TypedFieldId; import org.apache.drill.exec.record.VectorAccessible; +import org.apache.drill.exec.resolver.DefaultFunctionResolver; import org.apache.drill.exec.resolver.FunctionResolver; import org.apache.drill.exec.resolver.FunctionResolverFactory; import com.google.common.collect.Lists; +import org.apache.drill.exec.resolver.TypeCastRules; public class ExpressionTreeMaterializer { @@ -142,7 +147,38 @@ public class ExpressionTreeMaterializer { //replace with a new function call, since its argument could be changed. return new BooleanOperator(op.getName(), args, op.getPosition()); } - + + private LogicalExpression addCastExpression(LogicalExpression fromExpr, MajorType toType, FunctionImplementationRegistry registry) { + String castFuncName = CastFunctions.getCastFunc(toType.getMinorType()); + List<LogicalExpression> castArgs = Lists.newArrayList(); + castArgs.add(fromExpr); //input_expr + + if (!Types.isFixedWidthType(toType)) { + + /* We are implicitly casting to VARCHAR so we don't have a max length, + * using an arbitrary value. We trim down the size of the stored bytes + * to the actual size so this size doesn't really matter. + */ + castArgs.add(new ValueExpressions.LongExpression(65536, null)); + } + else if (toType.getMinorType().name().startsWith("DECIMAL")) { + // Add the scale and precision to the arguments of the implicit cast + castArgs.add(new ValueExpressions.LongExpression(fromExpr.getMajorType().getPrecision(), null)); + castArgs.add(new ValueExpressions.LongExpression(fromExpr.getMajorType().getScale(), null)); + } + + FunctionCall castCall = new FunctionCall(castFuncName, castArgs, ExpressionPosition.UNKNOWN); + FunctionResolver resolver = FunctionResolverFactory.getResolver(castCall); + DrillFuncHolder matchedCastFuncHolder = registry.findDrillFunction(resolver, castCall); + + if (matchedCastFuncHolder == null) { + logFunctionResolutionError(errorCollector, castCall); + return NullExpression.INSTANCE; + } + + return matchedCastFuncHolder.getExpr(castFuncName, castArgs, ExpressionPosition.UNKNOWN); + + } @Override public LogicalExpression visitFunctionCall(FunctionCall call, FunctionImplementationRegistry registry) { List<LogicalExpression> args = Lists.newArrayList(); @@ -186,34 +222,7 @@ public class ExpressionTreeMaterializer { argsWithCast.add(currentArg); } else { //Case 3: insert cast if param type is different from arg type. - String castFuncName = CastFunctions.getCastFunc(parmType.getMinorType()); - List<LogicalExpression> castArgs = Lists.newArrayList(); - castArgs.add(call.args.get(i)); //input_expr - - if (!Types.isFixedWidthType(parmType)) { - - /* We are implicitly casting to VARCHAR so we don't have a max length, - * using an arbitrary value. We trim down the size of the stored bytes - * to the actual size so this size doesn't really matter. - */ - castArgs.add(new ValueExpressions.LongExpression(65536, null)); - } - else if (parmType.getMinorType().name().startsWith("DECIMAL")) { - // Add the scale and precision to the arguments of the implicit cast - castArgs.add(new ValueExpressions.LongExpression(currentArg.getMajorType().getPrecision(), null)); - castArgs.add(new ValueExpressions.LongExpression(currentArg.getMajorType().getScale(), null)); - } - - FunctionCall castCall = new FunctionCall(castFuncName, castArgs, ExpressionPosition.UNKNOWN); - DrillFuncHolder matchedCastFuncHolder = registry.findDrillFunction(resolver, castCall); - - if (matchedCastFuncHolder == null) { - logFunctionResolutionError(errorCollector, castCall); - return NullExpression.INSTANCE; - } - - argsWithCast.add(matchedCastFuncHolder.getExpr(call.getName(), castArgs, ExpressionPosition.UNKNOWN)); - + argsWithCast.add(addCastExpression(call.args.get(i), parmType, registry)); } } @@ -255,31 +264,40 @@ public class ExpressionTreeMaterializer { errorCollector.addGeneralError(call.getPosition(), sb.toString()); } + @Override public LogicalExpression visitIfExpression(IfExpression ifExpr, FunctionImplementationRegistry registry) { - List<IfExpression.IfCondition> conditions = Lists.newArrayList(ifExpr.conditions); + IfExpression.IfCondition conditions = ifExpr.ifCondition; LogicalExpression newElseExpr = ifExpr.elseExpression.accept(this, registry); - for (int i = 0; i < conditions.size(); ++i) { - IfExpression.IfCondition condition = conditions.get(i); + LogicalExpression newCondition = conditions.condition.accept(this, registry); + LogicalExpression newExpr = conditions.expression.accept(this, registry); + conditions = new IfExpression.IfCondition(newCondition, newExpr); - LogicalExpression newCondition = condition.condition.accept(this, registry); - LogicalExpression newExpr = condition.expression.accept(this, registry); - conditions.set(i, new IfExpression.IfCondition(newCondition, newExpr)); + MinorType thenType = conditions.expression.getMajorType().getMinorType(); + MinorType elseType = newElseExpr.getMajorType().getMinorType(); + + // Check if we need a cast + if (thenType != elseType && !(thenType == MinorType.NULL || elseType == MinorType.NULL)) { + + MinorType leastRestrictive = TypeCastRules.getLeastRestrictiveType((Arrays.asList(thenType, elseType))); + if (leastRestrictive != thenType) { + // Implicitly cast the then expression + conditions = new IfExpression.IfCondition(newCondition, + addCastExpression(conditions.expression, newElseExpr.getMajorType(), registry)); + } else if (leastRestrictive != elseType) { + // Implicitly cast the else expression + newElseExpr = addCastExpression(newElseExpr, conditions.expression.getMajorType(), registry); + } else { + assert false: "Incorrect least restrictive type computed, leastRestrictive: " + + leastRestrictive.toString() + " thenType: " + thenType.toString() + " elseType: " + elseType; + } } // Resolve NullExpression into TypedNullConstant by visiting all conditions // We need to do this because we want to give the correct MajorType to the Null constant - Iterable<LogicalExpression> logicalExprs = Iterables.transform(conditions, - new Function<IfCondition, LogicalExpression>() { - @Override - public LogicalExpression apply(IfExpression.IfCondition input) { - return input.expression; - } - } - ); - - List<LogicalExpression> allExpressions = Lists.newArrayList(logicalExprs); + List<LogicalExpression> allExpressions = Lists.newArrayList(); + allExpressions.add(conditions.expression); allExpressions.add(newElseExpr); boolean containsNullExpr = Iterables.any(allExpressions, new Predicate<LogicalExpression>() { @@ -301,12 +319,7 @@ public class ExpressionTreeMaterializer { if(nonNullExpr.isPresent()) { MajorType type = nonNullExpr.get().getMajorType(); - for (int i = 0; i < conditions.size(); ++i) { - IfExpression.IfCondition condition = conditions.get(i); - conditions.set(i, - new IfExpression.IfCondition(condition.condition, rewriteNullExpression(condition.expression, type)) - ); - } + conditions = new IfExpression.IfCondition(conditions.condition, rewriteNullExpression(conditions.expression, type)); newElseExpr = rewriteNullExpression(newElseExpr, type); } @@ -314,16 +327,13 @@ 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() + if (IfExpression.newBuilder().setElse(newElseExpr).setIfCondition(conditions).build().getMajorType().getMode() == DataMode.OPTIONAL) { - for (int i = 0; i < conditions.size(); ++i) { - IfExpression.IfCondition condition = conditions.get(i); + IfExpression.IfCondition condition = conditions; 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))); - } - } + conditions = 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), @@ -331,7 +341,7 @@ public class ExpressionTreeMaterializer { } } - return validateNewExpr(IfExpression.newBuilder().setElse(newElseExpr).addConditions(conditions).build()); + return validateNewExpr(IfExpression.newBuilder().setElse(newElseExpr).setIfCondition(conditions).build()); } private LogicalExpression getConvertToNullableExpr(List<LogicalExpression> args, MinorType minorType, http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/846e291d/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 75a5ebc..633084f 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 @@ -157,7 +157,7 @@ public class DrillOptiq { for (int i=1; i<caseArgs.size(); i=i+2) { elseExpression = IfExpression.newBuilder() .setElse(elseExpression) - .addCondition(new IfCondition(caseArgs.get(i + 1), caseArgs.get(i))).build(); + .setIfCondition(new IfCondition(caseArgs.get(i + 1), caseArgs.get(i))).build(); } return elseExpression; } http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/846e291d/exec/java-exec/src/main/java/org/apache/drill/exec/resolver/TypeCastRules.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/resolver/TypeCastRules.java b/exec/java-exec/src/main/java/org/apache/drill/exec/resolver/TypeCastRules.java index bf202c8..8176567 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/resolver/TypeCastRules.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/resolver/TypeCastRules.java @@ -20,9 +20,11 @@ package org.apache.drill.exec.resolver; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; +import org.apache.drill.common.exceptions.DrillRuntimeException; import org.apache.drill.common.expression.FunctionCall; import org.apache.drill.common.types.Types; import org.apache.drill.common.types.TypeProtos.DataMode; @@ -768,11 +770,46 @@ public class TypeCastRules { rules.put(MinorType.VARBINARY, rule); } - public static boolean isCastable(MajorType from, MajorType to, NullHandling nullHandling) { + public static boolean isCastableWithNullHandling(MajorType from, MajorType to, NullHandling nullHandling) { if (nullHandling == NullHandling.INTERNAL && from.getMode() != to.getMode()) return false; + return isCastable(from.getMinorType(), to.getMinorType()); + } + + private static boolean isCastable(MinorType from, MinorType to) { + return from.equals(MinorType.NULL) || //null could be casted to any other type. + (rules.get(to) == null ? false : rules.get(to).contains(from)); + } + + /* + * Function checks if casting is allowed from the 'from' -> 'to' minor type. If its allowed + * we also check if the precedence map allows such a cast and return true if both cases are satisfied + */ + public static MinorType getLeastRestrictiveType(List<MinorType> types) { + assert types.size() >= 2; + MinorType result = types.get(0); + int resultPrec = ResolverTypePrecedence.precedenceMap.get(result); + + for (int i = 1; i < types.size(); i++) { + MinorType next = types.get(i); + if (next == result) { + // both args are of the same type; continue + continue; + } + + int nextPrec = ResolverTypePrecedence.precedenceMap.get(next); + + if (isCastable(next, result) && resultPrec >= nextPrec) { + // result is the least restrictive between the two args; nothing to do continue + continue; + } else if(isCastable(result, next) && nextPrec >= resultPrec) { + result = next; + resultPrec = nextPrec; + } else { + throw new DrillRuntimeException("Case expression branches have different output types "); + } + } - return from.getMinorType().equals(MinorType.NULL) || //null could be casted to any other type. - (rules.get(to.getMinorType()) == null ? false : rules.get(to.getMinorType()).contains(from.getMinorType())); + return result; } private static final int DATAMODE_CAST_COST = 1; @@ -817,7 +854,7 @@ public class TypeCastRules { // return -1; } - if (!TypeCastRules.isCastable(argType, parmType, holder.getNullHandling())) { + if (!TypeCastRules.isCastableWithNullHandling(argType, parmType, holder.getNullHandling())) { return -1; } http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/846e291d/exec/java-exec/src/test/java/org/apache/drill/exec/record/ExpressionTreeMaterializerTest.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/record/ExpressionTreeMaterializerTest.java b/exec/java-exec/src/test/java/org/apache/drill/exec/record/ExpressionTreeMaterializerTest.java index d07ce85..c294aee 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/record/ExpressionTreeMaterializerTest.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/record/ExpressionTreeMaterializerTest.java @@ -106,31 +106,26 @@ public class ExpressionTreeMaterializerTest extends ExecTest { ErrorCollector ec = new ErrorCollectorImpl(); + + LogicalExpression elseExpression = new IfExpression.Builder().setElse(new ValueExpressions.LongExpression(1L, ExpressionPosition.UNKNOWN)) + .setIfCondition(new IfExpression.IfCondition(new ValueExpressions.BooleanExpression("true", ExpressionPosition.UNKNOWN), + new FieldReference("test1", ExpressionPosition.UNKNOWN))) + .build(); + LogicalExpression expr = new IfExpression.Builder() - .addCondition( - new IfExpression.IfCondition( // - new FieldReference("test", ExpressionPosition.UNKNOWN), // - new IfExpression.Builder() - // - .addCondition( // - new IfExpression.IfCondition( - // - new ValueExpressions.BooleanExpression("true", ExpressionPosition.UNKNOWN), - new FieldReference("test1", ExpressionPosition.UNKNOWN))) - .setElse(new ValueExpressions.LongExpression(1L, ExpressionPosition.UNKNOWN)).build()) // - ) // - .setElse(new ValueExpressions.LongExpression(1L, ExpressionPosition.UNKNOWN)).build(); + .setIfCondition(new IfExpression.IfCondition(new FieldReference("test", ExpressionPosition.UNKNOWN), new ValueExpressions.LongExpression(2L, ExpressionPosition.UNKNOWN))) + .setElse(elseExpression).build(); + LogicalExpression newExpr = ExpressionTreeMaterializer.materialize(expr, batch, ec, registry); assertTrue(newExpr instanceof IfExpression); IfExpression newIfExpr = (IfExpression) newExpr; - assertEquals(1, newIfExpr.conditions.size()); - IfExpression.IfCondition ifCondition = newIfExpr.conditions.get(0); - assertTrue(ifCondition.expression instanceof IfExpression); - newIfExpr = (IfExpression) ifCondition.expression; - assertEquals(1, newIfExpr.conditions.size()); - ifCondition = newIfExpr.conditions.get(0); + //assertEquals(1, newIfExpr.conditions.size()); + IfExpression.IfCondition ifCondition = newIfExpr.ifCondition; + assertTrue(newIfExpr.elseExpression instanceof IfExpression); + //assertEquals(1, newIfExpr.conditions.size()); + //ifCondition = newIfExpr.conditions.get(0); assertEquals(bigIntType, ifCondition.expression.getMajorType()); - assertEquals(true, ((ValueExpressions.BooleanExpression) ifCondition.condition).value); + assertEquals(true, ((ValueExpressions.BooleanExpression) ((IfExpression)(newIfExpr.elseExpression)).ifCondition.condition).value); if (ec.hasErrors()) System.out.println(ec.toErrorString()); assertFalse(ec.hasErrors());
