Skip to content

[SPARK-15256] [SQL] [PySpark] Clarify DataFrameReader.jdbc() docstring #13034

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
Closed
nchammas wants to merge 5 commits into from
Closed

[SPARK-15256] [SQL] [PySpark] Clarify DataFrameReader.jdbc() docstring #13034

nchammas wants to merge 5 commits into from

Conversation

@nchammas
Copy link
Contributor

@nchammas nchammas commented May 10, 2016 •
edited
Loading

This PR:

  • Corrects the documentation for the properties parameter, which is supposed to be a dictionary and not a list.
  • Generally clarifies the Python docstring for DataFrameReader.jdbc() by pulling from the Scala docstrings and rephrasing things.
  • Corrects minor Sphinx typos.

nchammas
nchammas reviewed May 10, 2016
View reviewed changes
python/pyspark/sql/readwriter.py Outdated
Copy link
Contributor Author

@nchammas nchammas May 10, 2016

Choose a reason for hiding this comment

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

Single backticks in Sphinx just make things italic. e.g. See the current doc.

@SparkQA
Copy link

SparkQA commented May 10, 2016

Test build #58278 has finished for PR 13034 at commit 7699db2.

  • This patch fails Python style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented May 10, 2016

Test build #58282 has finished for PR 13034 at commit 6e30db8.

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

@nchammas
Copy link
Contributor Author

nchammas commented May 10, 2016

cc @davies

davies
davies reviewed May 10, 2016
View reviewed changes
python/pyspark/sql/readwriter.py Outdated
Copy link
Contributor

@davies davies May 10, 2016

Choose a reason for hiding this comment

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

We are deprecating HiveContext in 2.0, should we change this to say "Hive support"?

Copy link
Contributor Author

@nchammas nchammas May 10, 2016

Choose a reason for hiding this comment

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

Sure, I'll change all occurrences of this within this file.

Copy link
Contributor Author

@nchammas nchammas May 10, 2016

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@davies davies May 10, 2016

Choose a reason for hiding this comment

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

Could you also update the doc tests?

Copy link
Contributor Author

@nchammas nchammas May 10, 2016

Choose a reason for hiding this comment

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

Sure, though I'm not sure what to update specifically. Could you elaborate?

Copy link
Contributor

@davies davies May 11, 2016

Choose a reason for hiding this comment

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

I meant the hiveContext in doc tests. If that is not easy, we could do that later.

Copy link
Contributor Author

@nchammas nchammas May 11, 2016

Choose a reason for hiding this comment

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

Ah OK, I'll take a look.

Copy link
Contributor Author

@nchammas nchammas May 11, 2016

Choose a reason for hiding this comment

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

Looks like that was taken care of in #13044.

Copy link
Contributor Author

@nchammas nchammas May 11, 2016

Choose a reason for hiding this comment

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

Oh nevermind, I think that person missed a few places...

@SparkQA
Copy link

SparkQA commented May 10, 2016

Test build #58288 has finished for PR 13034 at commit 26dc46b.

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

@SparkQA
Copy link

SparkQA commented May 11, 2016

Test build #58371 has finished for PR 13034 at commit 576c404.

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

@nchammas
Copy link
Contributor Author

nchammas commented May 11, 2016

@davies - I think this is ready for another review.

I wasn't sure what doc tests you were referring to that needed to be updated, but all the tests are passing.

@davies
Copy link
Contributor

davies commented May 11, 2016

@nchammas Could you add [SQL] or [PYSPARK] in the title?

@nchammas nchammas changed the title [SPARK-15256] Clarify DataFrameReader.jdbc() docstring [SPARK-15256] [SQL] [PySpark] Clarify DataFrameReader.jdbc() docstring May 11, 2016
@SparkQA
Copy link

SparkQA commented May 11, 2016

Test build #58403 has finished for PR 13034 at commit 183b6e7.

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

@SparkQA
Copy link

SparkQA commented May 11, 2016

Test build #58407 has finished for PR 13034 at commit 6a73fe9.

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

@SparkQA
Copy link

SparkQA commented May 11, 2016

Test build #2985 has finished for PR 13034 at commit 6a73fe9.

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

@SparkQA
Copy link

SparkQA commented May 11, 2016

Test build #58414 has finished for PR 13034 at commit e7d97e4.

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

@davies
Copy link
Contributor

davies commented May 11, 2016

LGTM,
Merging this into master and 2.0, thanks!

asfgit pushed a commit that referenced this pull request May 11, 2016
@nchammas @davies
[SPARK-15256] [SQL] [PySpark] Clarify DataFrameReader.jdbc() docstring
f9ea545 
This PR:
* Corrects the documentation for the `properties` parameter, which is supposed to be a dictionary and not a list.
* Generally clarifies the Python docstring for DataFrameReader.jdbc() by pulling from the [Scala docstrings](https://github.com/apache/spark/blob/b28137764716f56fa1a923c4278624a56364a505/sql/core/src/main/scala/org/apache/spark/sql/DataFrameReader.scala#L201-L251) and rephrasing things.
* Corrects minor Sphinx typos.

Author: Nicholas Chammas <[email protected]>

Closes #13034 from nchammas/SPARK-15256.

(cherry picked from commit b9cf617)
Signed-off-by: Davies Liu <[email protected]>
@asfgit asfgit closed this in b9cf617 May 11, 2016
@nchammas nchammas deleted the SPARK-15256 branch May 11, 2016 22:34
@nchammas
Copy link
Contributor Author

nchammas commented May 11, 2016

Thanks for reviewing @davies!

@SparkQA
Copy link

SparkQA commented May 11, 2016

Test build #58401 has finished for PR 13034 at commit 61dd221.

  • This patch fails PySpark unit tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nchammas @SparkQA @davies