[SPARK-26383][Core] - NPE when use DataFrameReader.jdbc with wrong URL #23464

ayudovin · 2019-01-05T14:08:30Z

What changes were proposed in this pull request?

When passing wrong url to jdbc then It would throw IllegalArgumentException instead of NPE.

How was this patch tested?

Adding test case to Existing tests in JDBCSuite

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRDD.scala

HyukjinKwon · 2019-01-05T14:16:30Z

Please also follow the PR template. How did you test this?

dongjoon-hyun · 2019-01-05T14:20:18Z

It would be great if we have a test case.

ayudovin · 2019-01-05T21:00:16Z

I'm new one of contributing to spark.
I try to add tests to this pull requests but now I have some problem with compiling

gatorsmile · 2019-01-05T22:25:09Z

Add a test case in JDBCSuite?

You can run the test case by the following command:

build/sbt "sql/test-only org.apache.spark.sql.jdbc.JDBCSuite"

gatorsmile · 2019-01-05T22:25:36Z

ok to test

SparkQA · 2019-01-06T01:39:51Z

Test build #100812 has finished for PR 23464 at commit 5215f1b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-01-06T13:31:59Z

Test build #100827 has finished for PR 23464 at commit 2a6243c.

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

SparkQA · 2019-01-06T13:58:28Z

Test build #100828 has finished for PR 23464 at commit 0989db9.

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

srowen · 2019-01-06T14:40:11Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRDD.scala

