[GLUTEN-9671][VL] Fix broadcast exchange stackoverflow due to Kryo serialization#10541
Merged
FelixYBW merged 2 commits intoapache:mainfrom Aug 29, 2025
Merged
Conversation
This pull request introduces a safer and more robust approach for handling Spark's `BroadcastMode` during serialization. The main improvement is the introduction of a new `SafeBroadcastMode` abstraction and related utilities, which help avoid serialization issues that caused a Stackoverflow exception during broadcast exchanges. BroadcastMode was introduced in the [PR](apache#8116) that caused the issues observed. HashedRelationBroadcastMode embeds Catalyst expression trees, which are not safe to Kryo-serialize when running with `spark.kryo.referenceTracking=false` (default internally). With this change, the broadcast payload now contains only primitives and byte arrays (no Catalyst trees). For bound keys, we serialize just column ordinals (+ null-aware flag) and for computed keys (e.g., upper(col)), we serialize the key expressions once as Java bytes and deserialize only where needed to build projections. Ran internal test set (50 queries) and ran other query specifically checking if `spark.gluten.velox.offHeapBroadcastBuildRelation.enabled=true;` works.
Contributor
Contributor
|
@felixloesing Thanks for your fix. Just curious — why does Gluten require this change, while vanilla Spark doesn't? |
Contributor
Author
@JkSelf This is also described in the PR that introduced the |
JkSelf
reviewed
Aug 28, 2025
Contributor
JkSelf
left a comment
JkSelf
left a comment
There was a problem hiding this comment.
Thanks for your fix. LGTM except two minor comments.
| oos.writeObject(keys) | ||
| oos.flush() | ||
| bos.toByteArray | ||
| } finally { |
Contributor
There was a problem hiding this comment.
nit: Can we add a catch clause to log the exception and make debugging easier?
| try { | ||
| ois = new ObjectInputStream(bis) | ||
| ois.readObject().asInstanceOf[Seq[Expression]] | ||
| } finally { |
Contributor
Author
|
@JkSelf Thanks for the review! Added a catch clause and logged the exception. |
JkSelf
approved these changes
Aug 29, 2025
Contributor
JkSelf
left a comment
JkSelf
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the fix.
wForget
pushed a commit
to wForget/gluten
that referenced
this pull request
Sep 17, 2025
…rialization (apache#10541) This pull request introduces a safer and more robust approach for handling Spark's BroadcastMode during serialization. The main improvement is the introduction of a new SafeBroadcastMode abstraction and related utilities, which help avoid serialization issues that caused a Stackoverflow exception during broadcast exchanges. BroadcastMode was introduced in this PR that caused the issue we observed. HashedRelationBroadcastMode embeds Catalyst expression trees, which are not safe to Kryo-serialize when running with spark.kryo.referenceTracking=false (default internally). With this change, the broadcast payload now contains only primitives and byte arrays (no Catalyst trees). For bound keys, we serialize just column ordinals (+ null-aware flag) and for computed keys (e.g., upper(col)), we serialize the key expressions once as Java bytes and deserialize only where needed to build projections. (cherry picked from commit 91c52e1)
wForget
added a commit
that referenced
this pull request
Sep 17, 2025
…ckoverflow due to Kryo serialization #10733 This pull request introduces a safer and more robust approach for handling Spark's BroadcastMode during serialization. The main improvement is the introduction of a new SafeBroadcastMode abstraction and related utilities, which help avoid serialization issues that caused a Stackoverflow exception during broadcast exchanges. BroadcastMode was introduced in this PR that caused the issue we observed. HashedRelationBroadcastMode embeds Catalyst expression trees, which are not safe to Kryo-serialize when running with spark.kryo.referenceTracking=false (default internally). With this change, the broadcast payload now contains only primitives and byte arrays (no Catalyst trees). For bound keys, we serialize just column ordinals (+ null-aware flag) and for computed keys (e.g., upper(col)), we serialize the key expressions once as Java bytes and deserialize only where needed to build projections. (cherry picked from commit 91c52e1) Co-authored-by: Felix Loesing <felix.loesing@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a safer and more robust approach for handling Spark's
BroadcastModeduring serialization. The main improvement is the introduction of a newSafeBroadcastModeabstraction and related utilities, which help avoid serialization issues that caused a Stackoverflow exception during broadcast exchanges. BroadcastMode was introduced in this PR that caused the issue we observed. HashedRelationBroadcastMode embeds Catalyst expression trees, which are not safe to Kryo-serialize when running withspark.kryo.referenceTracking=false(default internally).With this change, the broadcast payload now contains only primitives and byte arrays (no Catalyst trees). For bound keys, we serialize just column ordinals (+ null-aware flag) and for computed keys (e.g., upper(col)), we serialize the key expressions once as Java bytes and deserialize only where needed to build projections.
Test Plan
Ran internal test set (50 queries) and ran other query specifically checking if
spark.gluten.velox.offHeapBroadcastBuildRelation.enabled=true;works.