[SPARK-38542][SQL]UnsafeHashedRelation should serialize numKeys out #35836
Conversation
wangyum
left a comment
wangyum
left a comment
There was a problem hiding this comment.
We also encountered this issue after replacing SoftReference with WeakReference. cc @cloud-fan
| readInt: () => Int, | ||
| readLong: () => Long, | ||
| readBuffer: (Array[Byte], Int, Int) => Unit): Unit = { | ||
| numKeys = readInt() |
There was a problem hiding this comment.
how did we get the num keys before?
There was a problem hiding this comment.
This field has not been serialized from the beginning.
The reason why this problem was not triggered before is because TorrentBroadcast._value is like this:
private lazy val _value: T = readBroadcastBlock()
It was modified after #22995 and became like this:
private var _value: SoftReference[T] = _
The driver side may deserialize UnsafeHashedRelation, and then cause this problem.
There was a problem hiding this comment.
The executors also need to deserialize the hash relation, why they are fine?
There was a problem hiding this comment.
Currently, the numKeys field is only used in the UnsafeHashedRelation.keys() method, and UnsafeHashedRelation.keys() is only used on the driver side, so the executor will not trigger this problem.
And the numKeys field does not affect the execution of the UnsafeHashedRelation.get() method, so there was no problem before.
My personal understanding, you can also help analyze.
|
Can one of the admins verify this patch? |
|
thanks, merging to master/3.2! |
### What changes were proposed in this pull request? UnsafeHashedRelation should serialize numKeys out ### Why are the changes needed? One case I found was this: We turned on ReusedExchange(BroadcastExchange), but the returned UnsafeHashedRelation is missing numKeys. The reason is that the current type of TorrentBroadcast._value is SoftReference, so the UnsafeHashedRelation obtained by deserialization loses numKeys, which will lead to incorrect calculation results. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Added a line of assert to an existing unit test Closes #35836 from mcdull-zhang/UnsafeHashed. Authored-by: mcdull-zhang <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 8476c8b) Signed-off-by: Wenchen Fan <[email protected]>
|
@cloud-fan it seems this pr does not backport into branch-3.3, guess the |
|
hmm, @mcdull-zhang can you open a backport PR for 3.3? thanks! |
### What changes were proposed in this pull request? UnsafeHashedRelation should serialize numKeys out ### Why are the changes needed? One case I found was this: We turned on ReusedExchange(BroadcastExchange), but the returned UnsafeHashedRelation is missing numKeys. The reason is that the current type of TorrentBroadcast._value is SoftReference, so the UnsafeHashedRelation obtained by deserialization loses numKeys, which will lead to incorrect calculation results. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Added a line of assert to an existing unit test Closes #35836 from mcdull-zhang/UnsafeHashed. Authored-by: mcdull-zhang <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>
|
I've backported it to 3.3 |
oh sorry, I just saw these conversations. |
### What changes were proposed in this pull request? UnsafeHashedRelation should serialize numKeys out ### Why are the changes needed? One case I found was this: We turned on ReusedExchange(BroadcastExchange), but the returned UnsafeHashedRelation is missing numKeys. The reason is that the current type of TorrentBroadcast._value is SoftReference, so the UnsafeHashedRelation obtained by deserialization loses numKeys, which will lead to incorrect calculation results. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Added a line of assert to an existing unit test Closes apache#35836 from mcdull-zhang/UnsafeHashed. Authored-by: mcdull-zhang <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 8476c8b) Signed-off-by: Wenchen Fan <[email protected]>
5 participants
What changes were proposed in this pull request?
UnsafeHashedRelation should serialize numKeys out
Why are the changes needed?
One case I found was this:
We turned on ReusedExchange(BroadcastExchange), but the returned UnsafeHashedRelation is missing numKeys.
The reason is that the current type of TorrentBroadcast._value is SoftReference, so the UnsafeHashedRelation obtained by deserialization loses numKeys, which will lead to incorrect calculation results.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Added a line of assert to an existing unit test