HIVE-29029: When stack UDTF is used with UNION ALL throwing ClassCastException in CBO mode. #5978
Conversation
…Exception in CBO mode.
| SELECT STACK(3,'A',10,date '2015-01-01','z','B',20,date '2016-01-01','y','C',30,date '2017-08-09','x') AS (col0,col1,col2,col3) | ||
| UNION ALL | ||
| SELECT STACK(3,'A',10,date '2015-01-01','n','B',20,date '2016-01-01','m','C',30,date '2017-08-09','l') AS (col0,col1,col2,col3); |
There was a problem hiding this comment.
The ClassCastException occurs during compilation so testing result correctness is slightly out of scope for the current PR. It is probably fine to add one end-to-end test but not sure if we need all of them. Does each test in this file contribute to additional test coverage?
There was a problem hiding this comment.
I just want to make sure the results are also as expected, if not required I can remove.
|
|
||
| SELECT * FROM (VALUES(1, '1'), (2, 'orange'), (5, 'yellow')) AS Colors1 | ||
| UNION ALL | ||
| SELECT * FROM (VALUES(10, 'green'), (11, 'blue'), (12, 'indigo'), (20, 'violet')) AS Colors2 |
There was a problem hiding this comment.
Do we need two VALUES branches to trigger the problem or one suffices? Do we need three rows on each VALUES?
Whatever is not needed to repro the problem should be dropped. Keeping tests minimal speeds-up reviews, improves readability, and minimizes flakiness in the longterm.
There was a problem hiding this comment.
Here I want to test some scenario like UNION ALL is applied for VALUES, for which rule will come into picture and converts two values into one INLINE udtf and STACK, for which rule should not be applied.
| SELECT INLINE(array(struct('A',10,date '2015-01-01'),struct('B',20,date '2016-02-02'))) | ||
| UNION ALL | ||
| SELECT STACK(2,'X',10,date '2017-01-01','Y',20,date '2018-01-01'); | ||
|
|
||
| EXPLAIN SELECT INLINE(array(struct('A',10,date '2015-01-01'),struct('B',20,date '2016-02-02'))) | ||
| UNION ALL | ||
| SELECT STACK(2,'X',30,date '2017-01-01','Y',40,date '2018-01-01'); | ||
|
|
||
| SELECT INLINE(array(struct('A',10,date '2015-01-01'),struct('B',20,date '2015-02-02'))) | ||
| UNION ALL | ||
| SELECT INLINE(array(struct('C',30,date '2016-01-01'),struct('D',40,date '2016-02-02'))); | ||
|
|
||
| EXPLAIN SELECT INLINE(array(struct('A',10,date '2015-01-01'),struct('B',20,date '2015-02-02'))) | ||
| UNION ALL | ||
| SELECT INLINE(array(struct('C',30,date '2016-01-01'),struct('D',40,date '2016-02-02'))); | ||
|
|
||
| SELECT EXPLODE(map('A',10,'B',20,'C',30)) | ||
| UNION ALL | ||
| SELECT EXPLODE(map('X',70,'Y',80,'Z',90)); |
There was a problem hiding this comment.
I like the fact that we are testing various UDTFs with UNION ALL but not sure under what criteria we picked the combinations to test. Is it exhaustive? Is it on most commonly used? Is it something else?
The fix is localized in a single rule so if we want to add test cases for this purpose it feels more appropriate to do it via unit tests (see Test*Rule.java classes) if that's feasible.
There was a problem hiding this comment.
Removed other UDTFs which are not required to be tested as a part of this PR.
| if (!(((RexCall)((HiveTableFunctionScan) input).getCall()).getOperands().get(0) instanceof RexCall)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This check is not very readable and it also makes various assumptions about the type and operands of TableFunctionScan#getCall that may lead to other ClassCastException or other problems. Maybe we should consider adding a helper function and putting all the necessary checks there. For instance:
private static boolean isInlineArrayCall(RexNode expr) {
if (!(expr instanceof RexCall)) {
return false;
}
RexCall call = (RexCall) expr;
SqlOperator op = call.getOperator();
if (!"inline".equalsIgnoreCase(op.getName())) {
return false;
}
if (call.getOperands().size() != 1) {
return false;
}
RexNode operand = call.getOperands().get(0);
if (!(operand instanceof RexCall)) {
return false;
}
call = (RexCall) operand;
return "array".equalsIgnoreCase(call.getOperator().getName());
}It is a bit verbose but not sure if there is a better way to put it :/
Other than that I am not sure if we should make the check here or rather in Line 145-146 as you initially proposed.
There was a problem hiding this comment.
Added required checks now with separate if conditions. We cannot use "inline" text here as for VALUES also it creates dummy table.
asf-ci-hive
added
tests unstable
tests pending
and removed
tests pending
tests unstable
labels
|
HIVE-29029: When stack UDTF is used with UNION ALL throwing ClassCastException in CBO mode.
What changes were proposed in this pull request?
Ignore HiveUnionSimpleSelectsToInlineTableRule when stack UDTF is used with UNION ALL.
Why are the changes needed?
To fix the ClassCastException when stack UDTF is used with UNION ALL.
Does this PR introduce any user-facing change?
No
How was this patch tested?
mvn -Dtest=TestMiniLlapLocalCliDriver -Dqfile=udtf_with_unionall.q -pl itests/qtest -Pitests