[SPARK-22753][SQL] Get rid of dataSource.writeAndRead #19941
Conversation
|
Test build #84707 has finished for PR 19941 at commit
|
|
retest this please |
|
Test build #84730 has finished for PR 19941 at commit
|
|
Maybe something wrong with the Jenkins env? I found the build from 84707 to 84733 failed in same reason: |
|
|
||
| providingClass.newInstance() match { | ||
| case dataSource: CreatableRelationProvider => | ||
| case dataSource: RelationProvider => |
There was a problem hiding this comment.
To the reviewers, current implementation here made datasource should both inherit RelationProvider and CreatableRelationProvider(like the UT), because here just want get the relation by using RelationProvider.createRelation but not save a DF to a destination by using CreatableRelationProvider.createRelation. I need more advise to do better.
There was a problem hiding this comment.
They are different public traits. We are unable to change this.
There was a problem hiding this comment.
If here use CreatableRelationProvider.createRelation, will it write to destination one more time?
There was a problem hiding this comment.
Yes, it will write to the destination, but InsertIntoDataSourceDirCommand only supports FileFormat
|
retest this please |
|
Test build #84754 has finished for PR 19941 at commit
|
|
retest this please |
|
Test build #84759 has finished for PR 19941 at commit
|
| val saveMode = if (overwrite) SaveMode.Overwrite else SaveMode.ErrorIfExists | ||
| try { | ||
| sparkSession.sessionState.executePlan(dataSource.planForWriting(saveMode, query)) | ||
| dataSource.writeAndRead(saveMode, query) |
There was a problem hiding this comment.
How about changing it to
sparkSession.sessionState.executePlan(dataSource.planForWriting(saveMode, query)).toRddThere was a problem hiding this comment.
I implemented like this at first, but after checking this patch (https://github.com/apache/spark/pull/18064/files), I changed to current implementation, is the wrapping of execution id unnecessary here?
There was a problem hiding this comment.
I think it is not needed. You can try it.
There was a problem hiding this comment.
You're right, test the query "INSERT OVERWRITE DIRECTORY '/home/liyuanjian/tmp' USING json SELECT 1 AS a, 'c' as b;".
There was a problem hiding this comment.
Could we revert all the changes except this?
There was a problem hiding this comment.
Revert done.
Sorry, maybe I misunderstand your words of 'get rid of dataSource.writeAndRead'. Like you and Wenchen's discussion in #16481, shouldn't we make writeAndRead just return a BaseRelation without write to the destination? Thank you for your patient reply.
|
Test build #84916 has finished for PR 19941 at commit
|
|
LGTM pending Jenkins |
|
Test build #84939 has finished for PR 19941 at commit
|
|
Thanks! Merged to master. |
3 participants
What changes were proposed in this pull request?
As the discussion in #16481 and #18975 (comment)
Currently the BaseRelation returned by
dataSource.writeAndReadonly used inCreateDataSourceTableAsSelect, planForWriting and writeAndRead has some common code paths.In this patch I removed the writeAndRead function and added the getRelation function which only use in
CreateDataSourceTableAsSelectCommandwhile saving data to non-existing table.How was this patch tested?
Existing UT