-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-29313][SQL] Fix failure on writing to noop in benchmarks
#25988
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
|
@cloud-fan @brkyvz @rdblue Please, take a look at the PR. |
srowen
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.
Seems OK to me. I wonder why noop fails in any mode? you could argue the mode is just irrelevant. In particular the 'output' never 'exists' in noop mode, conceptually.
|
I agree that the save mode for
spark/sql/core/src/main/scala/org/apache/spark/sql/DataFrameWriter.scala Lines 270 to 287 in c8159c7
|
|
Test build #111643 has finished for PR 25988 at commit
|
|
I'm not sure about this. For me, we need to fix the root cause inside DSv2. |
|
@brkyvz and @cloud-fan . Is this change intentional? |
|
we are going to support all save modes in |
|
Thanks. Then, I'll merge this. This will unblock the benchmarks. |
|
Merged to master. |
|
Thank you, @MaxGekk , @cloud-fan , @srowen . |
|
Sorry I'm just now getting to look at this. @cloud-fan, any idea why v2 was used here for these sources? Built-in v2 implementations should be disabled, so it is concerning that they are inadvertently used, right? |
|
we only disable file source v2, but noop souce is not file source. |
What changes were proposed in this pull request?
In the PR, I propose to specify the save mode explicitly while writing to the
noopdatasource in benchmarks. I setOverwritemode in the following benchmarks:Why are the changes needed?
Otherwise writing to
noopfails with:most likely due to #25876
Does this PR introduce any user-facing change?
No
How was this patch tested?
I generated results of
ExtractBenchmarkvia the command: