HIVE-28489: Partition the input data of GroupBy with GroupingSet - #5424
Conversation
|
ngsg
changed the title
asf-ci-hive
added
tests pending
tests passed
and removed
tests passed
tests pending
labels
BsoBird
left a comment
BsoBird
left a comment
There was a problem hiding this comment.
It seems like there are no major issues. However, it would be best to have a few more people review it together.
| import org.apache.hadoop.hive.ql.plan.ColStatistics; | ||
| import org.apache.hadoop.hive.ql.plan.ExprNodeColumnDesc; | ||
| import org.apache.hadoop.hive.ql.plan.ExprNodeDesc; | ||
| import org.apache.hadoop.hive.ql.plan.ExprNodeFieldDesc; |
There was a problem hiding this comment.
Remove unused import entries.
There was a problem hiding this comment.
removed an unused import.
asf-ci-hive
added
tests pending
tests passed
and removed
tests passed
tests pending
labels
|
The intention is admirable, but I am not 100% confident that this is the best approach because I need to know how GROUPING SETS works. It still needs time for me |
Hello. @okumin Trino seems to have adopted a similar scheme to optimize grouping sets. It appears that the modifications in this submission are intended to port the optimization rules from Trino to HIVE. Since Trino has been using this rule for a long time, and we have received positive feedback from users after introducing this patch, I personally believe that the approach used in this submission is a viable solution. After all, if we find a better solution, it would be quite easy for us to replace the changes introduced by this PR. |
|
|
||
| TableDesc keyTable = PlanUtils.getReduceKeyTableDesc(new ArrayList<>(), "", ""); | ||
|
|
||
| List<ExprNodeDesc> keyColumns = new ArrayList<>(); |
There was a problem hiding this comment.
I could be wrong. Should we also use the partition key as a sort key to optimize the hash table utilization? I see we already achieved a good result with this implementation, so I might be wrong.
If we do that, I wonder if we can generalize the utility to insert REDUCE_SINK + SEL somewhere because it is a useful conversion...
There was a problem hiding this comment.
I agree that inserting RS + SEL might be useful optimization if we can find the place where heavy sort happens. However, I'm not sure whether HashTable can fully take advantage of pre-sorting.
I'll test your idea in our cluster and share the result later on.
There was a problem hiding this comment.
I tested your idea using 10TB TPC-DS dataset. I used query18, query22, query67 and set hive.optimize.grouping.set.threshold to 1,000,000,000. I ran each query 3 times.
| query18 | query22 | query67 | |
|---|---|---|---|
| # GBY input rows estimation | 382,977,011 | 258,698,384 | 5,279,977,323 |
| # GBY output rows estimation | 1,914,885,055 | 1,293,491,920 | 47,519,795,907 |
| Query execution time(current impl.) | 27.575, 24.089, 23.265 | 14.827, 13.925, 12.959 | 426.62, 419.793, 414.403 |
| Query execution time(with additional sort) | 27.636, 24.836, 24.054 | 16.531, 15.729, 15.624 | 318.425, 323.733, 336.545 |
There is almost no change in the query execution time of relatively small GroupBys (query18 and query22). However, it appears that a large GroupBy benefits from the additional sort. Therefore, I conclude that adding a sort before large GroupBys is worthwhile. The latest commit(efe982c) includes an implementation of additional sort that I used for the test.
|
@BsoBird Thanks. Could you give us a document or equivalent implementation of Trino? |
asf-ci-hive
added
tests pending
tests unstable
and removed
tests passed
tests pending
labels
asf-ci-hive
added
tests pending
tests passed
and removed
tests unstable
tests pending
labels
okumin
left a comment
okumin
left a comment
There was a problem hiding this comment.
Mostly, looks good to me based on my experience. Only one comment.
| int numReducers = Utilities.estimateReducers( | ||
| parentOp.getStatistics().getDataSize(), context.bytesPerReducer, context.maxReducers, false); | ||
|
|
||
| ReduceSinkDesc rsConf = new ReduceSinkDesc(keyColumns, 0, valueColumns, keyColumnNames, |
There was a problem hiding this comment.
I guess the second argument should be keyColumns.size()
There was a problem hiding this comment.
You're right. Fixed it to use keyColumns.size().
|
4 participants
What changes were proposed in this pull request?
This patch introduces a new optimization for GroupBy with GroupingSet. GroupingSet often emits too much rows and becomes a bottleneck of query execution. The proposed optimization tries mitigating this situation by reducing the cardinality of GroupBy key. For the detailed explanation, please checkout the slides attached in the JIRA page (HIVE-28489).
This patch introduces a new configuration key:
hive.optimize.grouping.set.threshold. The default value of 1,000,000,000 is chosen because it showed best performance in 10TB TPC-DS experiment. Setting this value to negative number disables the optimization.Why are the changes needed?
To improve query execution time of GroupBy with GroupingSet.
Does this PR introduce any user-facing change?
No
Is the change a dependency upgrade?
No
How was this patch tested?
We tested this patch using 10TB TPC-DS experiment. This patch contains a qfile test to verify optimized query plan.