kasakrisz commented on code in PR #3706:
URL: https://github.com/apache/hive/pull/3706#discussion_r1006475709
##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/topnkey/TopNKeyPushdownProcessor.java:
##########
@@ -347,14 +347,12 @@ private void pushdownThroughTopNKey(TopNKeyOperator
topNKey) throws SemanticExce
if (topNKeyDesc.getKeyColumns().size() == commonKeyPrefix.size()) {
// TNK keys are subset of the parent TNK keys
pushdownThroughParent(topNKey);
- if (topNKey.getChildOperators().get(0).getType() ==
OperatorType.TOPNKEY) {
- LOG.debug("Removing {} since child {} supersedes it",
parent.getName(), topNKey.getName());
-
topNKey.getParentOperators().get(0).removeChildAndAdoptItsChildren(topNKey);
- }
+ LOG.debug("Removing {} since child {} supersedes it",
parent.getName(), topNKey.getName());
+
parent.getParentOperators().get(0).removeChildAndAdoptItsChildren(parent);
Review Comment:
This does not seems to be a valid change because the previous statement is
`pushdownThroughParent(topNKey);`
Let assume we have the following operator subtree:
```
SEL
TNK[P] (key = a, b)
TNK[C] (key = a)
```
Two outcome exists depending on how far the
`pushdownThroughParent(topNKey);` can push down `TNK[C]`:
* `TNK[C]` pushed through `SEL`
```
TNK[C] (key = a)
SEL
TNK[P] (key = a, b)
```
We stop here since `SEL` should process rows filtered by `TNK[P] (key = a,
b)`
* `TNK[C]` pushed through only `TNK[P]`
```
SEL
TNK[C] (key = a)
TNK[P] (key = a, b)
```
In this case `TNK[C]` can be removed since `TNK[P]` filters out more rows
and there are no operators between the TNKs which would process the rows
filtered out by `TNK[P]`.
This is what the condition `(topNKey.getChildOperators().get(0).getType() ==
OperatorType.TOPNKEY)` checks and if it is true we do the remove:
```
SEL
TNK[P] (key = a, b)
```
##########
ql/src/test/results/clientpositive/llap/bucket_groupby.q.out:
##########
@@ -1753,9 +1753,9 @@ STAGE PLANS:
filterExpr: (ds = '103') (type: boolean)
Statistics: Num rows: 500 Data size: 89000 Basic stats:
COMPLETE Column stats: COMPLETE
Top N Key Operator
- sort order: ++
- keys: key (type: string), value (type: string)
- null sort order: zz
+ sort order: +
+ keys: key (type: string)
+ null sort order: z
Review Comment:
I think removing one of the key columns from TNK is too aggressive in this
case because both GBYs in the mapper and reducer has the same keys.
--
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]
