Skip to content
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.

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

chore: Add safety check to CometBuffer #1050

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

viirya
Copy link
Member

@viirya viirya commented Nov 4, 2024

Which issue does this PR close?

Closes #1035.

Rationale for this change

What changes are included in this PR?

How are these changes tested?

@viirya
Copy link
Member Author

viirya commented Nov 5, 2024

This cannot pass all tests due to an issue (#1059) at Spark side.

@viirya
Copy link
Member Author

viirya commented Nov 5, 2024

I'm considering to add a workaround in Comet as waiting for Spark change might take longer time (and for 3.4/3.5 such change might not easily to backport).

* Copied from Spark `ColumnarToRowExec`. Comet needs the fix for SPARK-50235 but cannot wait for
* the fix to be released in Spark versions. We copy the implementation here to apply the fix.
*/
case class CometColumnarToRowExec(child: SparkPlan)
Copy link
Member

Choose a reason for hiding this comment

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

We could potentially move this to the shim layer and only use when using versions of Spark that do not have the fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CometBuffer can potentially lead to concurrent modification of a held buffer (aka is "Unsound" in Rust terms)
3 participants