Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ class JdbcRelationProvider extends CreatableRelationProvider
if (tableExists) {
mode match {
case SaveMode.Overwrite =>
if (options.isTruncate && isCascadingTruncateTable(options.url) == Some(false)) {

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.

We still need to respect it, but we can enhance PostgresDialect to support the JDBC option truncate

if (options.isTruncate) {
// In this case, we should truncate table and then load.
truncateTable(conn, options.table)
truncateTable(conn, options)
val tableSchema = JdbcUtils.getSchemaOption(conn, options)
saveTable(df, tableSchema, isCaseSensitive, options)
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,20 +96,18 @@ object JdbcUtils extends Logging {
}

/**
* Truncates a table from the JDBC database.
* Truncates a table from the JDBC database without side effects.
*/
def truncateTable(conn: Connection, table: String): Unit = {
def truncateTable(conn: Connection, options: JDBCOptions): Unit = {
val dialect = JdbcDialects.get(options.url)
val statement = conn.createStatement
try {
statement.executeUpdate(s"TRUNCATE TABLE $table")
statement.executeUpdate(dialect.getTruncateQuery(options.table))
} finally {
statement.close()
}
}

def isCascadingTruncateTable(url: String): Option[Boolean] = {
JdbcDialects.get(url).isCascadingTruncateTable()
}

/**
* Returns an Insert SQL statement for inserting a row into the target table via JDBC conn.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import org.apache.spark.sql.types.{DataType, MetadataBuilder}
/**
* AggregatedDialect can unify multiple dialects into one virtual Dialect.
* Dialects are tried in order, and the first dialect that does not return a
* neutral element will will.
* neutral element will win.
*
* @param dialects List of dialects.
*/
Expand Down Expand Up @@ -54,13 +54,7 @@ private class AggregatedDialect(dialects: List[JdbcDialect]) extends JdbcDialect
dialects.head.getSchemaQuery(table)
}

override def isCascadingTruncateTable(): Option[Boolean] = {
// If any dialect claims cascading truncate, this dialect is also cascading truncate.
// Otherwise, if any dialect has unknown cascading truncate, this dialect is also unknown.
dialects.flatMap(_.isCascadingTruncateTable()).reduceOption(_ || _) match {
case Some(true) => Some(true)
case _ if dialects.exists(_.isCascadingTruncateTable().isEmpty) => None
case _ => Some(false)
}
override def getTruncateQuery(table: String): String = {
dialects.head.getTruncateQuery(table)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -48,5 +48,7 @@ private object DB2Dialect extends JdbcDialect {
case _ => None
}

override def isCascadingTruncateTable(): Option[Boolean] = Some(false)
override def getTruncateQuery(table: String): String = {
s"TRUNCATE $table"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why override the default here?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(And all others that are not Postgres?)

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.

The JdbcDialect object now no longer provides a default implementation for getTruncateQuery (as per @dongjoon-hyun 's suggestion). The reason is that this explicitly forces dialects to implement a truncate operation without side effects.

One thing I don't like about this solution is the code duplication across dialects. But it does prevent dialects from inheriting default behaviour and (accidentally) truncating with side effects.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Gotcha. It is also backward incompatible. Ie if someone implemented his/her own dialect one needs to update it (but removing the isCasc...) does this already so I assume that is ok

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,8 @@ private object DerbyDialect extends JdbcDialect {
Option(JdbcType("DECIMAL(31,5)", java.sql.Types.DECIMAL))
case _ => None
}

override def getTruncateQuery(table: String): String = {
s"TRUNCATE $table"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,16 @@ abstract class JdbcDialect extends Serializable {
s"SELECT * FROM $table WHERE 1=0"
}

/**
* The SQL query that should be used to truncate a table. Dialects can override this method to
* return a query that is suitable for a particular database. For PostgreSQL, for instance,
* a different query is used to prevent "TRUNCATE" affecting other tables.
* @param table The name of the table.
* @return The SQL query to use for truncating a table
*/
@Since("2.3.0")

@dongjoon-hyun dongjoon-hyun Dec 7, 2017

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.

@danielvdende .
As we see here, this is a new feature. It seems that we had better a new JIRA issue as a New Feature with a title like Add getTruncateQuery to JdbcDialect.

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.

So create a new "improvement" JIRA and change the ticket number in this PR? (just checking I understand you correctly)

@dongjoon-hyun dongjoon-hyun Dec 7, 2017

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.

Yep. And, we need to fix the following in the PR description, too.

The PostgresDialect indicates that cascade is true by default for Postgres.
This is not the case. This patch fixes this.

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

def getTruncateQuery(table: String): String

/**
* Override connection specific properties to run before a select is made. This is in place to
* allow dialects that need special treatment to optimize behavior.
Expand Down Expand Up @@ -147,14 +157,6 @@ abstract class JdbcDialect extends Serializable {
case arrayValue: Array[Any] => arrayValue.map(compileValue).mkString(", ")
case _ => value
}

/**
* Return Some[true] iff `TRUNCATE TABLE` causes cascading default.
* Some[true] : TRUNCATE TABLE causes cascading.
* Some[false] : TRUNCATE TABLE does not cause cascading.
* None: The behavior of TRUNCATE TABLE is unknown (default).
*/
def isCascadingTruncateTable(): Option[Boolean] = None

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.

We can't drop this, because this is a public API.

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.

As a middle ground, could it be marked deprecated? Seeing as we don't really need it anymore...

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.

Although, custom third party dialects may want it.

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,7 @@ private object MsSqlServerDialect extends JdbcDialect {
case _ => None
}

override def isCascadingTruncateTable(): Option[Boolean] = Some(false)
override def getTruncateQuery(table: String): String = {
s"TRUNCATE $table"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,5 +45,7 @@ private case object MySQLDialect extends JdbcDialect {
s"SELECT 1 FROM $table LIMIT 1"
}

override def isCascadingTruncateTable(): Option[Boolean] = Some(false)
override def getTruncateQuery(table: String): String = {
s"TRUNCATE $table"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -83,5 +83,7 @@ private case object OracleDialect extends JdbcDialect {
case _ => value
}

override def isCascadingTruncateTable(): Option[Boolean] = Some(false)
override def getTruncateQuery(table: String): String = {
s"TRUNCATE $table"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,17 @@ private object PostgresDialect extends JdbcDialect {
s"SELECT 1 FROM $table LIMIT 1"
}

/**
* The SQL query used to truncate a table. For Postgres, the default behaviour is to
* also truncate any descendant tables. As this is a (possibly unwanted) side-effect,
* the Postgres dialect adds 'ONLY' to truncate only the table in question
* @param table The name of the table.
* @return The SQL query to use for truncating a table
*/
override def getTruncateQuery(table: String): String = {
s"TRUNCATE ONLY $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.

Could you add a function comment about why we use TRUNCATE ONLY here.

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.

Also, please add the similar explanation to JdbcDialect.scala, too.

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.

yes will do

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

}

override def beforeFetch(connection: Connection, properties: Map[String, String]): Unit = {
super.beforeFetch(connection, properties)

Expand All @@ -97,8 +108,5 @@ private object PostgresDialect extends JdbcDialect {
if (properties.getOrElse(JDBCOptions.JDBC_BATCH_FETCH_SIZE, "0").toInt > 0) {
connection.setAutoCommit(false)
}

}

override def isCascadingTruncateTable(): Option[Boolean] = Some(true)
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,8 @@ private case object TeradataDialect extends JdbcDialect {
case BooleanType => Option(JdbcType("CHAR(1)", java.sql.Types.CHAR))
case _ => None
}

override def getTruncateQuery(table: String): String = {
s"TRUNCATE $table"
}
}
46 changes: 19 additions & 27 deletions sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -752,43 +752,19 @@ class JDBCSuite extends SparkFunSuite
override def getSchemaQuery(table: String): String = {
s"My $table Schema"
}
override def isCascadingTruncateTable(): Option[Boolean] = Some(true)
override def getTruncateQuery(table: String): String = {
s"TRUNCATE $table"
}
}, testH2Dialect))
assert(agg.canHandle("jdbc:h2:xxx"))
assert(!agg.canHandle("jdbc:h2"))
assert(agg.getCatalystType(0, "", 1, null) === Some(LongType))
assert(agg.getCatalystType(1, "", 1, null) === Some(StringType))
assert(agg.isCascadingTruncateTable() === Some(true))
assert(agg.quoteIdentifier ("Dummy") === "My Dummy quoteIdentifier")
assert(agg.getTableExistsQuery ("Dummy") === "My Dummy Table")
assert(agg.getSchemaQuery ("Dummy") === "My Dummy Schema")
}

test("Aggregated dialects: isCascadingTruncateTable") {
def genDialect(cascadingTruncateTable: Option[Boolean]): JdbcDialect = new JdbcDialect {
override def canHandle(url: String): Boolean = true
override def getCatalystType(
sqlType: Int,
typeName: String,
size: Int,
md: MetadataBuilder): Option[DataType] = None
override def isCascadingTruncateTable(): Option[Boolean] = cascadingTruncateTable
}

def testDialects(cascadings: List[Option[Boolean]], expected: Option[Boolean]): Unit = {
val dialects = cascadings.map(genDialect(_))
val agg = new AggregatedDialect(dialects)
assert(agg.isCascadingTruncateTable() === expected)
}

testDialects(List(Some(true), Some(false), None), Some(true))
testDialects(List(Some(true), Some(true), None), Some(true))
testDialects(List(Some(false), Some(false), None), None)
testDialects(List(Some(true), Some(true)), Some(true))
testDialects(List(Some(false), Some(false)), Some(false))
testDialects(List(None, None), None)
}

test("DB2Dialect type mapping") {
val db2Dialect = JdbcDialects.get("jdbc:db2://127.0.0.1/db")
assert(db2Dialect.getJDBCType(StringType).map(_.databaseTypeDefinition).get == "CLOB")
Expand Down Expand Up @@ -854,6 +830,22 @@ class JDBCSuite extends SparkFunSuite
assert(derby.getTableExistsQuery(table) == defaultQuery)
}

test("truncate table query by jdbc dialect") {
val MySQL = JdbcDialects.get("jdbc:mysql://127.0.0.1/db")
val Postgres = JdbcDialects.get("jdbc:postgresql://127.0.0.1/db")
val db2 = JdbcDialects.get("jdbc:db2://127.0.0.1/db")
val h2 = JdbcDialects.get(url)
val derby = JdbcDialects.get("jdbc:derby:db")
val table = "weblogs"
val defaultQuery = s"TRUNCATE $table"
val postgresQuery = s"TRUNCATE ONLY $table"
assert(MySQL.getTableExistsQuery(table) == defaultQuery)
assert(Postgres.getTableExistsQuery(table) == postgresQuery)
assert(db2.getTableExistsQuery(table) == defaultQuery)
assert(h2.getTableExistsQuery(table) == defaultQuery)
assert(derby.getTableExistsQuery(table) == defaultQuery)
}

test("Test DataFrame.where for Date and Timestamp") {
// Regression test for bug SPARK-11788
val timestamp = java.sql.Timestamp.valueOf("2001-02-20 11:22:33.543543");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ class JDBCWriteSuite extends SharedSQLContext with BeforeAndAfter {

val testH2Dialect = new JdbcDialect {
override def canHandle(url: String) : Boolean = url.startsWith("jdbc:h2")
override def isCascadingTruncateTable(): Option[Boolean] = Some(false)
}

before {
Expand Down