Update AggregateChecker to add errors to ErrorCollector instead of throwing Exceptions 1. Tests pending
Project: http://git-wip-us.apache.org/repos/asf/incubator-drill/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-drill/commit/4e817d11 Tree: http://git-wip-us.apache.org/repos/asf/incubator-drill/tree/4e817d11 Diff: http://git-wip-us.apache.org/repos/asf/incubator-drill/diff/4e817d11 Branch: refs/heads/master Commit: 4e817d115a0c86e6491b453d962be16aa7f7985e Parents: 356c2db Author: vkorukanti <[email protected]> Authored: Fri Mar 28 06:18:13 2014 -0700 Committer: Jacques Nadeau <[email protected]> Committed: Sat Mar 29 14:43:48 2014 -0700 ---------------------------------------------------------------------- .../expression/visitors/AggregateChecker.java | 59 ++++++++++---------- .../visitors/ExpressionValidator.java | 2 +- 2 files changed, 30 insertions(+), 31 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/4e817d11/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 630d00a..100bf94 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 @@ -18,6 +18,7 @@ package org.apache.drill.common.expression.visitors; import org.apache.drill.common.expression.CastExpression; +import org.apache.drill.common.expression.ErrorCollector; import org.apache.drill.common.expression.FunctionCall; import org.apache.drill.common.expression.FunctionHolderExpression; import org.apache.drill.common.expression.IfExpression; @@ -31,92 +32,90 @@ import org.apache.drill.common.expression.ValueExpressions.FloatExpression; import org.apache.drill.common.expression.ValueExpressions.IntExpression; import org.apache.drill.common.expression.ValueExpressions.QuotedString; -public final class AggregateChecker extends SimpleExprVisitor<Boolean>{ - +public final class AggregateChecker implements ExprVisitor<Boolean, ErrorCollector, RuntimeException>{ + public static final AggregateChecker INSTANCE = new AggregateChecker(); - - private AggregateChecker(){}; - - public static boolean isAggregating(LogicalExpression e){ - return e.accept(INSTANCE, null); + + public static boolean isAggregating(LogicalExpression e, ErrorCollector errors){ + return e.accept(INSTANCE, errors); } @Override - public Boolean visitFunctionCall(FunctionCall call) { + public Boolean visitFunctionCall(FunctionCall call, ErrorCollector errors) { throw new UnsupportedOperationException("FunctionCall is not expected here. "+ "It should have been converted to FunctionHolderExpression in materialization"); } @Override - public Boolean visitFunctionHolderExpression(FunctionHolderExpression holder) throws RuntimeException { + public Boolean visitFunctionHolderExpression(FunctionHolderExpression holder, ErrorCollector errors) { if(holder.isAggregating()){ - for(LogicalExpression e : holder.args){ - if(e.accept(this, null)) - throw new IllegalArgumentException(String.format( - "Your aggregating function call %s also includes arguments that contain aggregations."+ - " This isn't allowed.", holder.getName())); + for (int i = 0; i < holder.args.size(); i++) { + LogicalExpression e = holder.args.get(i); + if(e.accept(this, errors)){ + errors.addGeneralError(e.getPosition(), + String.format("Aggregating function call %s includes nested aggregations at arguments number %d. " + + "This isn't allowed.", holder.getName(), i)); + } } return true; }else{ for(LogicalExpression e : holder.args){ - if(e.accept(this, null)) return true; + if(e.accept(this, errors)) return true; } return false; } } @Override - public Boolean visitIfExpression(IfExpression ifExpr) { + public Boolean visitIfExpression(IfExpression ifExpr, ErrorCollector errors) { for(IfCondition c : ifExpr.conditions){ - if(c.condition.accept(this, null) || c.expression.accept(this, null)) return true; + if(c.condition.accept(this, errors) || c.expression.accept(this, errors)) return true; } - return ifExpr.elseExpression.accept(this, null); + return ifExpr.elseExpression.accept(this, errors); } @Override - public Boolean visitSchemaPath(SchemaPath path) { + public Boolean visitSchemaPath(SchemaPath path, ErrorCollector errors) { return false; } @Override - public Boolean visitIntConstant(IntExpression intExpr) { + public Boolean visitIntConstant(IntExpression intExpr, ErrorCollector errors) { return false; } @Override - public Boolean visitFloatConstant(FloatExpression fExpr) { + public Boolean visitFloatConstant(FloatExpression fExpr, ErrorCollector errors) { return false; } @Override - public Boolean visitLongConstant(LongExpression intExpr) { + public Boolean visitLongConstant(LongExpression intExpr, ErrorCollector errors) { return false; } @Override - public Boolean visitDoubleConstant(DoubleExpression dExpr) { + public Boolean visitDoubleConstant(DoubleExpression dExpr, ErrorCollector errors) { return false; } @Override - public Boolean visitBooleanConstant(BooleanExpression e) { + public Boolean visitBooleanConstant(BooleanExpression e, ErrorCollector errors) { return false; } @Override - public Boolean visitQuotedStringConstant(QuotedString e) { + public Boolean visitQuotedStringConstant(QuotedString e, ErrorCollector errors) { return false; } @Override - public Boolean visitUnknown(LogicalExpression e, Void value) throws RuntimeException { + public Boolean visitUnknown(LogicalExpression e, ErrorCollector errors) { return false; } @Override - public Boolean visitCastExpression(CastExpression e, Void value) throws RuntimeException { - return e.getInput().accept(this, value); + public Boolean visitCastExpression(CastExpression e, ErrorCollector errors) { + return e.getInput().accept(this, errors); } - - } http://git-wip-us.apache.org/repos/asf/incubator-drill/blob/4e817d11/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 96666bd..cf110cd 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 @@ -52,7 +52,7 @@ public class ExpressionValidator implements ExprVisitor<Void, ErrorCollector, Ru public Void visitFunctionHolderExpression(FunctionHolderExpression holder, ErrorCollector errors) throws RuntimeException { // make sure aggregate functions are not nested inside aggregate functions - AggregateChecker.isAggregating(holder); + AggregateChecker.isAggregating(holder, errors); // make sure arguments are constant if the function implementation expects constants for any arguments ConstantChecker.checkConstants(holder, errors);
