-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-34049][SS] DataSource V2: Use Write abstraction in StreamExecution #31093
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
Conversation
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
jaceklaskowski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (non-binding)
|
Test build #133843 has finished for PR 31093 at commit
|
HeartSaVioR
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
| this | ||
| } | ||
|
|
||
| override def buildForBatch(): BatchWrite = writer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, this change is optional (shouldn't break anything even this is not change), as we're trying to guarantee backward compatibility, right? I think it's better to change this - just wanted to check about compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a test module, it's okay to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we could leave it as is. I changed because it would previously fail if a data source implemented the build method only.
In the next PR, I am going to implement RequiredDistributionAndOrdering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confusion. I didn't mean we should leave it as it is. I just meant to confirm whether the test won't fail even without the fix, to make sure we guarantee compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I confirm it is compatible, @HeartSaVioR.
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM. Thank you, @aokolnychyi and all.
Merged to master for Apache Spark 3.2.0.
|
Thanks everyone! |
…tion ### What changes were proposed in this pull request? This PR makes `StreamExecution` use the `Write` abstraction introduced in SPARK-33779. Note: we will need separate plans for streaming writes in order to support the required distribution and ordering in SS. This change only migrates to the `Write` abstraction. ### Why are the changes needed? These changes prevent exceptions from data sources that implement only the `build` method in `WriteBuilder`. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests. Closes apache#31093 from aokolnychyi/spark-34049. Authored-by: Anton Okolnychyi <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
What changes were proposed in this pull request?
This PR makes
StreamExecutionuse theWriteabstraction introduced in SPARK-33779.Note: we will need separate plans for streaming writes in order to support the required distribution and ordering in SS. This change only migrates to the
Writeabstraction.Why are the changes needed?
These changes prevent exceptions from data sources that implement only the
buildmethod inWriteBuilder.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing tests.