Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-19296][SQL] Deduplicate url and table in JdbcUtils #16753

Closed
wants to merge 2 commits into from
Closed

[SPARK-19296][SQL] Deduplicate url and table in JdbcUtils #16753

wants to merge 2 commits into from

Conversation

HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Jan 31, 2017 •
edited

What changes were proposed in this pull request?

This PR deduplicates arguments, url and table in JdbcUtils with JDBCOptions.

It avoids to use duplicated arguments, for example, as below:

from

val jdbcOptions = new JDBCOptions(url, table, map)
JdbcUtils.saveTable(ds, url, table, jdbcOptions)

to

val jdbcOptions = new JDBCOptions(url, table, map)
JdbcUtils.saveTable(ds, jdbcOptions)

How was this patch tested?

Running unit test in JdbcSuite/JDBCWriteSuite

Building with Scala 2.10 as below:

./dev/change-scala-version.sh 2.10
./build/mvn -Pyarn -Phadoop-2.4 -Dscala-2.10 -DskipTests clean package

@HyukjinKwon
Copy link
Member Author

Hi @gatorsmile, could you take a look for this one please? (It might not need a JIRA but it happened to be opened by someone).

@srowen
Copy link
Member

srowen commented Jan 31, 2017

It's true, though I wonder if it's still by design, that these methods take url and table as important first-class arguments, and then also other options, even though the options also contain the same arguments.

Or, could the other methods like tableExists reasonably also not have to take these arguments? Consistency is probably more important.

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Jan 31, 2017 •
edited

@srowen, I see. Let me maybe give a shot to make them consistent to show if it looks good (otherwise, let me just close my pr and resolve his JIRA).

@HyukjinKwon HyukjinKwon changed the title [SPARK-19296][SQL] Deduplicate arguments in JdbcUtils.saveTable [SPARK-19296][SQL] Deduplicate url and table in JdbcUtils Jan 31, 2017
@SparkQA
Copy link

SparkQA commented Jan 31, 2017

Test build #72192 has finished for PR 16753 at commit 9be8f84.

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

@SparkQA
Copy link

SparkQA commented Jan 31, 2017

Test build #72193 has finished for PR 16753 at commit d78a7d0.

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

@SparkQA
Copy link

SparkQA commented Jan 31, 2017

Test build #72194 has finished for PR 16753 at commit cfe258b.

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

@SparkQA
Copy link

SparkQA commented Jan 31, 2017

Test build #72199 has finished for PR 16753 at commit 6b2841a.

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

srowen
srowen requested changes Jan 31, 2017
View reviewed changes
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

Yeah that does look tidier to me. If this is indeed a private API, it seems reasonable to rationalize it in this way. Not sure who else might be a good reviewer here.

gatorsmile
gatorsmile reviewed Feb 1, 2017
View reviewed changes
...re/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcRelationProvider.scala Outdated
@@ -53,33 +53,31 @@ class JdbcRelationProvider extends CreatableRelationProvider
parameters: Map[String, String],
df: DataFrame): BaseRelation = {
val jdbcOptions = new JDBCOptions(parameters)
val url = jdbcOptions.url
val table = jdbcOptions.table
Copy link
Member

@gatorsmile gatorsmile Feb 1, 2017 •
edited

Choose a reason for hiding this comment

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

Maybe, we do not need to have a variable for table too.

gatorsmile
gatorsmile reviewed Feb 1, 2017
View reviewed changes
...re/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcRelationProvider.scala Outdated
val table = jdbcOptions.table
val createTableOptions = jdbcOptions.createTableOptions
val isTruncate = jdbcOptions.isTruncate
Copy link
Member

Choose a reason for hiding this comment

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

The same here.

@gatorsmile
Copy link
Member

LGTM except two comments.

@HyukjinKwon
Copy link
Member Author

Thank you @srowen and @gatorsmile!

@SparkQA
Copy link

SparkQA commented Feb 1, 2017

Test build #72245 has finished for PR 16753 at commit 918456a.

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

@gatorsmile
Copy link
Member

gatorsmile commented Feb 1, 2017 •
edited

LGTM. Thanks! Merging to master.

@asfgit asfgit closed this in 5ed397b Feb 1, 2017
cmonkey pushed a commit to cmonkey/spark that referenced this pull request Feb 15, 2017
@HyukjinKwon @cmonkey
[SPARK-19296][SQL] Deduplicate url and table in JdbcUtils
71e5095 
## What changes were proposed in this pull request?

This PR deduplicates arguments, `url` and `table` in `JdbcUtils` with `JDBCOptions`.

It avoids to use duplicated arguments, for example, as below:

from

```scala
val jdbcOptions = new JDBCOptions(url, table, map)
JdbcUtils.saveTable(ds, url, table, jdbcOptions)
```

to

```scala
val jdbcOptions = new JDBCOptions(url, table, map)
JdbcUtils.saveTable(ds, jdbcOptions)
```

## How was this patch tested?

Running unit test in `JdbcSuite`/`JDBCWriteSuite`

Building with Scala 2.10 as below:

```
./dev/change-scala-version.sh 2.10
./build/mvn -Pyarn -Phadoop-2.4 -Dscala-2.10 -DskipTests clean package
```

Author: hyukjinkwon <gurwls223@gmail.com>

Closes apache#16753 from HyukjinKwon/SPARK-19296.
@HyukjinKwon HyukjinKwon deleted the SPARK-19296 branch January 2, 2018 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gatorsmile gatorsmile gatorsmile left review comments

@srowen srowen srowen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@HyukjinKwon @srowen @SparkQA @gatorsmile