Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
6be5046
[SPARK-14525][SQL] Make DataFrameWrite.save work for jdbc
JustinPihony Apr 22, 2016
db639a5
Merge assertion into jdbc
JustinPihony May 19, 2016
69f7c7b
Merge https://github.com/apache/spark into jdbc_reconciliation
JustinPihony Jun 6, 2016
88d181e
[SPARK-14525][SQL] Make jdbc a CreatableRelationProvider for simpler …
JustinPihony Jun 7, 2016
c44271e
[SPARK-14525][SQL] Clean empty space commit
JustinPihony Jun 7, 2016
0a98e45
Merge
JustinPihony Jul 7, 2016
cb9889e
[SPARK-14525][SQL]Address some code reviews
JustinPihony Jul 7, 2016
d18efef
Merge branch 'jdbc_reconciliation' of https://github.com/JustinPihony…
JustinPihony Jul 7, 2016
754b360
Change sys.error to require
JustinPihony Aug 31, 2016
1d0d61c
Remove the last sys.error
JustinPihony Aug 31, 2016
c8e4143
Remove the local import
JustinPihony Sep 1, 2016
f7f2615
Merge
JustinPihony Sep 8, 2016
95431e3
Merge branch 'jdbc_reconciliation' of https://github.com/JustinPihony…
JustinPihony Sep 8, 2016
e8c2d7d
Fix whitespace
JustinPihony Sep 8, 2016
ae6ad8b
Removed SchemaRelationProvider
JustinPihony Sep 8, 2016
c387c17
Add forgotten stashed test
JustinPihony Sep 8, 2016
57ac87e
Address comments
JustinPihony Sep 8, 2016
de53734
Address more comments
JustinPihony Sep 8, 2016
379e00f
More insanity
JustinPihony Sep 8, 2016
ea9d2fe
Analysis Exception
JustinPihony Sep 8, 2016
c686b0e
Simplify require
JustinPihony Sep 8, 2016
7ef7a48
Documentation
JustinPihony Sep 12, 2016
c9dcdc4
Documentation for java and python
JustinPihony Sep 15, 2016
447ab82
Add semicolons to my java
JustinPihony Sep 15, 2016
4a02c82
Add import
JustinPihony Sep 15, 2016
a238156
Fixed up java and tested past it on my end
JustinPihony Sep 15, 2016
06c1cba
R and SQL documentation
JustinPihony Sep 24, 2016
8fb86b4
Move import back
JustinPihony Sep 24, 2016
724bbe2
Address comments
JustinPihony Sep 26, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 5 additions & 40 deletions sql/core/src/main/scala/org/apache/spark/sql/DataFrameWriter.scala
Original file line number Diff line number Diff line change
Expand Up @@ -587,47 +587,12 @@ final class DataFrameWriter private[sql](df: DataFrame) {
*/
def jdbc(url: String, table: String, connectionProperties: Properties): Unit = {
assertNotStreaming("jdbc() can only be called on non-continuous queries")

val props = new Properties()
extraOptions.foreach { case (key, value) =>
props.put(key, value)
}
import scala.collection.JavaConverters._

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: I'd import these with other imports

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I opted to only import them here because it is the only place they are required, so there is no need to drag in the import to the whole class.

@HyukjinKwon HyukjinKwon Sep 1, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(+1 for moving the import up.)

// connectionProperties should override settings in extraOptions
props.putAll(connectionProperties)
val conn = JdbcUtils.createConnectionFactory(url, props)()

try {
var tableExists = JdbcUtils.tableExists(conn, url, table)

if (mode == SaveMode.Ignore && tableExists) {
return
}

if (mode == SaveMode.ErrorIfExists && tableExists) {
sys.error(s"Table $table already exists.")
}

if (mode == SaveMode.Overwrite && tableExists) {
JdbcUtils.dropTable(conn, table)
tableExists = false
}

// Create the table if the table didn't exist.
if (!tableExists) {
val schema = JdbcUtils.schemaString(df, url)
val sql = s"CREATE TABLE $table ($schema)"
val statement = conn.createStatement
try {
statement.executeUpdate(sql)
} finally {
statement.close()
}
}
} finally {
conn.close()
}

JdbcUtils.saveTable(df, url, table, props)
this.extraOptions = this.extraOptions ++ (connectionProperties.asScala)
// explicit url and dbtable should override all
this.extraOptions += ("url" -> url, "dbtable" -> table)
format("jdbc").save

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The omission of parentheses on methods should only be used when the method has no side-effects.

Thus, please change it to save()

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ private[sql] case class JDBCRelation(
url: String,
table: String,
parts: Array[Partition],
properties: Properties = new Properties())(@transient val sparkSession: SparkSession)
properties: Properties = new Properties(),
providedSchemaOption: Option[StructType] = None)(@transient val sparkSession: SparkSession)
extends BaseRelation
with PrunedFilteredScan
with InsertableRelation {
Expand All @@ -96,7 +97,16 @@ private[sql] case class JDBCRelation(

override val needConversion: Boolean = false

override val schema: StructType = JDBCRDD.resolveTable(url, table, properties)
override val schema: StructType = {
val resolvedSchema = JDBCRDD.resolveTable(url, table, properties)
providedSchemaOption match {
case Some(providedSchema) =>
if (providedSchema.sql.toLowerCase == resolvedSchema.sql.toLowerCase) resolvedSchema

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the only area I'm unsure about. I'd like a second opinion on whether this seems ok, or if I need to build something more custom for schema comparison.

@HyukjinKwon HyukjinKwon Jun 12, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess it would make sense if it does not try to apply the resolved schema but just use the specified one when the schema is explicitly set like the other data sources.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can easily do a simpler getOrElse as is done in spark-xml which has more of a benefit of being lazier. But if an error does occur due to a mismatch, then the error is further from the original issue. I'm fine with either scenario, but at least wanted to give the other side for this one. Thoughts?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think JDBCRDD.resolveTable needs another query execution. Although it would be less expensive than inferring schemas in CSV or JSON, it would be still a bit of overhead. I am not 100% about this too. So, I think it might be better to be consistent with the others in this case.

else sys.error(s"User specified schema, $providedSchema, " +
s"does not match the actual schema, $resolvedSchema.")
case None => resolvedSchema
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It seems we might not need the extra brackets here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@HyukjinKwon I thought that since this was over the 100 limit it would be more readable/maintainable in the long run to have brackets. Again, I have no preference and if you feel strongly I will make the change.


// Check if JDBCRDD.compileFilter can accept input filters
override def unhandledFilters(filters: Array[Filter]): Array[Filter] = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,37 +19,105 @@ package org.apache.spark.sql.execution.datasources.jdbc

import java.util.Properties

import org.apache.spark.sql.SQLContext
import org.apache.spark.sql.sources.{BaseRelation, DataSourceRegister, RelationProvider}
import org.apache.spark.sql.{DataFrame, SaveMode, SQLContext}
import org.apache.spark.sql.sources.{BaseRelation, CreatableRelationProvider, DataSourceRegister, RelationProvider, SchemaRelationProvider}
import org.apache.spark.sql.types.StructType

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not used?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not used, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct, this was left from SchemaRelationProvider


class JdbcRelationProvider extends RelationProvider with DataSourceRegister {
class JdbcRelationProvider extends CreatableRelationProvider
with SchemaRelationProvider with RelationProvider with DataSourceRegister {

override def shortName(): String = "jdbc"

/** Returns a new base relation with the given parameters. */
override def createRelation(
sqlContext: SQLContext,
parameters: Map[String, String]): BaseRelation = {
val jdbcOptions = new JDBCOptions(parameters)
if (jdbcOptions.partitionColumn != null
&& (jdbcOptions.lowerBound == null
|| jdbcOptions.upperBound == null
|| jdbcOptions.numPartitions == null)) {
createRelation(sqlContext, parameters, null)
}

/** Returns a new base relation with the given parameters. */
override def createRelation(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this a separate method instead of using an optional arg to try to retain binary compatibility?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, this is to meet the requirements of trait RelationProvider.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any reason why this is removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It was moved into the JDBCOptions as had been previously discussed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

uh, we do not have a test case to cover that. Since you made a change, could you add such a test case? Thanks!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added.

sqlContext: SQLContext,
parameters: Map[String, String],
schema: StructType): BaseRelation = {
val url = parameters.getOrElse("url", sys.error("Option 'url' not specified"))
val table = parameters.getOrElse("dbtable", sys.error("Option 'dbtable' not specified"))
val partitionColumn = parameters.getOrElse("partitionColumn", null)
val lowerBound = parameters.getOrElse("lowerBound", null)
val upperBound = parameters.getOrElse("upperBound", null)
val numPartitions = parameters.getOrElse("numPartitions", null)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is a class for those options, JDBCOptions. It would be nicer if those options are managed in a single place.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@HyukjinKwon Thanks, I did not know about this. Before I push code I was curious why JDBCOptions does not include the partitioning validation? That seems like a point of duplication also.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the validation can be done together in JDBCOptions.


if (partitionColumn != null
&& (lowerBound == null || upperBound == null || numPartitions == null)) {
sys.error("Partitioning incompletely specified")
}

val partitionInfo = if (jdbcOptions.partitionColumn == null) {
null
} else {
val partitionInfo = if (partitionColumn == null) null

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

else {
JDBCPartitioningInfo(
jdbcOptions.partitionColumn,
jdbcOptions.lowerBound.toLong,
jdbcOptions.upperBound.toLong,
jdbcOptions.numPartitions.toInt)
partitionColumn, lowerBound.toLong, upperBound.toLong, numPartitions.toInt)
}
val parts = JDBCRelation.columnPartition(partitionInfo)
val properties = new Properties() // Additional properties that we will pass to getConnection
parameters.foreach(kv => properties.setProperty(kv._1, kv._2))
JDBCRelation(jdbcOptions.url, jdbcOptions.table, parts, properties)(sqlContext.sparkSession)
JDBCRelation(url, table, parts, properties, Option(schema))(sqlContext.sparkSession)
}

/*
* The following structure applies to this code:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what does this table mean? what is CreateTable, saveTable, BaseRelation?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now, at least, three of reviewers are confused of this bit. Do you mind if I submit a PR to clean up this part?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If the table does not exist and the mode is OVERWRITE, we create a table, then insert rows into the table, and finally return a BaseRelation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I also took a look at @gatorsmile 's approach, I think it's easier to understand, why it's rejected? We can also get rid of the return:

if (tableExists) {
  mode match {
    case SaveMode.Ignore =>
    ......
  }
} else {
  ......
}

* | tableExists | !tableExists
*------------------------------------------------------------------------------------
* Ignore | BaseRelation | CreateTable, saveTable, BaseRelation
* ErrorIfExists | ERROR | CreateTable, saveTable, BaseRelation
* Overwrite | DropTable, CreateTable, | CreateTable, saveTable, BaseRelation
* | saveTable, BaseRelation |
* Append | saveTable, BaseRelation | CreateTable, saveTable, BaseRelation
*/
override def createRelation(
sqlContext: SQLContext,
mode: SaveMode,
parameters: Map[String, String],
data: DataFrame): BaseRelation = {
val url = parameters.getOrElse("url",
sys.error("Saving jdbc source requires url to be set." +
" (ie. df.option(\"url\", \"ACTUAL_URL\")"))
val table = parameters.getOrElse("dbtable", parameters.getOrElse("table",
sys.error("Saving jdbc source requires dbtable to be set." +
" (ie. df.option(\"dbtable\", \"ACTUAL_DB_TABLE\")")))

import collection.JavaConverters._

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we maybe move this up too if either is okay?

val props = new Properties()
props.putAll(parameters.asJava)
val conn = JdbcUtils.createConnectionFactory(url, props)()

try {
val tableExists = JdbcUtils.tableExists(conn, url, table)

val (doCreate, doSave) = (mode, tableExists) match {

@HyukjinKwon HyukjinKwon Sep 8, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Initially, I meant to correct this as @gatorsmile did in here. I am not saying this is wrong or inappropriate but just personally I'd prefer this way.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I also prefer to my way, which looks cleaner and easier to understand.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Your way results in the need for a return, which can lead to problems and is generally discouraged. In the current implementation you could just have it do nothing and the next if block will be skipped anyway, but that leaves a lot of room for error in further code changes. Whereas this way is very explicit about the rules and what each combination will yield.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok. I am fine, if the other are ok about it. Let me review your version.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Then would it make sense if we add some comments for each case? In a quick look, it seems really confusing what each case means to me.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did add a comment in the method signature. That and the variable naming conventions should cover this.

case (SaveMode.Ignore, true) => (false, false)
case (SaveMode.ErrorIfExists, true) => sys.error(s"Table $table already exists.")
case (SaveMode.Overwrite, true) =>
JdbcUtils.dropTable(conn, table)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm, I don't think this is the semantics we have elsewhere. It's truncated unless truncating won't work (i.e. different schema)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If you want I can make the change. I was trying to not change behavior as this is the current behavior.

(true, true)
case (SaveMode.Append, true) => (false, true)
case (_, true) => sys.error(s"Unexpected SaveMode, '$mode', for handling existing tables.")
case (_, false) => (true, true)

@HyukjinKwon HyukjinKwon Jun 12, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Personally, I think this combinations of booleans might be a bit confusing. It might be better if they have some variables so that we can understand what each case means.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure I get what you mean, as the booleans map directly to a variable. The only thing I can think of beyond using a var and setting the variables directly (ugly) is to create a JDBCActionDecider to wrap the values and return that, but that ultimately adds another level of indirection that seems unnecessary in this case.
And I do not think that comments should be necessary, but there is also the added benefit of having this broken down in the method comment.

}

if(doCreate) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

add a space after if

val schema = JdbcUtils.schemaString(data, url)
val sql = s"CREATE TABLE $table ($schema)"
val statement = conn.createStatement
try {
statement.executeUpdate(sql)
} finally {
statement.close()
}
}
if(doSave) JdbcUtils.saveTable(data, url, table, props)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The same here. we need one extra space after if

} finally {
conn.close()
}

createRelation(sqlContext, parameters, data.schema)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -151,4 +151,16 @@ class JDBCWriteSuite extends SharedSQLContext with BeforeAndAfter {
assert(2 === spark.read.jdbc(url1, "TEST.PEOPLE1", properties).count)
assert(2 === spark.read.jdbc(url1, "TEST.PEOPLE1", properties).collect()(0).length)
}

test("save works for format(\"jdbc\") if url and dbtable are set") {
val df = sqlContext.createDataFrame(sparkContext.parallelize(arr2x2), schema2)

df.write.format("jdbc")
.options(Map("url" -> url, "dbtable" -> "TEST.BASICCREATETEST"))
.save

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: save -> save()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done


assert(2 === sqlContext.read.jdbc(url, "TEST.BASICCREATETEST", new Properties).count)
assert(
2 === sqlContext.read.jdbc(url, "TEST.BASICCREATETEST", new Properties).collect()(0).length)
}
}