abstractdog commented on code in PR #6366:
URL: https://github.com/apache/hive/pull/6366#discussion_r3079404756
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java:
##########
@@ -1717,6 +1717,10 @@ private boolean
checkExprNodeDescForDecimal64(ExprNodeDesc exprNodeDesc) throws
GenericUDF udf = ((ExprNodeGenericFuncDesc)
exprNodeDesc).getGenericUDF();
Class<?> udfClass = udf.getClass();
if (udf instanceof GenericUDFToDecimal) {
+ ExprNodeDesc child = exprNodeDesc.getChildren().get(0);
+ if (isDecimalFamily(child.getTypeString())) {
Review Comment:
I wish I had a better understanding here, I believe
`checkExprNodeDescForDecimal64` method should have a good javadoc here, maybe
the one I already found in this class should be improved and moved to this
place:
```
/*
* For columns, we check decimal columns for DECIMAL_64
DataTypePhysicalVariation.
* For UDFs, we check for @VectorizedExpressionsSupportDecimal64
annotation, etc.
*/
final boolean isExprDecimal64 =
checkExprNodeDescForDecimal64(childExpr);
```
Am I right to assume the following:
1. `checkExprNodeDescForDecimal64` checks an expression and its children,
whether they are DECIMAL64 compatible
2. in case of `ExprNodeGenericFuncDesc`, it first checks some specific
cases: is GenericUDFToDecimal? has annotation? then recursively checks children
nodes by calling `checkExprNodeDescForDecimal64`
the way it works is that it checks every known scenario when the expression
is NOT decimal64 compatible (in which case it returns false), and returns true
otherwise
I'm just thinking aloud, I would appreciate if you can confirm my
understanding, and add a proper javadoc with the above snippet + my
understanding
I believe we should have never added non-trivial conditions without
explanation like below :)
```
if (udf instanceof GenericUDFToDecimal) {
return true;
}
```
assming that we're on the same page with `checkExprNodeDescForDecimal64`, I
have one more (maybe last) quesion, which is maybe related to the same scenario
you mentioned: `CAST(integer_column as DECIMAL(15,1))`: for an integer
`isDecimalFamily` returns false, I assume, in which case it simply falls to the
`return true` branch, is it correct and expected?
whatever is the answer, I would appreciate one more "EXPLAIN VECTORIZATION
DETAIL" and query in the q file with integer, or anything that hits this
codepath:
```
if (udf instanceof GenericUDFToDecimal) {
ExprNodeDesc child = exprNodeDesc.getChildren().get(0);
if (isDecimalFamily(child.getTypeString())) {
return checkExprNodeDescForDecimal64(child);
}
return true; <----- THIS
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
