> On Feb. 28, 2014, 2:33 a.m., Jinfeng Ni wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/EvaluationVisitor.java, > > line 330 > > <https://reviews.apache.org/r/18372/diff/2/?file=504198#file504198line330> > > > > ConstantFilter should not see FunctionCall, since it has been converted > > in materialization step. Here, better to raise exception.
done > On Feb. 28, 2014, 2:33 a.m., Jinfeng Ni wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java, > > line 133 > > <https://reviews.apache.org/r/18372/diff/2/?file=504199#file504199line133> > > > > The error message for failure of cast function resolve is not same as > > the regular functions (line 149 - 168). > > > > It might be better to wrap Line 149-168 in a method, and call this > > method for both cast function and regular function, so that we have similar > > style of error message. done > On Feb. 28, 2014, 2:33 a.m., Jinfeng Ni wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFuncHolder.java, > > line 101 > > <https://reviews.apache.org/r/18372/diff/2/?file=504203#file504203line101> > > > > this is probably a minor issue. If the function has a long list of > > registerNames, then we will output every one in the list? Probably printing the first name should be sufficient. Changed accordingly. > On Feb. 28, 2014, 2:33 a.m., Jinfeng Ni wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/hive/DrillBigIntObjectInspector.java, > > line 1 > > <https://reviews.apache.org/r/18372/diff/2/?file=504222#file504222line1> > > > > Probably it's better to use freemarker to generate the ObjectInspector > > class, since they look similar for int, bigint, float4, etc.... > > I am going to log a separate JIRA to: 1. generate ObjectInspectors using freemarker 2. Remove ObjectInspectorHelper and generate the code using codemodel as part of function implementation code generation. > On Feb. 28, 2014, 2:33 a.m., Jinfeng Ni wrote: > > common/src/main/java/org/apache/drill/common/expression/ExpressionValidator.java, > > line 40 > > <https://reviews.apache.org/r/18372/diff/2/?file=504169#file504169line40> > > > > For FunctionHolderExpression, we probably should check if an aggregate > > function is nested within another aggregate function. Similar logic as > > AggregateChecker. > > > > i.e sum(col1 + avg(col2)) should be blocked. > > Added code to: 1. validate nested aggregate functions within aggregate function using AggregateChecker. 2. validate argument expressions for function arguments that expect constant expressions using ConstantChecker. Modified AggregateChecker and ConstantChecker to handle FunctionHolderExpression > On Feb. 28, 2014, 2:33 a.m., Jinfeng Ni wrote: > > common/src/main/java/org/apache/drill/common/expression/visitors/ConstantChecker.java, > > line 16 > > <https://reviews.apache.org/r/18372/diff/2/?file=504187#file504187line16> > > > > Looks like ConstantChecker is only used by ref. Probably we could > > delete it? (ConstantExpressionIdentifier, which will be used in > > EvalVisitor, would still be needed). ConstantChecker was used in BasicArgumentValidator as part of ExpressionValidator. As BasicArgumentValidator is removed, we lost the check where we verified the function arguments are constants when FunctionDefinition expects constant arguments. So we need ConstantChecker and used as part of ExpressionValidator. - Venki ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18372/#review35741 ----------------------------------------------------------- On Feb. 26, 2014, 2:20 a.m., Venki Korukanti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18372/ > ----------------------------------------------------------- > > (Updated Feb. 26, 2014, 2:20 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 > 11d97e8 > 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 > 8009632 > > 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 > 8ffc07a > > 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 > 6a98f94 > > common/src/main/java/org/apache/drill/common/expression/fn/MathFunctions.java > ee3a099 > > 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 > 711bdb8 > > common/src/main/java/org/apache/drill/common/expression/visitors/AggregateChecker.java > 128f284 > > common/src/main/java/org/apache/drill/common/expression/visitors/ConstantChecker.java > 02ce231 > > common/src/main/java/org/apache/drill/common/expression/visitors/ExprVisitor.java > db34ab7 > > common/src/main/java/org/apache/drill/common/expression/visitors/SimpleExprVisitor.java > 65a939b > > common/src/test/java/org/apache/drill/common/expression/parser/TreeTest.java > 6833b69 > contrib/pom.xml dc07979 > contrib/storage-hive/pom.xml PRE-CREATION > contrib/storage-hive/src/main/resources/drill-module.conf PRE-CREATION > exec/java-exec/pom.xml 1c4dc32 > exec/java-exec/src/main/codegen/templates/ComparisonFunctions.java ba9da07 > > exec/java-exec/src/main/java/org/apache/drill/exec/compile/sig/ConstantExpressionIdentifier.java > 46dbbe9 > > 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 > a7895d3 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java > 36433ad > > 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/fn/DrillAggFuncHolder.java > 7622865 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFuncHolder.java > dcdf1ea > > 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 > 4939063 > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/FunctionConverter.java > befa9bf > > 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/impl/Alternator.java > 5bff29e > > 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 > f991a41 > > 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 > 76683f3 > > 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 > a042a5a > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/MathFunctions.java > ea251c3 > > 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/hive/AbstractPrimitiveObjectInspector.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/hive/DrillBigIntObjectInspector.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/hive/DrillBitObjectInspector.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/expr/fn/impl/hive/DrillFloat4ObjectInspector.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/hive/DrillFloat8ObjectInspector.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/hive/DrillIntObjectInspector.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/hive/DrillSmallIntObjectInspector.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/hive/DrillTinyIntObjectInspector.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/hive/DrillVar16CharObjectInspector.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/hive/DrillVarCharObjectInspector.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/hive/ObjectInspectorHelper.java > PRE-CREATION > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/AggBatch.java > 86fea4e > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatch.java > 298b031 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java > 43d3b45 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java > da8978f > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/sort/SortBatch.java > 4d04735 > 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 > b79ccd0 > > 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/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 > > exec/ref/src/main/java/org/apache/drill/exec/ref/eval/SimpleEvaluationVisitor.java > 1b04880 > > exec/ref/src/main/java/org/apache/drill/exec/ref/eval/fn/FunctionArguments.java > 47c0405 > > exec/ref/src/test/java/org/apache/drill/exec/ref/rops/CollapsingAggregateTest.java > d63c0cc > exec/ref/src/test/java/org/apache/drill/exec/ref/rops/OrderROPTest.java > d4ea473 > sqlparser/src/test/java/org/apache/drill/jdbc/test/JdbcTest.java 9ae9682 > > 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 > >
