HIVE-29433: ClassCastException in FilterLongColumnBetween.evaluate when vectorization is enabled: DecimalColumnVector cannot be cast to class LongColumnVector

[tanishq-chugh]

What changes were proposed in this pull request?

Fix checkExprNodeDescForDecimal64 to ensure the input type is also validated when udf is an instance of GenericUDFToDecimal

Why are the changes needed?

To Fix ClassCastException occurring at runtime

Does this PR introduce any user-facing change?

Yes, it fixes query containing a decimal cast running into ClassCastException when vectorization is enabled

How was this patch tested?

Qtest

[abstractdog]

thanks @tanishq-chugh for the patch so far, added some comments

[abstractdog]

this patch needs an EXPLAIN VECTORIZATION DETAIL for the same
can you also check if EXPLAIN VECTORIZATION DETAIL shows the difference between pre/post patch?

[tanishq-chugh]

Yes. Have added the same in commit - 78bcca6

The only difference in EXPLAIN VECTORIZATION DETAIL with and without this patch lies in the predicateExpression.

Without this patch, predicateExpression is:
predicateExpression: FilterDecimal64ColumnBetween(col 3:decimal(15,1)/DECIMAL_64, decimal64LeftVal 1000, decimalLeftVal 1000, decimal64RightVal 10000, decimalRightVal 10000)(children: CastDecimalToDecimal(col 0:decimal(38,10)) -> 3:decimal(15,1))

With this patch, predicateExpression is:
predicateExpression: FilterDecimalColumnBetween(col 3:decimal(15,1), left 100, right 1000)(children: CastDecimalToDecimal(col 0:decimal(38,10)) -> 3:decimal(15,1))

[abstractdog]

thanks, I believe this exactly shows the expected behavior, which is that FilterDecimalColumnBetween is used, otherwise, the column vector produced by CastDecimalToDecimal (which is an instance of DecimalColumnVector) cannot be used from the parent expression FilterDecimal64ColumnBetween which expects a LongColumnVector (which represents the decimal64)

[abstractdog]

hm, while I understand the patch, I can see that the same check is actually performed a few lines below, see:

hive/ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java

Lines 1730 to 1752 in 01d9111

	// Carefully check the children to make sure they are Decimal64.
	List<ExprNodeDesc> children = exprNodeDesc.getChildren();
	for (ExprNodeDesc childExprNodeDesc : children) {
	// Some cases were converted before calling getVectorExpressionForUdf.
	// So, emulate those cases first.
	if (childExprNodeDesc instanceof ExprNodeConstantDesc) {
	DecimalTypeInfo childDecimalTypeInfo =
	decimalTypeFromCastToDecimal(childExprNodeDesc, returnDecimalType);
	if (childDecimalTypeInfo == null) {
	return false;
	}
	if (!checkTypeInfoForDecimal64(childDecimalTypeInfo)) {
	return false;
	}
	continue;
	}
	// Otherwise, recurse.
	if (!checkExprNodeDescForDecimal64(childExprNodeDesc)) {
	return false;
	}

couldn't that be used here?

[tanishq-chugh]

Hi @abstractdog , Thanks for checking this.
I understand that we have the same check in the code following right after in the for loop, but currently in such casts, the code flow does not reach the for loop and directly returns true from Code Link, which is not correct for such decimal to decimal casts and causing issue.

Also, if we remove the whole check of udf instanceof GenericUDFToDecimal to make the code flow fall through the code logic ahead, i believe it will cause performance regression for other casts. For exm: if we have an integer column here in same case: CAST(integer_column as DECIMAL(15,1)) , this would tend to return false in such a case and IMO would be a performance regression.

Please do let me know what are your thoughts.

[abstractdog]

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
      }

[abstractdog]

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

[tanishq-chugh]

Definitely, it would be better to have a sanity check with a different precision as well. Added the same in commit - 78bcca6

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
1.1% Duplication on New Code

See analysis details on SonarQube Cloud