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.

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

[MINOR][DOC] Fix comments of ConvertToLocalRelation rule #23273

Closed
wants to merge 2 commits into from

Conversation

seancxmao
Copy link
Contributor

@seancxmao seancxmao commented Dec 10, 2018

What changes were proposed in this pull request?

There are some comments issues left when ConvertToLocalRelation rule was added (see #22205/SPARK-25212). This PR fixes those comments issues.

How was this patch tested?

N/A

@SparkQA
Copy link

SparkQA commented Dec 10, 2018

Test build #99909 has finished for PR 23273 at commit dfd0f71.

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

@seancxmao
Copy link
Contributor Author

retest this please.

@SparkQA
Copy link

SparkQA commented Dec 11, 2018

Test build #99944 has finished for PR 23273 at commit dfd0f71.

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

*
* This is relatively simple as it currently handles only 2 single case: Project and Limit.
* This rule currently handles 3 cases: [[Project]], [[Limit]] and [[Filter]].
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should just not specify this in the docs. Are you sure the [[...]] links work when you generate scaladoc? the test builder doesn't check. I've seen many errors/warnings generated by some types of links.
If so again maybe easier to restrict this to merely removing this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srowen Sorry, I found that the links just do not work for scaladoc, though they works in IDE like Intellij IDEA. I should have generated the docs to see if there's any problem.

I have changed [[...]] to backticks in a new commit.

Also, I have some findings:

  • Because org/apache/spark/sql/catalyst is excluded for doc generation, I include catalyst in SparkBuild.scala and run build/sbt unidoc, there are many errors/warnings, should we fix these problems? Seems simple but many places.
  • As for [[...]], fully qualified class name should be used if a link points to a class in another package. If the link points to a class in the same package, package prefix could be omitted.

Copy link
Member

Choose a reason for hiding this comment

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

catalyst isn't meant to be a public API (for our convenience, it's not all marked private in the source). That's why it's not in the docs and yeah that's why writing references to it won't work, as I recall.

For this comment I think it's simplest to remove it, or leave it but don't add references, or move it to an inline comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot @srowen for your explanation. Since it's not a public API and the code is clear, it makes sense to remove this line, this can also eliminate the needs to modify this line when new cases are added. I have removed this line in the new commit.

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.

I also wouldn't tag this as SPARK-25212 as that was merged into 2.4.0. This is just a "MINOR" change

@seancxmao seancxmao changed the title [SPARK-25212][SQL][FOLLOWUP][DOC] Fix comments of ConvertToLocalRelation rule [MINOR][DOC] Fix comments of ConvertToLocalRelation rule Dec 12, 2018
@SparkQA
Copy link

SparkQA commented Dec 12, 2018

Test build #100006 has finished for PR 23273 at commit 1cca847.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@seancxmao
Copy link
Contributor Author

retest this please.

@SparkQA
Copy link

SparkQA commented Dec 12, 2018

Test build #100032 has finished for PR 23273 at commit 1cca847.

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

@seancxmao
Copy link
Contributor Author

retest this please.

@SparkQA
Copy link

SparkQA commented Dec 13, 2018

Test build #100054 has finished for PR 23273 at commit 1cca847.

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

@seancxmao
Copy link
Contributor Author

seancxmao commented Dec 13, 2018

@srowen The build failed twice because of the same reason. It seems we ran into SparkR CRAN feasibility check server problem, which blocks PR build.

Error messages from log:

* this is package 'SparkR' version '3.0.0'
* checking CRAN incoming feasibility ...Error in .check_package_CRAN_incoming(pkgdir) : 
  dims [product 24] do not match the length of object [0]
Execution halted

@seancxmao
Copy link
Contributor Author

retest this please.

@SparkQA
Copy link

SparkQA commented Dec 13, 2018

Test build #100075 has finished for PR 23273 at commit 1cca847.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@seancxmao
Copy link
Contributor Author

retest this please.

@SparkQA
Copy link

SparkQA commented Dec 13, 2018

Test build #100082 has finished for PR 23273 at commit 1cca847.

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

@srowen
Copy link
Member

srowen commented Dec 13, 2018

Merged to master

@srowen srowen closed this in f372609 Dec 13, 2018
@seancxmao
Copy link
Contributor Author

Thank you! @srowen

holdenk pushed a commit to holdenk/spark that referenced this pull request Jan 5, 2019
## What changes were proposed in this pull request?
There are some comments issues left when `ConvertToLocalRelation` rule was added (see apache#22205/[SPARK-25212](https://issues.apache.org/jira/browse/SPARK-25212)). This PR fixes those comments issues.

## How was this patch tested?
N/A

Closes apache#23273 from seancxmao/ConvertToLocalRelation-doc.

Authored-by: seancxmao <seancxmao@gmail.com>
Signed-off-by: Sean Owen <sean.owen@databricks.com>
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
## What changes were proposed in this pull request?
There are some comments issues left when `ConvertToLocalRelation` rule was added (see apache#22205/[SPARK-25212](https://issues.apache.org/jira/browse/SPARK-25212)). This PR fixes those comments issues.

## How was this patch tested?
N/A

Closes apache#23273 from seancxmao/ConvertToLocalRelation-doc.

Authored-by: seancxmao <seancxmao@gmail.com>
Signed-off-by: Sean Owen <sean.owen@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants