Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/HashAggregateExec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -755,7 +755,10 @@ case class HashAggregateExec(
}

// generate hash code for key
val hashExpr = Murmur3Hash(groupingExpressions, 42)
Copy link
Member

@maropu maropu Apr 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// SPARK-24076: HashAggregate uses the same hash algorithm on the same expressions
// as ShuffleExchange, it may lead to bad hash conflict when shuffle.partitions=8192*n,
// pick a different seed to avoid this conflict
val hashExpr = Murmur3Hash(groupingExpressions, 48)
Copy link
Contributor

@cloud-fan cloud-fan Feb 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we just use UnsafeRow.hashCode here?

Copy link
Contributor Author

@yucai yucai Feb 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

@cloud-fan cloud-fan Feb 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes please, thanks!

Copy link

@nikitagkonda nikitagkonda Mar 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

@cloud-fan cloud-fan Mar 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

val hashEval = BindReferences.bindReference(hashExpr, child.output).genCode(ctx)

val (checkFallbackForGeneratedHashMap, checkFallbackForBytesToBytesMap, resetCounter,
Expand Down