[SPARK-24076][SQL] Use different seed in HashAggregate to avoid hash conflict

[yucai]

What changes were proposed in this pull request?

HashAggregate uses the same hash algorithm and seed as ShuffleExchange, it may lead to bad hash conflict when shuffle.partitions=8192*n.

Considering below example:

SET spark.sql.shuffle.partitions=8192;
INSERT OVERWRITE TABLE target_xxx
SELECT
 item_id,
 auct_end_dt
FROM
 from source_xxx
GROUP BY
 item_id,
 auct_end_dt;

In the shuffle stage, if user sets the shuffle.partition = 8192, all tuples in the same partition will meet the following relationship:

hash(tuple x) = hash(tuple y) + n * 8192

Then in the next HashAggregate stage, all tuples from the same partition need be put into a 16K BytesToBytesMap (unsafeRowAggBuffer).

Here, the HashAggregate uses the same hash algorithm on the same expression as shuffle, and uses the same seed, and 16K = 8192 * 2, so actually, all tuples in the same parititon will only be hashed to 2 different places in the BytesToBytesMap. It is bad hash conflict. With BytesToBytesMap growing, the conflict will always exist.

Before change:

After change:

How was this patch tested?

Unit tests and production cases.

[maropu]

oh... good catch. I think you'd be better to put the detailed info. (written in the jira) in the description above?

[maropu]

How about writing comments for the reason why we set the seed value here?

[yucai]

@maropu Thanks for comments, I have updated, could you help take a look at?

[maropu]

cc: @hvanhovell

[viirya]

Can you also show the screenshot after this change?

[SparkQA]

Test build #89830 has finished for PR 21149 at commit 5e88468.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #89839 has finished for PR 21149 at commit 0818618.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[maropu]

ping @hvanhovell @gatorsmile

[hvanhovell]

LGTM - merging to master. Thanks!

[yucai]

@maropu @hvanhovell thanks very much!

[cloud-fan]

can we just use UnsafeRow.hashCode here?

[yucai]

@cloud-fan you mean unsafeRowKeys.hashCode(), right?
I think it is a good idea, unsafe row has [null bit set] etc., the result should be different, we don't need weird 48 also. Do you want me to create a followup PR?

[cloud-fan]

yes please, thanks!

[nikitagkonda]

@cloud-fan would this perform slower since now we are moving to interpreted version for hashcode generation? If not then why didn't we use unsafeRowKeys.hashCode() in the first place?

[cloud-fan]

it should be faster, as unsafeRowKeys.hashCode() is just one function call. I don't know why we didn't do it in the first place, the code is pretty old.