----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18372/#review38239 -----------------------------------------------------------
common/src/main/java/org/apache/drill/common/expression/ExpressionValidator.java <https://reviews.apache.org/r/18372/#comment70295> On line 44, for AggregateChecker and ConstantChecker, when there is nested aggregated function call, you will throw IllegalArgumentException. This is different from the logic in method visitIfExpression, where error message is added into a ErrorCollector. Do you think we had better to make the behavior consistent in ExpressionValidator? If yes, we need pass in ErrorCollector to AggregateChecker. Similar for ConstantChecker on line 47. common/src/main/java/org/apache/drill/common/expression/visitors/ConstantChecker.java <https://reviews.apache.org/r/18372/#comment70297> I feel the logic here is not right. You only check whether the i-th argument is constant when argConstantOnly(i) is true. If not, then you did not check the argument at all, but you will still return "true". For example, 1 + col seems to return true from this method. - Jinfeng Ni On March 20, 2014, 10:04 a.m., Venki Korukanti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18372/ > ----------------------------------------------------------- > > (Updated March 20, 2014, 10:04 a.m.) > > > Review request for drill, Jacques Nadeau and Jinfeng Ni. > > > Repository: drill-git > > > Description > ------- > > Currently works with simple UDFs that implement GenericUDF and UDF > interfaces. Not all column types are supported currently. Supported types are > BIT, all INTs, FLOAT4/8, VARCHAR/VAR16CHAR. This patch also removes the > FunctionDefnitions (and related code) and uses FunctionHolder for function > resolution. > > There are few tests failing. Currently debugging those. Will update the patch > once the failures are resolved. > > github branch: https://github.com/vkorukanti/incubator-drill/commits/func-rb > > > Diffs > ----- > > > common/src/main/antlr3/org/apache/drill/common/expression/parser/ExprParser.g > b5cf292 > common/src/main/java/org/apache/drill/common/expression/Arg.java 0dd47ff > > common/src/main/java/org/apache/drill/common/expression/ArgumentValidator.java > 18456b5 > > common/src/main/java/org/apache/drill/common/expression/ArgumentValidators.java > da46645 > > common/src/main/java/org/apache/drill/common/expression/BasicArgumentValidator.java > f6c94d9 > common/src/main/java/org/apache/drill/common/expression/CallProvider.java > 2339261 > > common/src/main/java/org/apache/drill/common/expression/ExpressionStringBuilder.java > 69dd9b3 > > common/src/main/java/org/apache/drill/common/expression/ExpressionValidator.java > 076c150 > common/src/main/java/org/apache/drill/common/expression/FunctionCall.java > e3ed4c5 > > common/src/main/java/org/apache/drill/common/expression/FunctionCallFactory.java > PRE-CREATION > > common/src/main/java/org/apache/drill/common/expression/FunctionDefinition.java > f9e408b > > common/src/main/java/org/apache/drill/common/expression/FunctionHolderExpression.java > PRE-CREATION > > common/src/main/java/org/apache/drill/common/expression/FunctionInstance.java > cda49e9 > > common/src/main/java/org/apache/drill/common/expression/FunctionRegistry.java > 7cb386a > > common/src/main/java/org/apache/drill/common/expression/LogicalExpression.java > 70c2617 > common/src/main/java/org/apache/drill/common/expression/NoArgValidator.java > eaabfeb > > common/src/main/java/org/apache/drill/common/expression/fn/AggregationFunctions.java > 5b46b78 > > common/src/main/java/org/apache/drill/common/expression/fn/BooleanFunctions.java > f21b750 > > common/src/main/java/org/apache/drill/common/expression/fn/CastFunctionDefs.java > 4be3820 > > common/src/main/java/org/apache/drill/common/expression/fn/CastFunctions.java > PRE-CREATION > > common/src/main/java/org/apache/drill/common/expression/fn/MathFunctions.java > e1d3390 > > common/src/main/java/org/apache/drill/common/expression/fn/StringFunctions.java > 1158115 > > common/src/main/java/org/apache/drill/common/expression/fn/TypeFunctions.java > 23ca87c > > common/src/main/java/org/apache/drill/common/expression/fn/UnaryFunctions.java > 0a678fd > > common/src/main/java/org/apache/drill/common/expression/visitors/AbstractExprVisitor.java > 469cedd > > common/src/main/java/org/apache/drill/common/expression/visitors/AggregateChecker.java > 36e6a52 > > common/src/main/java/org/apache/drill/common/expression/visitors/ConstantChecker.java > a951a60 > > common/src/main/java/org/apache/drill/common/expression/visitors/ExprVisitor.java > 87d146a > > common/src/main/java/org/apache/drill/common/expression/visitors/SimpleExprVisitor.java > 71266f7 > > common/src/test/java/org/apache/drill/common/expression/parser/TreeTest.java > 6833b69 > contrib/storage-hive/src/main/resources/drill-module.conf PRE-CREATION > exec/java-exec/src/main/codegen/config.fmpp fcf1f4d > exec/java-exec/src/main/codegen/data/HiveTypes.tdd PRE-CREATION > exec/java-exec/src/main/codegen/templates/CastFunctions.java af32a27 > exec/java-exec/src/main/codegen/templates/CastFunctionsSrcVarLen.java > 5855381 > > exec/java-exec/src/main/codegen/templates/CastFunctionsSrcVarLenTargetVarLen.java > 8594d5a > exec/java-exec/src/main/codegen/templates/CastFunctionsTargetVarLen.java > c864e72 > exec/java-exec/src/main/codegen/templates/ComparisonFunctions.java ba9da07 > exec/java-exec/src/main/codegen/templates/MathFunctionTemplates.java > 9ec2178 > exec/java-exec/src/main/codegen/templates/MathFunctions.java c683261 > exec/java-exec/src/main/codegen/templates/ObjectInspectorHelper.java > PRE-CREATION > exec/java-exec/src/main/codegen/templates/ObjectInspectors.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/compile/sig/ConstantExpressionIdentifier.java > 83a5e6f > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/DrillFuncHolderExpr.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/EvaluationVisitor.java > cac47b3 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java > c0f34f4 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/HiveFuncHolderExpr.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/ImplicitCastBuilder.java > 99c26f3 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/annotations/FunctionTemplate.java > 2208e53 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillAggFuncHolder.java > 5f1e358 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFuncHolder.java > d1619b2 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFunctionImplementationRegistry.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillSimpleFuncHolder.java > de847df > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionConverter.java > 3145240 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionImplementationRegistry.java > 93f1992 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/HiveFuncHolder.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/HiveFunctionImplementationRegistry.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/agg/impl/CountFunctions.java > b0939f1 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/agg/impl/SumFunctions.java > 57905fe > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/Alternator.java > f65af3b > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/BitFunctions.java > 41bb7c6 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/ByteSubstring.java > 964e4af > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/CharSubstring.java > 0b43028 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/ComparatorFunctions.java > cd0eb21 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/ComparisonFunctionsNullable.java > 9be620a > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/HashFunctions.java > 3cc82b4 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/IsNotNull.java > 5ee5ccb > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/IsNull.java > a1d3368 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/MathFunctions.java > 3fcf5c9 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/SimpleRepeatedFunctions.java > 2f838b1 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java > 9212867 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/hive/AbstractPrimitiveObjectInspector.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/hive/DrillDeferredObject.java > PRE-CREATION > exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java > 36cfb53 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/AggBatch.java > ad60e8f > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatch.java > 6e98543 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java > 8572085 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java > 3413046 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortBatch.java > 3364a45 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillAggregateRel.java > b4c1bf9 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java > 13f5494 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillParseContext.java > b82fef5 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java > 0fdff3c > > exec/java-exec/src/main/java/org/apache/drill/exec/server/DrillbitContext.java > 0e6b7bf > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java > baecc3f > exec/java-exec/src/test/java/org/apache/drill/exec/expr/ExpressionTest.java > 0de64b4 > > exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/GeneratorFunctions.java > 7ec7243 > > exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestHiveUDFs.java > PRE-CREATION > > exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestSimpleFunctions.java > 5bfe369 > > exec/java-exec/src/test/java/org/apache/drill/exec/record/ExpressionTreeMaterializerTest.java > b3b7536 > exec/java-exec/src/test/resources/functions/hive/GenericUDF.json > PRE-CREATION > exec/java-exec/src/test/resources/functions/hive/UDF.json PRE-CREATION > > Diff: https://reviews.apache.org/r/18372/diff/ > > > Testing > ------- > > GenericUDF.json and UDF.json contain the testcases for both types of UDFs. > > > Thanks, > > Venki Korukanti > >
