-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-18445][BUILD][DOCS] Fix the markdown for Note:/NOTE:/Note that/'''Note:''' across Scala/Java API documentation
#15889
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
Conversation
|
I am going to, double check any missed one first, and then sweep them again and again by leaving comments with images from the built API doc each by each change to double-check. |
|
Test build #68660 has finished for PR 15889 at commit
|
|
Hi @sandeepmreddy, thanks for approving but actually it is WIP. It would be great if it is reviewed when it's complete. |
|
Test build #68721 has finished for PR 15889 at commit
|
Note:/NOTE:/Note that across Scala/Java API documentationNote:/NOTE:/Note that/'''Note:''' across Scala/Java API documentation
|
Test build #68723 has finished for PR 15889 at commit
|
|
Test build #68727 has finished for PR 15889 at commit
|
|
@HyukjinKwon this is ready to go right? |
|
Ah, I couldn't check all of the java documentation yet because I have some problems with javadoc8 because of several errors (although they look already existing ones). So, I am thinking of trying to use different version. (Maybe I am doing something wrong assuming it is not mentioned in docs/README.md). Will check several times more the built documentation and also update PR description within today.I am also fine if you are worried of conflicts from merging others. I can resolve them. |
|
Test build #68756 has started for PR 15889 at commit |
HyukjinKwon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems @note plays fine with existing formats.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note:/NOTE:/Note that/'''Note:''' across Scala/Java API documentationNote:/NOTE:/Note that/'''Note:''' across Scala/Java API documentation
project/SparkBuild.scala
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srowen, this was added to deal with @note annotation in Java documentation. It seems @note for scaladoc not for javadoc. So, I had to manually deal with this.
pom.xml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the equivalent one with SparkBuild.scala.
Note:/NOTE:/Note that/'''Note:''' across Scala/Java API documentationNote:/NOTE:/Note that/'''Note:''' across Scala/Java API documentation
|
Test build #68761 has finished for PR 15889 at commit
|
d22136e to
39873dc
Compare
|
(it was simply rebased and added (in 39873dc) 5 instances introduced in another PR) |
| * | ||
| * @note This is NOT guaranteed to provide exactly the fraction of the count | ||
| * of the given [[Dataset]]. | ||
| * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Thank you for approving Sean! |
|
Test build #68775 has finished for PR 15889 at commit
|
|
retest this please |
|
Test build #68783 has finished for PR 15889 at commit
|
|
Test build #68785 has finished for PR 15889 at commit
|
srowen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few last questions now that I skimmed it again, actually
| * "combined type" C. Note that V and C can be different -- for example, one might group an | ||
| * "combined type" C. | ||
| * | ||
| * @note V and C can be different -- for example, one might group an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was skimming the changes again and noticed this. This causes this note to render pretty separately from this text, but it seems to belong with the sentence above.
Does it cause the bullet points below to be in the note? because they also seem unrelated to this 'note'.
Could be OK but wasn't sure whether this was really a stand-alone note as much as a usage of the word "note"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yes. It seems so. Will check the same instances again. Thank you.
| * supporting the key and value types K and V in this RDD. | ||
| * | ||
| * Note that, we should make sure our tasks are idempotent when speculation is enabled, i.e. do | ||
| * @note We should make sure our tasks are idempotent when speculation is enabled, i.e. do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise I wonder if these following sentences all go into the note? they go together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It almost looks like it should be a code comment, not scaladoc. Well, I don't feel strongly about changing it if it was already in the scaladoc.
| * it will process either one or all of the RDDs returned by the queue. | ||
| * | ||
| * NOTE: | ||
| * @note |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise here just want to make sure that this actually causes the following 3 lines are in the note?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Test build #68828 has started for PR 15889 at commit |
| * items with the same key). | ||
| * | ||
| * @note V and C can be different -- for example, one might group a RDD of type (Int, Int) into | ||
| * an RDD of type (Int, List[Int]). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, it works alright as a standalone note. It could have stayed an inline sentence too. OK either way.
| * | ||
| * @note Vectors to be transformed must be the same length | ||
| * as the source vectors given to [[PCA.fit()]]. | ||
| * @note Vectors to be transformed must be the same lengtt as the source vectors given |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh sorry @HyukjinKwon but there's now a typo here: lengtt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh! I will check the same instances. I am so sorry.
|
Test build #68839 has finished for PR 15889 at commit
|
|
Test build #68850 has finished for PR 15889 at commit
|
| * In addition, users can control the partitioning of the output RDD, and whether to perform | ||
| * map-side aggregation (if a mapper can produce multiple items with the same key). | ||
| * | ||
| * @note V and C can be different -- for example, one might group a RDD of type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"an RDD" is correct actually. Could you make this one last fix by reverting just that change? I bet there are occurrences of "a RDD" in the docs elsewhere and you're welcome to fix them too if it makes it easier to search and replace it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah... I see. Sure. I thought "a RDD" is correct but just realised it after googling it.. Will fix and replace. I see. It should be an
4576f7d to
ee5b035
Compare
|
Test build #68859 has finished for PR 15889 at commit
|
|
Test build #68861 has finished for PR 15889 at commit
|
… that`/`'''Note:'''` across Scala/Java API documentation It seems in Scala/Java, - `Note:` - `NOTE:` - `Note that` - `'''Note:'''` - `note` This PR proposes to fix those to `note` to be consistent. **Before** - Scala  - Java  **After** - Scala  - Java  The notes were found via ```bash grep -r "NOTE: " . | \ # Note:|NOTE:|Note that|'''Note:''' grep -v "// NOTE: " | \ # starting with // does not appear in API documentation. grep -E '.scala|.java' | \ # java/scala files grep -v Suite | \ # exclude tests grep -v Test | \ # exclude tests grep -e 'org.apache.spark.api.java' \ # packages appear in API documenation -e 'org.apache.spark.api.java.function' \ # note that this is a regular expression. So actual matches were mostly `org/apache/spark/api/java/functions ...` -e 'org.apache.spark.api.r' \ ... ``` ```bash grep -r "Note that " . | \ # Note:|NOTE:|Note that|'''Note:''' grep -v "// Note that " | \ # starting with // does not appear in API documentation. grep -E '.scala|.java' | \ # java/scala files grep -v Suite | \ # exclude tests grep -v Test | \ # exclude tests grep -e 'org.apache.spark.api.java' \ # packages appear in API documenation -e 'org.apache.spark.api.java.function' \ -e 'org.apache.spark.api.r' \ ... ``` ```bash grep -r "Note: " . | \ # Note:|NOTE:|Note that|'''Note:''' grep -v "// Note: " | \ # starting with // does not appear in API documentation. grep -E '.scala|.java' | \ # java/scala files grep -v Suite | \ # exclude tests grep -v Test | \ # exclude tests grep -e 'org.apache.spark.api.java' \ # packages appear in API documenation -e 'org.apache.spark.api.java.function' \ -e 'org.apache.spark.api.r' \ ... ``` ```bash grep -r "'''Note:'''" . | \ # Note:|NOTE:|Note that|'''Note:''' grep -v "// '''Note:''' " | \ # starting with // does not appear in API documentation. grep -E '.scala|.java' | \ # java/scala files grep -v Suite | \ # exclude tests grep -v Test | \ # exclude tests grep -e 'org.apache.spark.api.java' \ # packages appear in API documenation -e 'org.apache.spark.api.java.function' \ -e 'org.apache.spark.api.r' \ ... ``` And then fixed one by one comparing with API documentation/access modifiers. After that, manually tested via `jekyll build`. Author: hyukjinkwon <[email protected]> Closes #15889 from HyukjinKwon/SPARK-18437. (cherry picked from commit d5b1d5f) Signed-off-by: Sean Owen <[email protected]>
|
Merged to master/2.1 |
|
Thank you!! |
| } | ||
|
|
||
| /** Write a RDD partition out in a single Spark task. */ | ||
| /** Write an RDD partition out in a single Spark task. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you changing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because a RDD was incorrect in English.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing something? I'm not familiar with English grammer, but shouldn't we use a when the RDD start with R?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I searched the code base and there are both a RDD and an RDD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should replace them to an RDD.
I think on my Mac I found and replaced them by
find . -type f -name "*.scala" | xargs sed -i '' 's/ A RDD / An RDD /g'
find . -type f -name "*.scala" | xargs sed -i '' 's/ A RDD\./ An RDD\./g'
find . -type f -name "*.scala" | xargs sed -i '' 's/ a RDD / an RDD /g'
find . -type f -name "*.scala" | xargs sed -i '' 's/ a RDD\./ an RDD\./g'
find . -type f -name "*.java" | xargs sed -i '' 's/ A RDD / An RDD /g'
find . -type f -name "*.java" | xargs sed -i '' 's/ A RDD\./ An RDD\./g'
find . -type f -name "*.java" | xargs sed -i '' 's/ a RDD / an RDD /g'
find . -type f -name "*.java" | xargs sed -i '' 's/ a RDD\./ an RDD\./g'
find . -type f -name "*.r" | xargs sed -i '' 's/ A RDD / An RDD /g'
find . -type f -name "*.r" | xargs sed -i '' 's/ A RDD\./ An RDD\./g'
find . -type f -name "*.r" | xargs sed -i '' 's/ a RDD / an RDD /g'
find . -type f -name "*.r" | xargs sed -i '' 's/ a RDD\./ an RDD\./g'
find . -type f -name "*.py" | xargs sed -i '' 's/ A RDD / An RDD /g'
find . -type f -name "*.py" | xargs sed -i '' 's/ A RDD\./ An RDD\./g'
find . -type f -name "*.py" | xargs sed -i '' 's/ a RDD / an RDD /g'
find . -type f -name "*.py" | xargs sed -i '' 's/ a RDD\./ an RDD\./g'
find . -type f -name "*.md" | xargs sed -i '' 's/ A RDD / An RDD /g'
find . -type f -name "*.md" | xargs sed -i '' 's/ A RDD\./ An RDD\./g'
find . -type f -name "*.md" | xargs sed -i '' 's/ a RDD / an RDD /g'
find . -type f -name "*.md" | xargs sed -i '' 's/ a RDD\./ an RDD\./g'If there are more, we should fix. Actually, i was preparing a minor PR to fix all of them including the same instances such as a X.. to an X...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I also made this mistake and found actually an RDD is correct after googling :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you point where a RDD is? I will try to include them in a similar way above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, "an RDD" is correct because "a" precedes consonant sounds and "an" precedes vowel sounds. So it's "an arr-dee-dee".
We don't need to fix up every last instance of this. It was a good idea to fix it all while fixing some that overlapped with this PR though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I will include
./R/pkg/inst/tests/testthat/test_sparkSQL.R:test_that("jsonRDD() on a RDD with json string", {
./R/pkg/R/RDD.R:#' @param x A RDD.
It seems I should have used -name "*.R" instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Sure, let me just keep it and will leave some comments in the related PRs in the future if possible.
… that`/`'''Note:'''` across Scala/Java API documentation ## What changes were proposed in this pull request? It seems in Scala/Java, - `Note:` - `NOTE:` - `Note that` - `'''Note:'''` - `note` This PR proposes to fix those to `note` to be consistent. **Before** - Scala  - Java  **After** - Scala  - Java  ## How was this patch tested? The notes were found via ```bash grep -r "NOTE: " . | \ # Note:|NOTE:|Note that|'''Note:''' grep -v "// NOTE: " | \ # starting with // does not appear in API documentation. grep -E '.scala|.java' | \ # java/scala files grep -v Suite | \ # exclude tests grep -v Test | \ # exclude tests grep -e 'org.apache.spark.api.java' \ # packages appear in API documenation -e 'org.apache.spark.api.java.function' \ # note that this is a regular expression. So actual matches were mostly `org/apache/spark/api/java/functions ...` -e 'org.apache.spark.api.r' \ ... ``` ```bash grep -r "Note that " . | \ # Note:|NOTE:|Note that|'''Note:''' grep -v "// Note that " | \ # starting with // does not appear in API documentation. grep -E '.scala|.java' | \ # java/scala files grep -v Suite | \ # exclude tests grep -v Test | \ # exclude tests grep -e 'org.apache.spark.api.java' \ # packages appear in API documenation -e 'org.apache.spark.api.java.function' \ -e 'org.apache.spark.api.r' \ ... ``` ```bash grep -r "Note: " . | \ # Note:|NOTE:|Note that|'''Note:''' grep -v "// Note: " | \ # starting with // does not appear in API documentation. grep -E '.scala|.java' | \ # java/scala files grep -v Suite | \ # exclude tests grep -v Test | \ # exclude tests grep -e 'org.apache.spark.api.java' \ # packages appear in API documenation -e 'org.apache.spark.api.java.function' \ -e 'org.apache.spark.api.r' \ ... ``` ```bash grep -r "'''Note:'''" . | \ # Note:|NOTE:|Note that|'''Note:''' grep -v "// '''Note:''' " | \ # starting with // does not appear in API documentation. grep -E '.scala|.java' | \ # java/scala files grep -v Suite | \ # exclude tests grep -v Test | \ # exclude tests grep -e 'org.apache.spark.api.java' \ # packages appear in API documenation -e 'org.apache.spark.api.java.function' \ -e 'org.apache.spark.api.r' \ ... ``` And then fixed one by one comparing with API documentation/access modifiers. After that, manually tested via `jekyll build`. Author: hyukjinkwon <[email protected]> Closes apache#15889 from HyukjinKwon/SPARK-18437.


















What changes were proposed in this pull request?
It seems in Scala/Java,
Note:NOTE:Note that'''Note:'''@noteThis PR proposes to fix those to
@noteto be consistent.Before
Scala

Java

After
Scala

Java

How was this patch tested?
The notes were found via
And then fixed one by one comparing with API documentation/access modifiers.
After that, manually tested via
jekyll build.