[SPARK-16401] [SQL] Data Source API: Enable Extending RelationProvider and CreatableRelationProvider without Extending SchemaRelationProvider by gatorsmile · Pull Request #14075 · apache/spark

gatorsmile · 2016-07-06T19:56:07Z

What changes were proposed in this pull request?

When users try to implement a data source API with extending only RelationProvider and CreatableRelationProvider, they will hit an error when resolving the relation.

spark.read
.format("org.apache.spark.sql.test.DefaultSourceWithoutUserSpecifiedSchema")
.load()
.write.
format("org.apache.spark.sql.test.DefaultSourceWithoutUserSpecifiedSchema")
.save()

The error they hit is like

org.apache.spark.sql.test.DefaultSourceWithoutUserSpecifiedSchema does not allow user-specified schemas.;
org.apache.spark.sql.AnalysisException: org.apache.spark.sql.test.DefaultSourceWithoutUserSpecifiedSchema does not allow user-specified schemas.;
at org.apache.spark.sql.execution.datasources.DataSource.resolveRelation(DataSource.scala:319)
at org.apache.spark.sql.execution.datasources.DataSource.write(DataSource.scala:494)
at org.apache.spark.sql.DataFrameWriter.save(DataFrameWriter.scala:211)

Actually, the bug fix is simple. DataSource.createRelation(sparkSession.sqlContext, mode, options, data) already returns a BaseRelation. We should not assign schema to userSpecifiedSchema. That schema assignment only makes sense for the data sources that extend FileFormat.

How was this patch tested?

Added a test case.

gatorsmile · 2016-07-06T19:58:50Z

@rxin @marmbrus @yhuai Not sure whether we should backport to 2.0. Personally, this sounds a critical bug to me.

rxin · 2016-07-06T20:16:43Z

Is this a regression?

gatorsmile · 2016-07-06T21:29:53Z

Will make a try tonight. My environment for 1.6 is at home.

SparkQA · 2016-07-06T21:51:13Z

Test build #61864 has finished for PR 14075 at commit dd644f8.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

JustinPihony · 2016-07-07T04:14:43Z

@rxin It does look like this might have been a regression introduced via the initial creation of DataSource from ResolvedDataSource. Although, @marmbrus could speak to it better as he was the one who wrote that code. Maybe there was a reason?

gatorsmile · 2016-07-07T04:33:26Z

@rxin This is a regression. I did try it in Spark 1.6. It works well. I think we need to fix it in Spark 2.0

Thanks!

rxin · 2016-07-07T05:44:00Z

Thanks - then we should merge this in 2.0.

gatorsmile · 2016-07-07T05:49:09Z

Sure, let me know whether I need to submit another PR for backporting to 2.0. Thanks!

rxin · 2016-07-08T05:42:26Z

cc @cloud-fan and @liancheng for review.

cloud-fan · 2016-07-08T08:53:44Z

+ sqlContext: SQLContext,
+ parameters: Map[String, String]): BaseRelation = {
+ LastOptions.parameters = parameters
+ LastOptions.schema = None


Why assign values here? We don't check them in the test right?

That is from the another default source. It is useless in this test case. Let me clean them. Thanks!

cloud-fan · 2016-07-08T08:58:53Z

Nice catch! The fix LGTM.

cloud-fan · 2016-07-08T16:58:40Z

+ override def createRelation(
+ sqlContext: SQLContext,
+ parameters: Map[String, String]): BaseRelation = {
+ LastOptions.parameters = parameters


hmmm, is it a convention to assign parameters to LastOptions? I think the test can work without it.

Yeah, sure. Let me remove all these things. Thanks!

SparkQA · 2016-07-08T17:29:08Z

Test build #61984 has finished for PR 14075 at commit 405d5d2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-07-08T18:55:13Z

Test build #61990 has finished for PR 14075 at commit fb8fd3a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

… and CreatableRelationProvider without Extending SchemaRelationProvider #### What changes were proposed in this pull request? When users try to implement a data source API with extending only `RelationProvider` and `CreatableRelationProvider`, they will hit an error when resolving the relation. ```Scala spark.read .format("org.apache.spark.sql.test.DefaultSourceWithoutUserSpecifiedSchema") .load() .write. format("org.apache.spark.sql.test.DefaultSourceWithoutUserSpecifiedSchema") .save() ``` The error they hit is like ``` org.apache.spark.sql.test.DefaultSourceWithoutUserSpecifiedSchema does not allow user-specified schemas.; org.apache.spark.sql.AnalysisException: org.apache.spark.sql.test.DefaultSourceWithoutUserSpecifiedSchema does not allow user-specified schemas.; at org.apache.spark.sql.execution.datasources.DataSource.resolveRelation(DataSource.scala:319) at org.apache.spark.sql.execution.datasources.DataSource.write(DataSource.scala:494) at org.apache.spark.sql.DataFrameWriter.save(DataFrameWriter.scala:211) ``` Actually, the bug fix is simple. [`DataSource.createRelation(sparkSession.sqlContext, mode, options, data)`](https://github.com/gatorsmile/spark/blob/dd644f8117e889cebd6caca58702a7c7e3d88bef/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSource.scala#L429) already returns a BaseRelation. We should not assign schema to `userSpecifiedSchema`. That schema assignment only makes sense for the data sources that extend `FileFormat`. #### How was this patch tested? Added a test case. Author: gatorsmile <gatorsmile@gmail.com> Closes#14075 from gatorsmile/dataSource. (cherry picked from commit 7374e51) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2016-07-09T12:40:02Z

thanks, merging to master and 2.0!

fix
dd644f8

gatorsmile changed the title ~~Data Source API: Extend RelationProvider and CreatableRelationProvider without Extending SchemaRelationProvider~~[SPARK-16401] [SQL] Data Source API: Extend RelationProvider and CreatableRelationProvider without Extending SchemaRelationProviderJul 6, 2016

gatorsmile mentioned this pull request Jul 7, 2016
[SPARK-16402] [SQL] JDBC Source: Implement save API of DataFrameWriter #14077
Closed

cloud-fan reviewed Jul 8, 2016
View reviewed changes

address comments.
405d5d2

cloud-fan reviewed Jul 8, 2016
View reviewed changes

address comments.
fb8fd3a

asfgit closed this in 7374e51Jul 9, 2016

gatorsmile mentioned this pull request Sep 1, 2016
[SPARK-14525][SQL] Make DataFrameWrite.save work for jdbc #12601
Closed

Uh oh!

Conversation

gatorsmile commented Jul 6, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

gatorsmile commented Jul 6, 2016

Uh oh!

rxin commented Jul 6, 2016

Uh oh!

gatorsmile commented Jul 6, 2016

Uh oh!

SparkQA commented Jul 6, 2016

Uh oh!

JustinPihony commented Jul 7, 2016

Uh oh!

gatorsmile commented Jul 7, 2016

Uh oh!

rxin commented Jul 7, 2016

Uh oh!

gatorsmile commented Jul 7, 2016

Uh oh!

rxin commented Jul 8, 2016

Uh oh!

cloud-fanJul 8, 2016

Choose a reason for hiding this comment

Uh oh!

gatorsmileJul 8, 2016

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jul 8, 2016

Uh oh!

cloud-fanJul 8, 2016

Choose a reason for hiding this comment

Uh oh!

gatorsmileJul 8, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 8, 2016

Uh oh!

SparkQA commented Jul 8, 2016

Uh oh!

cloud-fan commented Jul 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gatorsmile commented Jul 6, 2016•
edited
Loading