Skip to content

Conversation

@vlyubin
Copy link
Contributor

@vlyubin vlyubin commented Mar 3, 2015

One more thing if this PR is considered to be OK - it might make sense to add extra .jdbc() API's that take Properties to SQLContext.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@aarondav
Copy link
Contributor

aarondav commented Mar 3, 2015

Jenkins, add to whitelist

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28202 has started for PR 4859 at commit 5bed5f3.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 3, 2015

Test build #28202 has finished for PR 4859 at commit 5bed5f3.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28202/
Test PASSed.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably document this behavior somewhere -- I'm not sure if there's a standard place for data sources. If nowhere else, at least add it to the doc of createRelation().

Copy link
Contributor

Choose a reason for hiding this comment

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

We should add this to the programming guide under JDBC.

Copy link
Contributor

Choose a reason for hiding this comment

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

@aarondav
Copy link
Contributor

aarondav commented Mar 3, 2015

I think it would also be reasonable to add to the SQLContext#jdbc() methods a parameter for options: Map[String, String] like was done for createExternalTable. This would require confirmation by @marmbrus or @rxin, though.

@SparkQA
Copy link

SparkQA commented Mar 23, 2015

Test build #29012 has started for PR 4859 at commit 9f32724.

  • This patch merges cleanly.

@aarondav
Copy link
Contributor

This LGTM, we'll merge this and later add a jdbc() version.

@SparkQA
Copy link

SparkQA commented Mar 23, 2015

Test build #29012 has finished for PR 4859 at commit 9f32724.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29012/
Test PASSed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why null as the default instead of an empty properties map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No particular reason really, both are fine with DriverManagers' getConnection(). I've switched to empty properties map, I guess it is in fact neater than null.

@marmbrus
Copy link
Contributor

One minor comment otherwise LGTM, provided documentation and a programatic interface are added in a follow-up PR.

@SparkQA
Copy link

SparkQA commented Mar 23, 2015

Test build #29032 has started for PR 4859 at commit 7a8cfda.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 23, 2015

Test build #29032 has finished for PR 4859 at commit 7a8cfda.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29032/
Test PASSed.

@marmbrus
Copy link
Contributor

Thanks! Merged to master and branch-1.3.

@asfgit asfgit closed this in bfd3ee9 Mar 24, 2015
asfgit pushed a commit that referenced this pull request Mar 24, 2015
…e query

One more thing if this PR is considered to be OK - it might make sense to add extra .jdbc() API's that take Properties to SQLContext.

Author: Volodymyr Lyubinets <[email protected]>

Closes #4859 from vlyubin/jdbcProperties and squashes the following commits:

7a8cfda [Volodymyr Lyubinets] Support jdbc connection properties in OPTIONS part of the query

(cherry picked from commit bfd3ee9)
Signed-off-by: Michael Armbrust <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants