kasakrisz commented on code in PR #6575:
URL: https://github.com/apache/hive/pull/6575#discussion_r3557483016
##########
ql/src/test/queries/clientpositive/vector_decimal64_col_multiply_case_subquery_join.q:
##########
@@ -0,0 +1,111 @@
+SET hive.support.concurrency=TRUE;
+SET hive.txn.manager=org.apache.hadoop.hive.ql.lockmgr.DbTxnManager;
+set hive.cbo.enable=true;
+
+-- HIVE-28329: subquery + LEFT JOIN + CAST(decimal) with col * CASE returns
wrong result (0.00000000)
+
+-- Test tables
+create EXTERNAL table test1(
+col1 varchar(28),
+col2 varchar(28),
+col3 decimal(26,6),
+col4 varchar(28)
+);
+
+insert into test1 values('12345678','abc','1.00','DEF');
+
+create EXTERNAL table test2(
+col1 varchar(28),
+col2 varchar(28),
+col3 decimal(26,6),
+col4 varchar(28),
+col5 varchar(28)
+);
+
+insert into test2 values('12345678','abc','1.00','DEF','22222222');
+
+explain vectorization detail
+SELECT
+ int_cost
+FROM
+ (
+ SELECT
+ a.col4,
+ CAST(
+ CASE when a.col1 = '12345678' then a.col3 * case when a.col2 = '1'
then 1.77 else 0.72 end / 100 / 365 * 10 else a.col3 * 10 / 365 / 100 END AS
DECIMAL(26, 9)
+ ) AS int_cost
+ FROM
+ test1 a
+ ) aa
+ LEFT JOIN (
+ SELECT
+ col4
+ FROM
+ test2
+ WHERE
+ col5 = '22222222'
+ ) bb ON trim(aa.col4) = trim(bb.col4);
+
+SELECT
+ int_cost
+FROM
+ (
+ SELECT
+ a.col4,
+ CAST(
+ CASE when a.col1 = '12345678' then a.col3 * case when a.col2 = '1'
then 1.77 else 0.72 end / 100 / 365 * 10 else a.col3 * 10 / 365 / 100 END AS
DECIMAL(26, 9)
+ ) AS int_cost
+ FROM
+ test1 a
+ ) aa
+ LEFT JOIN (
+ SELECT
+ col4
+ FROM
+ test2
+ WHERE
+ col5 = '22222222'
+ ) bb ON trim(aa.col4) = trim(bb.col4);
+
+
+explain vectorization detail
+SELECT
+ int_cost
+FROM
+ (
+ SELECT
+ a.col4,
+ a.col3 * case when a.col2 = 'bc' then 1.77 else 0.72 end
+ AS int_cost
+ FROM
+ test1 a
+ ) aa
+ LEFT JOIN (
+ SELECT
+ col4
+ FROM
+ test2
+ ) bb ON trim(aa.col4) = trim(bb.col4);
Review Comment:
If this simpler query can repro the issue, the other one can be removed.
This one might be simplified further: try removing the subqueries and the left
join. Or use an inner join if it is still necessary.
Please rename the test file accordingly.
##########
ql/src/test/queries/clientpositive/vector_decimal64_col_multiply_case_subquery_join.q:
##########
@@ -0,0 +1,111 @@
+SET hive.support.concurrency=TRUE;
+SET hive.txn.manager=org.apache.hadoop.hive.ql.lockmgr.DbTxnManager;
+set hive.cbo.enable=true;
+
+-- HIVE-28329: subquery + LEFT JOIN + CAST(decimal) with col * CASE returns
wrong result (0.00000000)
+
+-- Test tables
+create EXTERNAL table test1(
+col1 varchar(28),
+col2 varchar(28),
+col3 decimal(26,6),
+col4 varchar(28)
+);
+
+insert into test1 values('12345678','abc','1.00','DEF');
+
+create EXTERNAL table test2(
+col1 varchar(28),
+col2 varchar(28),
+col3 decimal(26,6),
+col4 varchar(28),
+col5 varchar(28)
+);
+
+insert into test2 values('12345678','abc','1.00','DEF','22222222');
+
+explain vectorization detail
+SELECT
+ int_cost
+FROM
+ (
+ SELECT
+ a.col4,
+ CAST(
+ CASE when a.col1 = '12345678' then a.col3 * case when a.col2 = '1'
then 1.77 else 0.72 end / 100 / 365 * 10 else a.col3 * 10 / 365 / 100 END AS
DECIMAL(26, 9)
+ ) AS int_cost
+ FROM
+ test1 a
+ ) aa
+ LEFT JOIN (
+ SELECT
+ col4
+ FROM
+ test2
+ WHERE
+ col5 = '22222222'
+ ) bb ON trim(aa.col4) = trim(bb.col4);
Review Comment:
Can this query be simplified for reproducing the issue?
Does the subqueries and left join contributes to the incorrect result?
Can we use a simpler expression?
##########
ql/src/test/queries/clientpositive/vector_decimal64_col_multiply_case_subquery_join.q:
##########
@@ -0,0 +1,111 @@
+SET hive.support.concurrency=TRUE;
+SET hive.txn.manager=org.apache.hadoop.hive.ql.lockmgr.DbTxnManager;
+set hive.cbo.enable=true;
Review Comment:
Are these necessary to repro the issue?
* The tables in the test are external tables AFAIK we don't need a txn
manager.
* CBO is true by default
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/Vectorizer.java:
##########
@@ -4818,6 +4822,12 @@ public static Operator<? extends OperatorDesc>
vectorizeSelectOperator(
// The following method introduces a cast if x or y is DECIMAL_64 and
parent expression (x % y) is DECIMAL.
try {
fixDecimalDataTypePhysicalVariations(vContext, vectorSelectExprs);
+ for (int i = 0; i < size; i++) {
+ int exprIndex = vectorSelectExprIndexForCol[i];
+ if (exprIndex >= 0) {
+ projectedOutputColumns[i] =
vectorSelectExprs[exprIndex].getOutputColumnNum();
+ }
Review Comment:
Why is this change necessary? I found that `projectedOutputColumns` is set a
few lines earlier here
https://github.com/apache/hive/blob/82edd2a2ef04c7d75431f285f01c85a535ed07c5/ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/Vectorizer.java#L4804
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/Vectorizer.java:
##########
@@ -4914,14 +4910,81 @@ private static VectorExpression
fixDecimalDataTypePhysicalVariations(final Vecto
newParent.setOutputDataTypePhysicalVariation(parent.getOutputDataTypePhysicalVariation());
newParent.setInputTypeInfos(parent.getInputTypeInfos());
newParent.setInputDataTypePhysicalVariations(dataTypePhysicalVariations);
- newParent.setChildExpressions(parent.getChildExpressions());
+ newParent.setChildExpressions(children);
return newParent;
}
}
}
return parent;
}
+ /**
+ * Rebuild constructor arguments for a vector expression after wrapping
DECIMAL_64 child
+ * expressions with {@link ConvertDecimal64ToDecimal}. Column reference
inputs live in
+ * {@link VectorExpression#inputColumnNum} and are not included in
childExpressions, so they
+ * must be preserved when re-instantiating the parent.
+ */
+ static Object[] rebuildArgumentsForDecimal64(VectorExpression parent,
+ VectorExpression[] children) {
+ int[] updatedInputCols = extractInputColumnNums(parent);
+ replaceConvertedChildColumns(updatedInputCols, children);
+ return createDecimal64Arguments(updatedInputCols, parent);
+ }
+
+ private static int[] extractInputColumnNums(VectorExpression parent) {
+ int inputCount = 0;
+ for (int col : parent.inputColumnNum) {
+ if (col != -1) {
+ inputCount++;
+ }
+ }
+
+ int[] updatedInputCols = new int[inputCount];
+ int idx = 0;
+ for (int col : parent.inputColumnNum) {
+ if (col != -1) {
+ updatedInputCols[idx++] = col;
+ }
+ }
+ return updatedInputCols;
+ }
+
+ private static void replaceConvertedChildColumns(int[] updatedInputCols,
Review Comment:
nit. It is not clear what is replaced to what from the method signature.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/physical/Vectorizer.java:
##########
@@ -4914,14 +4910,81 @@ private static VectorExpression
fixDecimalDataTypePhysicalVariations(final Vecto
newParent.setOutputDataTypePhysicalVariation(parent.getOutputDataTypePhysicalVariation());
newParent.setInputTypeInfos(parent.getInputTypeInfos());
newParent.setInputDataTypePhysicalVariations(dataTypePhysicalVariations);
- newParent.setChildExpressions(parent.getChildExpressions());
+ newParent.setChildExpressions(children);
return newParent;
}
}
}
return parent;
}
+ /**
+ * Rebuild constructor arguments for a vector expression after wrapping
DECIMAL_64 child
+ * expressions with {@link ConvertDecimal64ToDecimal}. Column reference
inputs live in
+ * {@link VectorExpression#inputColumnNum} and are not included in
childExpressions, so they
+ * must be preserved when re-instantiating the parent.
+ */
+ static Object[] rebuildArgumentsForDecimal64(VectorExpression parent,
+ VectorExpression[] children) {
+ int[] updatedInputCols = extractInputColumnNums(parent);
+ replaceConvertedChildColumns(updatedInputCols, children);
+ return createDecimal64Arguments(updatedInputCols, parent);
+ }
+
+ private static int[] extractInputColumnNums(VectorExpression parent) {
+ int inputCount = 0;
+ for (int col : parent.inputColumnNum) {
+ if (col != -1) {
+ inputCount++;
+ }
+ }
+
+ int[] updatedInputCols = new int[inputCount];
Review Comment:
nit. Why is this called `updatedInputCols`?
IIUC these are the non -1 column numbers, the extracted ones, right?
--
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]
