[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-178820992
  
Hi Josh, do you see any problem? The Jenkins should have it tested and its 
clean to be merged be now


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r51035587
  
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala 
---
@@ -469,6 +469,13 @@ class JDBCSuite extends SparkFunSuite with 
BeforeAndAfter with SharedSQLContext
 
assert(derbyDialect.getJDBCType(BooleanType).map(_.databaseTypeDefinition).get 
== "BOOLEAN")
   }
 
+  test("Spark-10625: JDBC read should allow driver to insert 
unserializable into properties") {
--- End diff --

fixed!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r51035933
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -88,7 +101,8 @@ private[sql] case class JDBCRelation(
 
   override val needConversion: Boolean = false
 
-  override val schema: StructType = JDBCRDD.resolveTable(url, table, 
properties)
+  override val schema: StructType =
+JDBCRDD.resolveTable(url, table, 
JDBCRelation.getEffectiveProperties(properties))
--- End diff --

Yes, you are right, I removed the getEffectiveProperties() and all tests 
passed. This may be attributed to changes in getConnectionFactory()


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r51036047
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -108,6 +122,6 @@ private[sql] case class JDBCRelation(
   override def insert(data: DataFrame, overwrite: Boolean): Unit = {
 data.write
   .mode(if (overwrite) SaveMode.Overwrite else SaveMode.Append)
-  .jdbc(url, table, properties)
+  .jdbc(url, table, JDBCRelation.getEffectiveProperties(properties))
--- End diff --

yes, reverted without breaking any test.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r51036461
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -37,6 +37,9 @@ private[sql] case class JDBCPartitioningInfo(
 numPartitions: Int)
 
 private[sql] object JDBCRelation {
+
+  import scala.collection.JavaConverters._
--- End diff --

 import scala.collection.JavaConverters._ moved to the top with other 
non-wildcard imports


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-175818906
  
Yes, all comments resolved.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r51038017
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -75,6 +78,16 @@ private[sql] object JDBCRelation {
 }
 ans.toArray
   }
+
+  def getEffectiveProperties(
+  connectionProperties: Properties,
+  extraOptions: scala.collection.Map[String, String] = Map()): 
Properties = {
+val props = new Properties()
+props.putAll(extraOptions.asJava)
+// connectionProperties should override settings in extraOptions
+props.putAll(connectionProperties)
--- End diff --

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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[JoshRosen]

Github user JoshRosen commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-175203838
  
Hey, there's still a bunch of review comments that I left which haven't 
been acknowledged or addressed! Mind replying to them?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-174732217
  
Good! Boots on the ground.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-174732645
  
Rebased with minimal changes to code style (and reverted whitespace changes 
as well as correcting import order). All tests passed. Let's finish this


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[JoshRosen]

Github user JoshRosen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r50641262
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -37,6 +37,9 @@ private[sql] case class JDBCPartitioningInfo(
 numPartitions: Int)
 
 private[sql] object JDBCRelation {
+
+  import scala.collection.JavaConverters._
--- End diff --

Hey @tribbloid, #10519 has now been merged, so I think things should be in 
a much more testable state now in case you want to revive this PR and try to 
get it merged.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-168621202
  
Hm @tribbloid this now fails a test on the JDBC drivers:

```
[info] Exception encountered when attempting to run a suite with class 
name: org.apache.spark.sql.jdbc.JDBCSuite *** ABORTED *** (84 milliseconds)
[info]   java.sql.SQLException: No suitable driver found for 
jdbc:h2:mem:testdb0
[info]   at java.sql.DriverManager.getConnection(DriverManager.java:596)
[info]   at java.sql.DriverManager.getConnection(DriverManager.java:187)
[info]   at 
org.apache.spark.sql.jdbc.JDBCSuite$$anonfun$43.apply(JDBCSuite.scala:62)
[info]   at 
org.apache.spark.sql.jdbc.JDBCSuite$$anonfun$43.apply(JDBCSuite.scala:53)
[info]   at 
org.scalatest.BeforeAndAfter$class.runTest(BeforeAndAfter.scala:195)
[info]   at org.apache.spark.sql.jdbc.JDBCSuite.runTest(JDBCSuite.scala:36)
[info]   at 
org.scalatest.FunSuiteLike$$anonfun$runTests$1.apply(FunSuiteLike.scala:208)
[info]   at 
org.scalatest.FunSuiteLike$$anonfun$runTests$1.apply(FunSuiteLike.scala:208)
[info]   at 
org.scalatest.SuperEngine$$anonfun$traverseSubNodes$1$1.apply(Engine.scala:413)
[info]   at 
org.scalatest.SuperEngine$$anonfun$traverseSubNodes$1$1.apply(Engine.scala:401)
[info]   at scala.collection.immutable.List.foreach(List.scala:318)
[info]   at org.scalatest.SuperEngine.traverseSubNodes$1(Engine.scala:401)
[info]   at 
org.scalatest.SuperEngine.org$scalatest$SuperEngine$$runTestsInBranch(Engine.scala:396)
[info]   at org.scalatest.SuperEngine.runTestsImpl(Engine.scala:483)
[info]   at 
org.scalatest.FunSuiteLike$class.runTests(FunSuiteLike.scala:208)
[info]   at org.scalatest.FunSuite.runTests(FunSuite.scala:1555)
[info]   at org.scalatest.Suite$class.run(Suite.scala:1424)
[info]   at 
org.scalatest.FunSuite.org$scalatest$FunSuiteLike$$super$run(FunSuite.scala:1555)
[info]   at 
org.scalatest.FunSuiteLike$$anonfun$run$1.apply(FunSuiteLike.scala:212)
[info]   at 
org.scalatest.FunSuiteLike$$anonfun$run$1.apply(FunSuiteLike.scala:212)
[info]   at org.scalatest.SuperEngine.runImpl(Engine.scala:545)
[info]   at org.scalatest.FunSuiteLike$class.run(FunSuiteLike.scala:212)
[info]   at 
org.apache.spark.sql.jdbc.JDBCSuite.org$scalatest$BeforeAndAfter$$super$run(JDBCSuite.scala:36)
[info]   at org.scalatest.BeforeAndAfter$class.run(BeforeAndAfter.scala:241)
[info]   at 
org.apache.spark.sql.jdbc.JDBCSuite.org$scalatest$BeforeAndAfterAll$$super$run(JDBCSuite.scala:36)
[info]   at 
org.scalatest.BeforeAndAfterAll$class.liftedTree1$1(BeforeAndAfterAll.scala:257)
[info]   at 
org.scalatest.BeforeAndAfterAll$class.run(BeforeAndAfterAll.scala:256)
[info]   at org.apache.spark.sql.jdbc.JDBCSuite.run(JDBCSuite.scala:36)
[info]   at 
org.scalatest.tools.Framework.org$scalatest$tools$Framework$$runSuite(Framework.scala:462)
[info]   at 
org.scalatest.tools.Framework$ScalaTestTask.execute(Framework.scala:671)
[info]   at sbt.ForkMain$Run$2.call(ForkMain.java:296)
[info]   at sbt.ForkMain$Run$2.call(ForkMain.java:286)
[info]   at java.util.concurrent.FutureTask.run(FutureTask.java:262)
[info]   at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
[info]   at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
[info]   at java.lang.Thread.run(Thread.java:745)
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[JoshRosen]

Github user JoshRosen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48715771
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/UnserializableDriverHelper.scala
 ---
@@ -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.{DriverManager, Connection}
+import java.util.Properties
+import java.util.logging.Logger
+
+object UnserializableDriverHelper {
+
+  import scala.collection.JavaConverters._
+
+  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.filter(_.acceptsURL("jdbc:h2:"))
--- End diff --

I'd add an explicit `.toSeq` at the end; what's probably happening is that 
this is becoming a one-shot scala iterator, which is causing the finally block 
to not re-register the old drivers.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[JoshRosen]

Github user JoshRosen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48715834
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -37,6 +37,9 @@ private[sql] case class JDBCPartitioningInfo(
 numPartitions: Int)
 
 private[sql] object JDBCRelation {
+
+  import scala.collection.JavaConverters._
--- End diff --

Same here; move this to the top with the other Scala import.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[JoshRosen]

Github user JoshRosen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48715673
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/UnserializableDriverHelper.scala
 ---
@@ -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.{DriverManager, Connection}
+import java.util.Properties
+import java.util.logging.Logger
+
+object UnserializableDriverHelper {
+
+  import scala.collection.JavaConverters._
--- End diff --

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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[JoshRosen]

Github user JoshRosen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48715790
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/UnserializableDriverHelper.scala
 ---
@@ -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.{DriverManager, Connection}
+import java.util.Properties
+import java.util.logging.Logger
+
+object UnserializableDriverHelper {
+
+  import scala.collection.JavaConverters._
+
+  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.filter(_.acceptsURL("jdbc:h2:"))
+oldDrivers.foreach(DriverManager.deregisterDriver)
+DriverManager.registerDriver(UnserializableH2Driver)
--- End diff --

Should probably be inside of the `try` block, no?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[JoshRosen]

Github user JoshRosen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48714853
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/DriverRegistry.scala
 ---
@@ -57,4 +57,3 @@ object DriverRegistry extends Logging {
 case driver => driver.getClass.getCanonicalName
   }
 }
-
--- End diff --

Can you please minimize unrelated whitespace changes?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[JoshRosen]

Github user JoshRosen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48715367
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -108,6 +122,6 @@ private[sql] case class JDBCRelation(
   override def insert(data: DataFrame, overwrite: Boolean): Unit = {
 data.write
   .mode(if (overwrite) SaveMode.Overwrite else SaveMode.Append)
-  .jdbc(url, table, properties)
+  .jdbc(url, table, JDBCRelation.getEffectiveProperties(properties))
--- End diff --

This call is effectively just cloning the properties, but we're already 
doing the clone inside of `.jdbc()` itself, so we don't need this.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[JoshRosen]

Github user JoshRosen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48714897
  
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala 
---
@@ -469,6 +469,13 @@ class JDBCSuite extends SparkFunSuite with 
BeforeAndAfter with SharedSQLContext
 
assert(derbyDialect.getJDBCType(BooleanType).map(_.databaseTypeDefinition).get 
== "BOOLEAN")
   }
 
+  test("Spark-10625: JDBC read should allow driver to insert 
unserializable into properties") {
--- End diff --

Spark should be `SPARK` here, for consistency with other JIRA references.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[JoshRosen]

Github user JoshRosen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48715260
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -88,7 +101,8 @@ private[sql] case class JDBCRelation(
 
   override val needConversion: Boolean = false
 
-  override val schema: StructType = JDBCRDD.resolveTable(url, table, 
properties)
+  override val schema: StructType =
+JDBCRDD.resolveTable(url, table, 
JDBCRelation.getEffectiveProperties(properties))
--- End diff --

The non-serializability doesn't come into play here since this runs 
entirely on the driver, right? Or can this method wind up being called on 
executors?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[JoshRosen]

Github user JoshRosen commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-168622897
  
I think this PR would be much easier to test after #10519 is fixed, in case 
any reviewers here would also like to look at my PR.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[JoshRosen]

Github user JoshRosen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48715166
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -75,6 +76,19 @@ private[sql] object JDBCRelation {
 }
 ans.toArray
   }
+
+  def getEffectiveProperties(
--- End diff --

+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?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[JoshRosen]

Github user JoshRosen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48715580
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -75,6 +78,16 @@ private[sql] object JDBCRelation {
 }
 ans.toArray
   }
+
+  def getEffectiveProperties(
+  connectionProperties: Properties,
+  extraOptions: scala.collection.Map[String, String] = Map()): 
Properties = {
+val props = new Properties()
+props.putAll(extraOptions.asJava)
+// connectionProperties should override settings in extraOptions
+props.putAll(connectionProperties)
--- End diff --

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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48789707
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -37,6 +37,9 @@ private[sql] case class JDBCPartitioningInfo(
 numPartitions: Int)
 
 private[sql] object JDBCRelation {
+
+  import scala.collection.JavaConverters._
--- End diff --

OK, I'll merge after #10519 is closed, right now I see some changes that 
pretty much did what I did


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-168491449
  
**[Test build #2301 has 
started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2301/consoleFull)**
 for PR 8785 at commit 
[`6a240b1`](https://github.com/apache/spark/commit/6a240b12ec994ddb989a22b46b5b90a3e9fdc014).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-168493283
  
**[Test build #2301 has 
finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2301/consoleFull)**
 for PR 8785 at commit 
[`6a240b1`](https://github.com/apache/spark/commit/6a240b12ec994ddb989a22b46b5b90a3e9fdc014).
 * This patch **fails Spark unit tests**.
 * This patch merges cleanly.
 * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-168493407
  
**[Test build #2304 has 
started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2304/consoleFull)**
 for PR 8785 at commit 
[`6a240b1`](https://github.com/apache/spark/commit/6a240b12ec994ddb989a22b46b5b90a3e9fdc014).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-168494148
  
**[Test build #2304 has 
finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2304/consoleFull)**
 for PR 8785 at commit 
[`6a240b1`](https://github.com/apache/spark/commit/6a240b12ec994ddb989a22b46b5b90a3e9fdc014).
 * This patch **fails Spark unit tests**.
 * This patch merges cleanly.
 * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-168313849
  
Weird, OK. This LGTM.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48678521
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/UnserializableDriverHelper.scala
 ---
@@ -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.{DriverManager, Connection}
+import java.util.Properties
+import java.util.logging.Logger
+
+object UnserializableDriverHelper {
+
+  import scala.collection.JavaConverters._
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-168148505
  
**[Test build #2280 has 
finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2280/consoleFull)**
 for PR 8785 at commit 
[`f6c0fde`](https://github.com/apache/spark/commit/f6c0fdeb839b43d9009d61187cb052f274ab11d2).
 * This patch **fails to build**.
 * This patch merges cleanly.
 * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-168211061
  
Looks like the removed qualifier of Map in JDBCRelation has caused build to 
fail (Map is cast to immutable.Map). I'm adding back the qualifier 
scala.collection.Map now


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-168148300
  
**[Test build #2280 has 
started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2280/consoleFull)**
 for PR 8785 at commit 
[`f6c0fde`](https://github.com/apache/spark/commit/f6c0fdeb839b43d9009d61187cb052f274ab11d2).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48636488
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/UnserializableDriverHelper.scala
 ---
@@ -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.{DriverManager, Connection}
+import java.util.Properties
+import java.util.logging.Logger
+
+object UnserializableDriverHelper {
+
+  import scala.collection.JavaConverters._
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-168087181
  
I'm so done with IntelliJ autocorrection. But all fixed, thanks a lot!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-167978930
  
@tribbloid still having some issue with the style checker:
```
[error] 
/home/jenkins/workspace/NewSparkPullRequestBuilder/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala:42:0:
 Whitespace at end of line
```
It's minor but does need to pass


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-167539588
  
**[Test build #2258 has 
started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2258/consoleFull)**
 for PR 8785 at commit 
[`6af8fd8`](https://github.com/apache/spark/commit/6af8fd8b824f5f343a01868560b74a1f55acd02f).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-167539724
  
**[Test build #2258 has 
finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2258/consoleFull)**
 for PR 8785 at commit 
[`6af8fd8`](https://github.com/apache/spark/commit/6af8fd8b824f5f343a01868560b74a1f55acd02f).
 * This patch **fails Scala style tests**.
 * This patch merges cleanly.
 * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48472434
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/UnserializableDriverHelper.scala
 ---
@@ -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.{DriverManager, Connection}
+import java.util.Properties
+import java.util.logging.Logger
+
+object UnserializableDriverHelper {
+
+  import scala.collection.JavaConverters._
--- End diff --

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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48492460
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/UnserializableDriverHelper.scala
 ---
@@ -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.{DriverManager, Connection}
+import java.util.Properties
+import java.util.logging.Logger
+
+object UnserializableDriverHelper {
+
+  import scala.collection.JavaConverters._
--- End diff --

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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48489063
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/UnserializableDriverHelper.scala
 ---
@@ -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.{DriverManager, Connection}
+import java.util.Properties
+import java.util.logging.Logger
+
+object UnserializableDriverHelper {
+
+  import scala.collection.JavaConverters._
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48460773
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/UnserializableDriverHelper.scala
 ---
@@ -0,0 +1,54 @@
+/*
+ * 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.{DriverManager, Connection}
+import java.util.Properties
+import java.util.logging.Logger
+
+object UnserializableDriverHelper {
+
+  def replaceDriverDuring[T](f: => T): T = {
+import scala.collection.JavaConverters._
+
+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.filter(_.acceptsURL("jdbc:h2:"))
+oldDrivers.foreach{ DriverManager.deregisterDriver }
--- End diff --

Yes, both foreach are made one line


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48460762
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/UnserializableDriverHelper.scala
 ---
@@ -0,0 +1,54 @@
+/*
+ * 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.{DriverManager, Connection}
+import java.util.Properties
+import java.util.logging.Logger
+
+object UnserializableDriverHelper {
+
+  def replaceDriverDuring[T](f: => T): T = {
+import scala.collection.JavaConverters._
--- End diff --

Yes, promote to under object


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid closed the pull request at:

https://github.com/apache/spark/pull/8785


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-167461046
  
All issues should be fixed by now, submitting final pull request


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

GitHub user tribbloid reopened a pull request:

https://github.com/apache/spark/pull/8785

[Spark-10625] [SQL] Spark SQL JDBC read/write is unable to handle JDBC 
Drivers that adds unserializable objects into connection properties

Connection properties are now deep copied before they are used by JDBC 
Drivers, this solvs all problems in unit tests

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/Schedule1/spark SPARK-10625

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/8785.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #8785


commit 267afca7caa3e06a0fdac123f8a50033a726c564
Author: tribbloid 
Date:   2015-09-15T23:02:57Z

test case demonstrating SPARK-10625:
Spark SQL JDBC read/write is unable to handle JDBC Drivers that adds 
unserializable objects into connection properties

add one more unit test
fix JDBCRelation & DataFrameWriter to pass all tests

revise scala style

put driver replacement code into a shared function

fix styling

upgrade to master and resolve all related issues

commit cd641c01260eeabe910d819fffd9193461b37946
Author: tribbloid 
Date:   2015-12-10T01:15:18Z

remove the useless second deep copy of properties
rename test names to be more explicit

commit d33dc9c773e01695b3098f5a69733eacc2a88456
Author: tribbloid 
Date:   2015-12-16T02:39:37Z

minor refactoring based on Sean's suggestion

commit 1dcede3d98575152dda1c46b263fc4e2361f0766
Author: tribbloid 
Date:   2015-12-16T15:37:04Z

move JavaConverters import to under object

commit d2723c8a59301419af6132e06cacbb2860968c71
Author: tribbloid 
Date:   2015-12-21T19:07:05Z

remove the redundant toSeq and pull up lines in brackets

commit 6af8fd8b824f5f343a01868560b74a1f55acd02f
Author: tribbloid 
Date:   2015-12-28T02:05:00Z

improve styling in UnserializableDriverHelper and JDBCRelation




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48460893
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -75,6 +78,16 @@ private[sql] object JDBCRelation {
 }
 ans.toArray
   }
+
+  def getEffectiveProperties(
+  connectionProperties: Properties,
+  extraOptions: scala.collection.Map[String, String] = Map()): 
Properties = {
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48460878
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/UnserializableDriverHelper.scala
 ---
@@ -0,0 +1,54 @@
+/*
+ * 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.{DriverManager, Connection}
+import java.util.Properties
+import java.util.logging.Logger
+
+object UnserializableDriverHelper {
+
+  def replaceDriverDuring[T](f: => T): T = {
+import scala.collection.JavaConverters._
+
+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.filter(_.acceptsURL("jdbc:h2:"))
+oldDrivers.foreach{ DriverManager.deregisterDriver }
+DriverManager.registerDriver(UnserializableH2Driver)
+
+val result = try { f }
+finally {
--- End diff --

Yes, fixing try-finally formatting


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-166854056
  
@tribbloid let's wrap this up at last. Can you please review all the 
outstanding comments and address them or else close this?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-167015238
  
Of course Dr. I'll finish reading in a few hours.

Yours Peng

On 15-12-23 05:10 AM, Sean Owen wrote:
>
> @tribbloid  let's wrap this up at last. 
> Can you please review all the outstanding comments and address them or 
> else close this?
>
> —
> Reply to this email directly or view it on GitHub 
> .
>




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48194955
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/UnserializableDriverHelper.scala
 ---
@@ -0,0 +1,54 @@
+/*
+ * 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.{DriverManager, Connection}
+import java.util.Properties
+import java.util.logging.Logger
+
+object UnserializableDriverHelper {
+
+  def replaceDriverDuring[T](f: => T): T = {
+import scala.collection.JavaConverters._
--- End diff --

It's not necessary to make this local


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48194924
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/UnserializableDriverHelper.scala
 ---
@@ -0,0 +1,54 @@
+/*
+ * 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.{DriverManager, Connection}
+import java.util.Properties
+import java.util.logging.Logger
+
+object UnserializableDriverHelper {
+
+  def replaceDriverDuring[T](f: => T): T = {
+import scala.collection.JavaConverters._
+
+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.filter(_.acceptsURL("jdbc:h2:"))
+oldDrivers.foreach{ DriverManager.deregisterDriver }
+DriverManager.registerDriver(UnserializableH2Driver)
+
+val result = try { f }
+finally {
--- End diff --

This still isn't normal try-finally formatting

```
try {

} finally {

}
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48194995
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/UnserializableDriverHelper.scala
 ---
@@ -0,0 +1,54 @@
+/*
+ * 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.{DriverManager, Connection}
+import java.util.Properties
+import java.util.logging.Logger
+
+object UnserializableDriverHelper {
+
+  def replaceDriverDuring[T](f: => T): T = {
+import scala.collection.JavaConverters._
+
+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.filter(_.acceptsURL("jdbc:h2:"))
+oldDrivers.foreach{ DriverManager.deregisterDriver }
--- End diff --

Still not right w.r.t spaces: `foo.foreach(bar)`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r48195057
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -75,6 +78,16 @@ private[sql] object JDBCRelation {
 }
 ans.toArray
   }
+
+  def getEffectiveProperties(
+  connectionProperties: Properties,
+  extraOptions: scala.collection.Map[String, String] = Map()): 
Properties = {
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-166392614
  
Sorry I misunderstood your intention on previous comments, now both problem 
should be fixed: toSeq is removed and single line of code in brackets are 
pulled up.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-165577580
  
@tribbloid I think there are still some little style comments in 
`UnserializableDriverHelper`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-165099839
  
I think this is looking good otherwise.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r47773368
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -75,6 +75,18 @@ private[sql] object JDBCRelation {
 }
 ans.toArray
   }
+
+  def getEffectiveProperties(
+  connectionProperties: Properties,
+  extraOptions: scala.collection.Map[String, String] = Map()): 
Properties = {
+import scala.collection.JavaConverters._
--- End diff --

Here and below I don't see a need to scope this import so locally. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r47773335
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/UnserializableDriverHelper.scala
 ---
@@ -0,0 +1,58 @@
+/*
+ * 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.{DriverManager, Connection}
+import java.util.Properties
+import java.util.logging.Logger
+
+object UnserializableDriverHelper {
+
+  def replaceDriverDuring[T](f: => T): T = {
+import scala.collection.JavaConverters._
+
+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.filter(_.acceptsURL("jdbc:h2:")).toSeq
+oldDrivers.foreach{
--- End diff --

`asScala` should already make a `Seq`. You'll also probably want to just 
write `oldDrivers.foreach(DriverManager.deregisterDriver)` as I don't know if 
this will pass style checks (?) I think there's still the try-finally issue 
below.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r47790709
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -75,6 +75,18 @@ private[sql] object JDBCRelation {
 }
 ans.toArray
   }
+
+  def getEffectiveProperties(
+  connectionProperties: Properties,
+  extraOptions: scala.collection.Map[String, String] = Map()): 
Properties = {
+import scala.collection.JavaConverters._
--- End diff --

already corrected by moving to object-level scope


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-165147353
  
Good, thanks a lot for advices.
This should be the last patch


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r47730716
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -88,7 +100,11 @@ private[sql] case class JDBCRelation(
 
   override val needConversion: Boolean = false
 
-  override val schema: StructType = JDBCRDD.resolveTable(url, table, 
properties)
+  override val schema: StructType = JDBCRDD.resolveTable(
+url,
--- End diff --

Yes it does, changed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r47730785
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/UnserializableDriverHelper.scala
 ---
@@ -0,0 +1,58 @@
+/*
+ * 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.{DriverManager, Connection}
+import java.util.Properties
+import java.util.logging.Logger
+
+object UnserializableDriverHelper {
+
+  def replaceDriverDuring[T](f: => T): T = {
+import scala.collection.JavaConverters._
+
+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.filter(_.acceptsURL("jdbc:h2:")).toSeq
+oldDrivers.foreach{
--- End diff --

IMHO not if the previous line has ```toSeq```, should I refactor to make it 
obvious


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r47730829
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/UnserializableDriverHelper.scala
 ---
@@ -0,0 +1,58 @@
+/*
+ * 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.{DriverManager, Connection}
+import java.util.Properties
+import java.util.logging.Logger
+
+object UnserializableDriverHelper {
+
+  def replaceDriverDuring[T](f: => T): T = {
+import scala.collection.JavaConverters._
+
+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.filter(_.acceptsURL("jdbc:h2:")).toSeq
+oldDrivers.foreach{
+  DriverManager.deregisterDriver
+}
+DriverManager.registerDriver(UnserializableH2Driver)
+
+val result = try {
+  f
+}
+finally {
--- End diff --

same as before


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-164968520
  
Thanks a lot! All addressed. Sorry I was distracted from this ticket for 2 
days


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r47730696
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -75,6 +75,18 @@ private[sql] object JDBCRelation {
 }
 ans.toArray
   }
+
+  def getEffectiveProperties(
+  connectionProperties: Properties,
+  extraOptions: scala.collection.Map[String, String] = Map()): 
Properties = {
+val props = new Properties()
+extraOptions.foreach { case (key, value) =>
--- End diff --

Totally agree, changed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r47206801
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -75,6 +76,19 @@ private[sql] object JDBCRelation {
 }
 ans.toArray
   }
+
+  def getEffectiveProperties(
--- End diff --

@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?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r47206871
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -75,6 +75,18 @@ private[sql] object JDBCRelation {
 }
 ans.toArray
   }
+
+  def getEffectiveProperties(
+  connectionProperties: Properties,
+  extraOptions: scala.collection.Map[String, String] = Map()): 
Properties = {
+val props = new Properties()
+extraOptions.foreach { case (key, value) =>
--- End diff --

Sorry, only now really thinking through this -- if you import 
`scala.collection.JavaConverters._` then this clause can just be 
`props.putAll(extraOptions.asJava)`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r47207033
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/UnserializableDriverHelper.scala
 ---
@@ -0,0 +1,58 @@
+/*
+ * 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.{DriverManager, Connection}
+import java.util.Properties
+import java.util.logging.Logger
+
+object UnserializableDriverHelper {
+
+  def replaceDriverDuring[T](f: => T): T = {
+import scala.collection.JavaConverters._
+
+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.filter(_.acceptsURL("jdbc:h2:")).toSeq
+oldDrivers.foreach{
--- End diff --

`oldDrivers.foreach(DriverManager.deregisterDriver)`. Do you need `toSeq` 
above?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r47206961
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala
 ---
@@ -88,7 +100,11 @@ private[sql] case class JDBCRelation(
 
   override val needConversion: Boolean = false
 
-  override val schema: StructType = JDBCRDD.resolveTable(url, table, 
properties)
+  override val schema: StructType = JDBCRDD.resolveTable(
+url,
--- End diff --

Same, I think we may need to wrap this differently. Does it go on one line 
if `JDBCRDD` starts a new line?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r47207095
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/UnserializableDriverHelper.scala
 ---
@@ -0,0 +1,58 @@
+/*
+ * 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.{DriverManager, Connection}
+import java.util.Properties
+import java.util.logging.Logger
+
+object UnserializableDriverHelper {
+
+  def replaceDriverDuring[T](f: => T): T = {
+import scala.collection.JavaConverters._
+
+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.filter(_.acceptsURL("jdbc:h2:")).toSeq
+oldDrivers.foreach{
+  DriverManager.deregisterDriver
+}
+DriverManager.registerDriver(UnserializableH2Driver)
+
+val result = try {
+  f
+}
+finally {
--- End diff --

Pull this up to the previous line. Same comment about foreach below.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-163455378
  
@srowen Sorry you are right, there is already a deep copy and I should just 
use that, will correct immediately


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-163455429
  
done! Thanks a lot for pointing it out!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-162604858
  
@srowen yeah, I'll reply shortly


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-162701698
  
@srowen thanks a lot for posting the problem in import declarations, I've 
already correct it and won't optimize import habitually.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-162703442
  
@srowen for you second question: the connection property is deep copied 
twice to ensure that the original object is immutable. Reverting it breaks the 
scenario where a property is used in 2 JDBC write in short sequence. I haven't 
include this scenario into unit test yet, do you prefer me doing it? or this is 
not expected?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-162705458
  
I don't get that -- what does the original object matter if it's copied 
here? and how would the copy change? 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-162213600
  
@tribbloid are you still working on this? I had an outstanding question or 
two here


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r46122352
  
--- Diff: 
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCWriteSuite.scala ---
@@ -20,12 +20,11 @@ package org.apache.spark.sql.jdbc
 import java.sql.DriverManager
 import java.util.Properties
 
-import org.scalatest.BeforeAndAfter
-
-import org.apache.spark.sql.{Row, SaveMode}
 import org.apache.spark.sql.test.SharedSQLContext
 import org.apache.spark.sql.types._
+import org.apache.spark.sql.{Row, SaveMode}
 import org.apache.spark.util.Utils
+import org.scalatest.BeforeAndAfter
--- End diff --

Nit: most of the import changes in this PR are actually the wrong way; can 
you restore the ordering?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r46122485
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/DataFrameWriter.scala ---
@@ -303,7 +298,8 @@ final class DataFrameWriter private[sql](df: DataFrame) 
{
   conn.close()
 }
 
-JdbcUtils.saveTable(df, url, table, props)
+val propsForSave = 
JDBCRelation.getEffectiveProperties(connectionProperties, this.extraOptions)
--- End diff --

Sorry, I may still not understand: why can't you use `props` here? you're 
copying the `Properties` twice, it seems.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on the pull request:

https://github.com/apache/spark/pull/10020#issuecomment-160265026
  
This should be closed and be part of 
https://github.com/apache/spark/pull/8785 or else I'm unclear on the 
difference? Let's put the change in one place please.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on the pull request:

https://github.com/apache/spark/pull/10020#issuecomment-160333704
  
sorry its just a squashed commit, I speculate that PMC will squash rebase 
it anyway but I'm not so sure, so I requested 2 identical patches


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid closed the pull request at:

https://github.com/apache/spark/pull/10020


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/8785#discussion_r46041751
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/DataFrameWriter.scala ---
@@ -280,7 +275,8 @@ final class DataFrameWriter private[sql](df: DataFrame) 
{
   conn.close()
 }
 
-JdbcUtils.saveTable(df, url, table, props)
+val props2 = JDBCRelation.getEffectiveProperties(connectionProperties, 
this.extraOptions)
--- End diff --

Why does this need to be created twice? isn't `props` the same?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[srowen]

Github user srowen commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-160127622
  
I guess I'm missing why a deep copy solves a problem of an unserializable 
property value. It still exists in the copy right?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

GitHub user tribbloid opened a pull request:

https://github.com/apache/spark/pull/10020

[Spark-10625] [SQL] Spark SQL JDBC read/write is unable to handle JDBC 
Drivers that adds unserializable objects into connection properties

this pull request has several commits squashed together

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/Schedule1/spark SPARK-10625-squashed

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/10020.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #10020


commit e7aa6e7e5618e2a1f5157919322aefd3511caa00
Author: tribbloid 
Date:   2015-09-15T23:02:57Z

test case demonstrating SPARK-10625:
Spark SQL JDBC read/write is unable to handle JDBC Drivers that adds 
unserializable objects into connection properties

add one more unit test
fix JDBCRelation & DataFrameWriter to pass all tests

revise scala style

put driver replacement code into a shared function

fix styling

upgrade to master and resolve all related issues




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/10020#issuecomment-160206949
  
Can one of the admins verify this patch?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-160162604
  
Hi Sean, the property value can only be mutated by JDBC driver's function 
to fetch the schema on Spark driver, after which it's no longer serializable. 
My deep copy is made before the mutation


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [Spark-10625] [SQL] Spark SQL JDBC read/write ...

[tribbloid]

Github user tribbloid commented on the pull request:

https://github.com/apache/spark/pull/8785#issuecomment-142306277
  
PR fixed as well, thanks a lot Holden!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org