Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
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 @@ -38,6 +38,9 @@ private[sql] case class JDBCPartitioningInfo(
numPartitions: Int)

private[sql] object JDBCRelation extends Logging {

import scala.collection.JavaConverters._

/**
* Given a partitioning schematic (a column of integral type, a number of
* partitions, and upper and lower bounds on the column's value), generate
Expand Down Expand Up @@ -99,6 +102,16 @@ private[sql] object JDBCRelation extends Logging {
}
ans.toArray
}

def getEffectiveProperties(
Copy link
Contributor

Choose a reason for hiding this comment

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

this indentation is still a little funky, see https://cwiki.apache.org/confluence/display/SPARK/Spark+Code+Style+Guide

Copy link
Member

Choose a reason for hiding this comment

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

@tribbloid I think Holden's comment still stands -- see how other methods wrap args. I also don't think you need to fully-qualify scala.collection.Map here?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on Sean's comments. Also, could you add a one- or two-line comment to explain what's going on here? Maybe give this method Scaladoc?

connectionProperties: Properties,
extraOptions: scala.collection.Map[String, String] = Map()): Properties = {
Copy link
Member

Choose a reason for hiding this comment

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

Still need not be qualified; then I think this need not wrap?

Copy link
Author

Choose a reason for hiding this comment

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

Qualifier removed, but if without wrap the line exceeds 100 characters.

val props = new Properties()
props.putAll(extraOptions.asJava)
// connectionProperties should override settings in extraOptions
props.putAll(connectionProperties)
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this handle inherited properties? E.g. if connectionProperties was defined by inheriting defaults from a parent set of properties, would those inherited defaults be preserved here or would they be lost? This concern existed in the old code as well, but just thought I'd ask since we're using this method in a few places in order to do defensive copying of property objects.

Copy link
Author

Choose a reason for hiding this comment

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

yes they can be preserved by the deep copy, but unfortunately any changes on parent properties AFTER the deep copy won't be preserved in returned value. I expect callers of this function to use its returned value transiently.

props
}
}

private[sql] case class JDBCRelation(
Expand Down Expand Up @@ -127,7 +140,7 @@ private[sql] case class JDBCRelation(
sparkSession.sparkContext,
schema,
url,
properties,
JDBCRelation.getEffectiveProperties(properties),
table,
requiredColumns,
filters,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ object JdbcUtils extends Logging {
throw new IllegalStateException(
s"Did not find registered driver with class $driverClass")
}
driver.connect(url, properties)
driver.connect(url, JDBCRelation.getEffectiveProperties(properties))
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -686,6 +686,13 @@ class JDBCSuite extends SparkFunSuite
Some(DecimalType(DecimalType.MAX_PRECISION, 10)))
}

test("SPARK-10625: JDBC read should allow driver to insert unserializable into properties") {
UnserializableDriverHelper.replaceDriverDuring {
assert(sqlContext.read.jdbc(
urlWithUserAndPass, "TEST.PEOPLE", new Properties).collect().length === 3)
}
}

test("table exists 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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,4 +196,12 @@ 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("SPARK-10625: JDBC write should allow driver to insert unserializable into properties") {
UnserializableDriverHelper.replaceDriverDuring {
sql("INSERT INTO TABLE PEOPLE1 SELECT * FROM PEOPLE")
assert(2 === sqlContext.read.jdbc(url1, "TEST.PEOPLE1", properties).count)
assert(2 === sqlContext.read.jdbc(url1, "TEST.PEOPLE1", properties).collect()(0).length)
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.spark.sql.jdbc

import java.sql.{Connection, DriverManager}
import java.util.Properties
import java.util.logging.Logger

object UnserializableDriverHelper {

import scala.collection.JavaConverters._
Copy link
Member

Choose a reason for hiding this comment

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

This is imported locally in a few places, why? Below you don't import org.h2.Driver though. I'm not worried about changing it though.

Copy link
Author

Choose a reason for hiding this comment

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

Patches should target specific problem and minimize collateral impact. No? Then I won't import utility package in upcoming pull requests.

Copy link
Member

Choose a reason for hiding this comment

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

I meant why is it imported inside the object scope rather than with other imports -- the use of the converters is fine and important. There are some special cases where qualified name or local imports are needed but I think by convention, the strong default is to declare all of them at the top of the compilation unit, especially fairly common ones.

Copy link
Author

Choose a reason for hiding this comment

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

OK, I was under the false impression that import of implicits may cause conflicts. But in a well designed library this rarely happens

Copy link
Member

Choose a reason for hiding this comment

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

That's a reasonable argument, though we have imported this set of implicits widely elsewhere in the code

Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this import to the top; the arguments RE: scope of implicits are different on a case-by-case basis, but JavaConverters is a case that should pretty much always be safe to put in a somewhat global scope.


def replaceDriverDuring[T](f: => T): T = {
object UnserializableH2Driver extends org.h2.Driver {

override def connect(url: String, info: Properties): Connection = {

val result = super.connect(url, info)
info.put("unserializableDriver", this)
result
}

override def getParentLogger: Logger = null
}

val oldDrivers = DriverManager.getDrivers.asScala.toList.filter(_.acceptsURL("jdbc:h2:"))
oldDrivers.foreach(DriverManager.deregisterDriver)
DriverManager.registerDriver(UnserializableH2Driver)

val result = try {
f
} finally {
DriverManager.deregisterDriver(UnserializableH2Driver)
oldDrivers.foreach(DriverManager.registerDriver)
}
result
}
}