Repository: incubator-drill Updated Branches: refs/heads/master e2ad5b038 -> d5b9b11c9
DRILL-1663: Fix casting to a variable width type implicitly in a join condition Factor out logic in ExpressionTreeMaterializer so the same logic can be reused in ChainedHashTable. Project: http://git-wip-us.apache.org/repos/asf/incubator-drill/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-drill/commit/3430f811 Tree: http://git-wip-us.apache.org/repos/asf/incubator-drill/tree/3430f811 Diff: http://git-wip-us.apache.org/repos/asf/incubator-drill/diff/3430f811 Branch: refs/heads/master Commit: 3430f8112376932c96b4023c666eff2604f00223 Parents: e2ad5b0 Author: Mehant Baid <meha...@gmail.com> Authored: Fri Nov 7 13:55:48 2014 -0800 Committer: Mehant Baid <meha...@gmail.com> Committed: Mon Nov 10 14:01:40 2014 -0800 ---------------------------------------------------------------------- .../exec/expr/ExpressionTreeMaterializer.java | 146 +++++++++++-------- .../physical/impl/common/ChainedHashTable.java | 25 ++-- 2 files changed, 102 insertions(+), 69 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/3430f811/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 2a5afce..9cec3a4 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 @@ -101,6 +101,61 @@ public class ExpressionTreeMaterializer { } } + public static LogicalExpression addCastExpression(LogicalExpression fromExpr, MajorType toType, FunctionImplementationRegistry registry, ErrorCollector errorCollector) { + 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); + } + + private static void logFunctionResolutionError(ErrorCollector errorCollector, FunctionCall call) { + // add error to collector + StringBuilder sb = new StringBuilder(); + sb.append("Missing function implementation: "); + sb.append("["); + sb.append(call.getName()); + sb.append("("); + boolean first = true; + for(LogicalExpression e : call.args) { + TypeProtos.MajorType mt = e.getMajorType(); + if (first) { + first = false; + } else { + sb.append(", "); + } + sb.append(mt.getMinorType().name()); + sb.append("-"); + sb.append(mt.getMode().name()); + } + sb.append(")"); + sb.append("]"); + + errorCollector.addGeneralError(call.getPosition(), sb.toString()); + } + private static class MaterializeVisitor extends AbstractExprVisitor<LogicalExpression, FunctionImplementationRegistry, RuntimeException> { private ExpressionValidator validator = new ExpressionValidator(); private final ErrorCollector errorCollector; @@ -143,38 +198,7 @@ public class ExpressionTreeMaterializer { 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 + @Override public LogicalExpression visitFunctionCall(FunctionCall call, FunctionImplementationRegistry registry) { List<LogicalExpression> args = Lists.newArrayList(); for (int i = 0; i < call.args.size(); ++i) { @@ -217,7 +241,7 @@ public class ExpressionTreeMaterializer { argsWithCast.add(currentArg); } else { //Case 3: insert cast if param type is different from arg type. - argsWithCast.add(addCastExpression(call.args.get(i), parmType, registry)); + argsWithCast.add(addCastExpression(call.args.get(i), parmType, registry, errorCollector)); } } @@ -238,7 +262,7 @@ public class ExpressionTreeMaterializer { extArgsWithCast.add(currentArg); } else { // Insert cast if param type is different from arg type. - extArgsWithCast.add(addCastExpression(call.args.get(i), parmType, registry)); + extArgsWithCast.add(addCastExpression(call.args.get(i), parmType, registry, errorCollector)); } } @@ -249,31 +273,37 @@ public class ExpressionTreeMaterializer { return NullExpression.INSTANCE; } - private void logFunctionResolutionError(ErrorCollector errorCollector, FunctionCall call) { - // add error to collector - StringBuilder sb = new StringBuilder(); - sb.append("Missing function implementation: "); - sb.append("["); - sb.append(call.getName()); - sb.append("("); - boolean first = true; - for(LogicalExpression e : call.args) { - TypeProtos.MajorType mt = e.getMajorType(); - if (first) { - first = false; - } else { - sb.append(", "); - } - sb.append(mt.getMinorType().name()); - sb.append("-"); - sb.append(mt.getMode().name()); + public static LogicalExpression addCastExpression(LogicalExpression fromExpr, MajorType toType, FunctionImplementationRegistry registry, ErrorCollector errorCollector) { + 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)); } - sb.append(")"); - sb.append("]"); - errorCollector.addGeneralError(call.getPosition(), sb.toString()); - } + 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 visitIfExpression(IfExpression ifExpr, FunctionImplementationRegistry registry) { @@ -294,10 +324,10 @@ public class ExpressionTreeMaterializer { if (leastRestrictive != thenType) { // Implicitly cast the then expression conditions = new IfExpression.IfCondition(newCondition, - addCastExpression(conditions.expression, newElseExpr.getMajorType(), registry)); + addCastExpression(conditions.expression, newElseExpr.getMajorType(), registry, errorCollector)); } else if (leastRestrictive != elseType) { // Implicitly cast the else expression - newElseExpr = addCastExpression(newElseExpr, conditions.expression.getMajorType(), registry); + newElseExpr = addCastExpression(newElseExpr, conditions.expression.getMajorType(), registry, errorCollector); } else { /* Cannot cast one of the two expressions to make the output type of if and else expression * to be the same. Raise error. http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/3430f811/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/ChainedHashTable.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/ChainedHashTable.java b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/ChainedHashTable.java index 792b7ab..4b80781 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/ChainedHashTable.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/ChainedHashTable.java @@ -27,6 +27,7 @@ import org.apache.drill.common.expression.LogicalExpression; import org.apache.drill.common.expression.FunctionCall; import org.apache.drill.common.expression.ExpressionPosition; import org.apache.drill.common.exceptions.DrillRuntimeException; +import org.apache.drill.common.expression.ValueExpressions; import org.apache.drill.common.logical.data.NamedExpression; import org.apache.drill.common.types.TypeProtos; import org.apache.drill.common.types.TypeProtos.MinorType; @@ -287,8 +288,10 @@ public class ChainedHashTable { assert keyExprsBuild.length == keyExprsProbe.length; for (int i = 0; i < keyExprsBuild.length; i++) { - MinorType buildType = keyExprsBuild[i].getMajorType().getMinorType(); - MinorType probeType = keyExprsProbe[i].getMajorType().getMinorType(); + LogicalExpression buildExpr = keyExprsBuild[i]; + LogicalExpression probeExpr = keyExprsProbe[i]; + MinorType buildType = buildExpr.getMajorType().getMinorType(); + MinorType probeType = probeExpr.getMajorType().getMinorType(); if (buildType != probeType) { // We need to add a cast to one of the expressions @@ -296,22 +299,22 @@ public class ChainedHashTable { types.add(buildType); types.add(probeType); MinorType result = TypeCastRules.getLeastRestrictiveType(types); + ErrorCollector errorCollector = new ErrorCollectorImpl(); - // Add the cast - List<LogicalExpression> args = new LinkedList<>(); if (result == null) { throw new DrillRuntimeException(String.format("Join conditions cannot be compared failing build expression: %s failing probe expression: %s", - keyExprsBuild[i].getMajorType().toString(), keyExprsProbe[i].getMajorType().toString())); + buildExpr.getMajorType().toString(), probeExpr.getMajorType().toString())); } else if (result != buildType) { // Add a cast expression on top of the build expression - args.add(keyExprsBuild[i]); - FunctionCall castCall = new FunctionCall("cast" + result.toString().toUpperCase(), args, ExpressionPosition.UNKNOWN); - keyExprsBuild[i] = ExpressionTreeMaterializer.materialize(castCall, incomingBuild, new ErrorCollectorImpl(), context.getFunctionRegistry()); + LogicalExpression castExpr = ExpressionTreeMaterializer.addCastExpression(buildExpr, probeExpr.getMajorType(), context.getFunctionRegistry(), errorCollector); + // Store the newly casted expression + keyExprsBuild[i] = ExpressionTreeMaterializer.materialize(castExpr, incomingBuild, errorCollector, context.getFunctionRegistry()); } else if (result != probeType) { - args.add(keyExprsProbe[i]); - FunctionCall castCall = new FunctionCall("cast" + result.toString().toUpperCase(), args, ExpressionPosition.UNKNOWN); - keyExprsProbe[i] = ExpressionTreeMaterializer.materialize(castCall, incomingProbe, new ErrorCollectorImpl(), context.getFunctionRegistry()); + // Add a cast expression on top of the probe expression + LogicalExpression castExpr = ExpressionTreeMaterializer.addCastExpression(probeExpr, buildExpr.getMajorType(), context.getFunctionRegistry(), errorCollector); + // store the newly casted expression + keyExprsProbe[i] = ExpressionTreeMaterializer.materialize(castExpr, incomingProbe, errorCollector, context.getFunctionRegistry()); } } }
