zabetak commented on code in PR #5663:
URL: https://github.com/apache/hive/pull/5663#discussion_r1981122813
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/RexNodeConverter.java:
##########
@@ -108,22 +109,24 @@
import java.util.Collections;
import java.util.List;
+import static java.util.Arrays.asList;
+
/**
* Class that contains logic to translate Hive expressions ({@link
ExprNodeDesc})
* into Calcite expressions ({@link RexNode}).
*/
public class RexNodeConverter {
private final RexBuilder rexBuilder;
- private final RelDataTypeFactory typeFactory;
+ private final FunctionHelper functionHelper;
Review Comment:
The `HiveFunctionHelper` relies and calls `RexNodeConverter` and with this
change make `RexNodeConverter` rely on `HiveFunctionHelper` thus creating a
strange cyclic dependency between the classes.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/RexNodeConverter.java:
##########
@@ -366,8 +369,7 @@ public static RexNode handleExplicitCast(GenericUDF udf,
RelDataType returnType,
* It will be transformed into:
* CASE WHEN =(x + y, 1) THEN 'fee' WHEN =(x + y, 2) THEN 'fie' ELSE null END
*/
- public static List<RexNode> rewriteCaseChildren(String funcText,
List<RexNode> childRexNodeLst,
- RexBuilder rexBuilder) throws SemanticException {
+ public List<RexNode> rewriteCaseChildren(String funcText, List<RexNode>
childRexNodeLst) throws SemanticException {
List<RexNode> newChildRexNodeLst = new ArrayList<>();
if (FunctionRegistry.getNormalizedFunctionName(funcText).equals("case")) {
Review Comment:
I think it was a mistake to have two separate functions that are doing the
same thing and one is basically syntactic sugar for the other.
I am wondering if we could somehow do the transformation (convert to WHEN
EQUALS) at the AST level (or soon afterwards) to eliminate completely the
occurrences of the `CASE` function in logical/physical/execution phase. If
that's possible then we could drop quite some code and avoid ambiguities and
subtle bugs as this one.
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/RexNodeConverter.java:
##########
@@ -377,14 +379,12 @@ public static List<RexNode> rewriteCaseChildren(String
funcText, List<RexNode> c
if (i % 2 == 1) {
// We rewrite it
RexNode node = childRexNodeLst.get(i);
- if (node.isA(SqlKind.LITERAL) &&
!node.getType().equals(firstPred.getType())) {
- // this effectively changes the type of the literal to that of the
predicate
- // to which it is anyway going to be compared with
- // ex: CASE WHEN =($0:SMALLINT, 1:INTEGER) ... => CASE WHEN
=($0:SMALLINT, 1:SMALLINT)
- node = rexBuilder.makeCast(firstPred.getType(), node);
- }
- newChildRexNodeLst.add(
- rexBuilder.makeCall(SqlStdOperatorTable.EQUALS, firstPred,
node));
+ List<RexNode> newInputs = functionHelper.convertInputs(
Review Comment:
The use of `functionHelper` here seems a bit out of place since it is not
used anywhere else in this class. It does not seem appropriate to use it only
for translating the `CASE` expression.
I guess there are various other places inside this class that we create an
equality comparison between two operands. It doesn't seem appropriate to have
different logic for those parts. Can't we somehow use the existing logic
instead of introducing the functionHelper?
##########
ql/src/test/results/clientpositive/llap/cbo_case_when_wrong_type.q.out:
##########
@@ -56,7 +56,7 @@ POSTHOOK: Input: default@t
#### A masked pattern was here ####
CBO PLAN:
HiveProject($f0=[1])
- HiveFilter(condition=[IN($0, 1:SMALLINT, 2:SMALLINT)])
+ HiveFilter(condition=[AND(CASE(=(CAST($0):INTEGER, 1), true,
=(CAST($0):INTEGER, 2), true, false), IN($0, 1:SMALLINT, 2:SMALLINT,
3:SMALLINT))])
Review Comment:
This seems like a regression from HIVE-25734. Moreover the CBO plan here is
different from the CBO plan of the equivalent query with `case when a = 1 ...`
showing that there is some inconsistency between equivalent expressions.
--
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]