@@ -54,6 +54,11 @@ object JDBCRDD extends Logging {
    val table = options.tableOrQuery
    val dialect = JdbcDialects.get(url)
    val conn: Connection = JdbcUtils.createConnectionFactory(options)()
+
+    if (null == conn) {


Nit: conn == null. Yes, I think this check should be in the function that createConnectionFactory returns. I doubt it's ever reasonable to return a null connection.

Yea. It should be in that function. We can use require function to throw illegal argument exception. Looks it can potentially return null assuming from it's doc.

Does it mean that I should to replace this check to createConnectionFactory ?

Yeah, to the lambda it returns

srowen · 2019-01-06T14:40:29Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRDD.scala

@@ -68,7 +73,9 @@ object JDBCRDD extends Logging {
        statement.close()
      }
    } finally {
-      conn.close()
+      if (null != conn) {


It can't be null here right?

yes, it can't be null now because we throw exception before.
I just forgot to remove it

srowen · 2019-01-06T16:52:46Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRDD.scala

@@ -54,6 +54,7 @@ object JDBCRDD extends Logging {
    val table = options.tableOrQuery
    val dialect = JdbcDialects.get(url)
    val conn: Connection = JdbcUtils.createConnectionFactory(options)()
+


Go ahead and revert this

srowen · 2019-01-06T16:54:28Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala

-      driver.connect(options.url, options.asConnectionProperties)
+      val connection: Connection = driver.connect(options.url, options.asConnectionProperties)
+
+      if (connection == null) {


Use require(connection != null, ...). I'd also change the message and scaladoc to be a little more general, like here: s"The driver could not open a JDBC connection. Check the URL: ${options.url}"

srowen · 2019-01-06T16:56:14Z

sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala

+  test("SPARK-26383 throw IllegalArgumentException if url is wrong") {
+    val e = intercept[IllegalArgumentException] {
+      val opts = Map(
+        "url" -> "jdbc:mysql://localhost/db",


Do you want to use a more obviously invalid URL here? or does it only trigger this scenario if the URL is valid but can't be opened?

The driver returns null if it realizes it is the wrong kind of driver to connect to the given URL. I used mysql url with postgresql driver.

SparkQA · 2019-01-06T20:34:25Z

Test build #100839 has finished for PR 23464 at commit 34fe718.

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

srowen · 2019-01-06T21:28:04Z

sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala

@@ -45,6 +45,7 @@ class JDBCSuite extends QueryTest

  val url = "jdbc:h2:mem:testdb0"
  val urlWithUserAndPass = "jdbc:h2:mem:testdb0;user=testUser;password=testPass"
+  val wrongUrl = "jdbc:h2:mem:testdbx"


This is unused?

srowen · 2019-01-06T21:28:16Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala

@@ -48,6 +48,7 @@ object JdbcUtils extends Logging {
   * Returns a factory for creating connections to the given JDBC URL.
   *
   * @param options - JDBC options that contains url, table and other information.
+   * @throws IllegalArgumentException If JDBC options has wrong url


Maybe edit this description too per the exception message

SparkQA · 2019-01-06T23:53:34Z

Test build #100845 has finished for PR 23464 at commit e8fd2b6.

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

SparkQA · 2019-01-07T01:37:32Z

Test build #100847 has finished for PR 23464 at commit 07f5898.

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

srowen · 2019-01-07T14:58:40Z

Merged to master

### What changes were proposed in this pull request? When passing wrong url to jdbc then It would throw IllegalArgumentException instead of NPE. ### How was this patch tested? Adding test case to Existing tests in JDBCSuite Closes apache#23464 from ayudovin/fixing-npe. Authored-by: ayudovin <a.yudovin6695@gmail.com> Signed-off-by: Sean Owen <sean.owen@databricks.com>

fixing npe when wrong url

feca3ad

HyukjinKwon reviewed Jan 5, 2019

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRDD.scala Outdated Show resolved Hide resolved

fixing npe when wrong url

5215f1b

ayudovin added 2 commits January 6, 2019 12:30

throw exception if conn is null

2a6243c

adding tests for checking if conn is null

0989db9

srowen requested changes Jan 6, 2019

View reviewed changes

replace checking for null

34fe718

srowen requested changes Jan 6, 2019

View reviewed changes

changing to require instead of if

e8fd2b6

srowen reviewed Jan 6, 2019

View reviewed changes

clean code

07f5898

srowen approved these changes Jan 7, 2019

View reviewed changes

HyukjinKwon approved these changes Jan 7, 2019

View reviewed changes

srowen closed this in 868e025 Jan 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-26383][Core] - NPE when use DataFrameReader.jdbc with wrong URL #23464

[SPARK-26383][Core] - NPE when use DataFrameReader.jdbc with wrong URL #23464

ayudovin commented Jan 5, 2019 •

edited

Loading

HyukjinKwon commented Jan 5, 2019

dongjoon-hyun commented Jan 5, 2019

ayudovin commented Jan 5, 2019

gatorsmile commented Jan 5, 2019

gatorsmile commented Jan 5, 2019

SparkQA commented Jan 6, 2019

SparkQA commented Jan 6, 2019

SparkQA commented Jan 6, 2019

srowen Jan 6, 2019

HyukjinKwon Jan 6, 2019

ayudovin Jan 6, 2019

srowen Jan 6, 2019

srowen Jan 6, 2019

ayudovin Jan 6, 2019

srowen Jan 6, 2019

srowen Jan 6, 2019

srowen Jan 6, 2019

ayudovin Jan 6, 2019

SparkQA commented Jan 6, 2019

srowen Jan 6, 2019

srowen Jan 6, 2019

SparkQA commented Jan 6, 2019

SparkQA commented Jan 7, 2019

srowen commented Jan 7, 2019

[SPARK-26383][Core] - NPE when use DataFrameReader.jdbc with wrong URL #23464

[SPARK-26383][Core] - NPE when use DataFrameReader.jdbc with wrong URL #23464

Conversation

ayudovin commented Jan 5, 2019 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

HyukjinKwon commented Jan 5, 2019

dongjoon-hyun commented Jan 5, 2019

ayudovin commented Jan 5, 2019

gatorsmile commented Jan 5, 2019

gatorsmile commented Jan 5, 2019

SparkQA commented Jan 6, 2019

SparkQA commented Jan 6, 2019

SparkQA commented Jan 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jan 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jan 6, 2019

SparkQA commented Jan 7, 2019

srowen commented Jan 7, 2019

ayudovin commented Jan 5, 2019 •

edited

Loading