abstractdog commented on code in PR #6366:
URL: https://github.com/apache/hive/pull/6366#discussion_r3071957018
##########
ql/src/test/queries/clientpositive/cast_decimal_vectorized.q:
##########
@@ -0,0 +1,9 @@
+CREATE TABLE test_stats0 (e decimal(38,10)) stored as orc;
+insert into test_stats0 (e) values (0.0);
+
+set hive.vectorized.execution.enabled=false;
+select count(*) from test_stats0 where CAST(e as DECIMAL(15,1)) BETWEEN 100.0
AND 1000.0;
+
+
+set hive.vectorized.execution.enabled=true;
+select count(*) from test_stats0 where CAST(e as DECIMAL(15,1)) BETWEEN 100.0
AND 1000.0;
Review Comment:
this patch needs an EXPLAIN VECTORIZATION DETAIL <query> for the same
can you also check if EXPLAIN VECTORIZATION DETAIL shows the difference
between pre/post patch?
##########
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:
hm, while I understand the patch, I can see that the same check is actually
performed a few lines below, see:
https://github.com/apache/hive/blob/01d9111da5e6c17ed49ccd72104aed67d6e302fe/ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java#L1730-L1752
couldn't that be used here?
##########
ql/src/test/queries/clientpositive/cast_decimal_vectorized.q:
##########
@@ -0,0 +1,9 @@
+CREATE TABLE test_stats0 (e decimal(38,10)) stored as orc;
+insert into test_stats0 (e) values (0.0);
+
+set hive.vectorized.execution.enabled=false;
+select count(*) from test_stats0 where CAST(e as DECIMAL(15,1)) BETWEEN 100.0
AND 1000.0;
+
+
+set hive.vectorized.execution.enabled=true;
+select count(*) from test_stats0 where CAST(e as DECIMAL(15,1)) BETWEEN 100.0
AND 1000.0;
Review Comment:
DECIMAL(15,1) resolves to a DECIMAL64, which is good for testing this patch,
I would just widen the context for an extra sanity-check with different
precision to hit the non-DECIMAL64 codepath + EXPLAIN VECTORIZATION DETAIL
--
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]
