Skip to content

Conversation

@Wenpei
Copy link
Contributor

@Wenpei Wenpei commented Jan 5, 2016

Currently, RDD function aggregate's parameter doesn't explain well, especially parameter "zeroValue".
It's helpful to let junior scala user know that "zeroValue" attend both "seqOp" and "combOp" phase.

@rxin
Copy link
Contributor

rxin commented Jan 5, 2016

Jenkins, test this please.

@srowen
Copy link
Member

srowen commented Jan 5, 2016

I like this, but how about adding similar docs to other similar methods like treeAggregate, fold, etc? the semantics of fold were brought up just last week, for example. Your point about it being per-partition is quite pertinent there too.

@Wenpei
Copy link
Contributor Author

Wenpei commented Jan 6, 2016

Thanks, @srowen I will try to add parameter explanation to fold.
But for treeAggregate, it has explanation like below to refer to Aggregate function. I thought it's enough for user.

  • Aggregates the elements of this RDD in a multi-level tree pattern.
    *
  • @param depth suggested depth of the tree (default: 2)
  • @see [[org.apache.spark.rdd.RDD#aggregate]]

@Wenpei Wenpei changed the title [SPARK-12638] [API DOC] Parameter explaination not very accurate for rdd function "aggregate" [SPARK-12638] [API DOC] Parameter explanation not very accurate for rdd function "aggregate" Jan 6, 2016
@srowen
Copy link
Member

srowen commented Jan 6, 2016

LGTM

@Wenpei
Copy link
Contributor Author

Wenpei commented Jan 6, 2016

thanks,@srowen
@rxin There is no Jenkins report, anything need for me?

@SparkQA
Copy link

SparkQA commented Jan 6, 2016

Test build #2334 has finished for PR 10587 at commit 35b4bc8.

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

@Wenpei
Copy link
Contributor Author

Wenpei commented Jan 7, 2016

Fix scala style test failed.
Please test again, thanks.

@SparkQA
Copy link

SparkQA commented Jan 7, 2016

Test build #2345 has finished for PR 10587 at commit 679e62f.

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

@Wenpei
Copy link
Contributor Author

Wenpei commented Jan 7, 2016

Fix it.
But I found for jenkins result, line number did not march with correct line.

For example, jenkins result
[error] /home/jenkins/workspace/NewSparkPullRequestBuilder/core/src/main/scala/org/apache/spark/rdd/RDD.scala:976:58: Whitespace at end of line

But real line number should be 1077. Is that a issues?

Please test again, jenkins.

@srowen
Copy link
Member

srowen commented Jan 7, 2016

It looks unrelated to your change, I agree. I can try retesting, but you may somehow need a rebase to work around it.

@SparkQA
Copy link

SparkQA commented Jan 7, 2016

Test build #2346 has finished for PR 10587 at commit 11a929b.

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

@Wenpei
Copy link
Contributor Author

Wenpei commented Jan 7, 2016

@srowen it pass test now. ready for merge.

Thanks for review.

Wenpei

Copy link
Member

Choose a reason for hiding this comment

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

There are some typos now, like conbOp instead of combOp. Also there's a dangling phrase in both starting with "-" which is supposed to join the previous sentence? while you're here you can back-tick Nil

@Wenpei
Copy link
Contributor Author

Wenpei commented Jan 11, 2016

Thank you @srowen, I refine parameter explanation base on your suggestion.
Pls take a look.

@SparkQA
Copy link

SparkQA commented Jan 11, 2016

Test build #2361 has finished for PR 10587 at commit d045ea9.

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

@Wenpei
Copy link
Contributor Author

Wenpei commented Jan 11, 2016

@srowen Do you know what happened?
I check the failed log but don't know why. EVN issues?

@SparkQA
Copy link

SparkQA commented Jan 11, 2016

Test build #2363 has finished for PR 10587 at commit d045ea9.

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

@srowen
Copy link
Member

srowen commented Jan 11, 2016

I can't imagine it's related as it's just a doc change, so must be flaky tests, but let me just run it again

@SparkQA
Copy link

SparkQA commented Jan 11, 2016

Test build #2364 has finished for PR 10587 at commit d045ea9.

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

@Wenpei
Copy link
Contributor Author

Wenpei commented Jan 12, 2016

Same error with #10685. Wait hot fix #10704 take effect.

@Wenpei
Copy link
Contributor Author

Wenpei commented Jan 12, 2016

retest this please.

@SparkQA
Copy link

SparkQA commented Jan 12, 2016

Test build #2369 has finished for PR 10587 at commit d045ea9.

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

@srowen
Copy link
Member

srowen commented Jan 12, 2016

merged to master/1.6

asfgit pushed a commit that referenced this pull request Jan 12, 2016
…d function "aggregate"

Currently, RDD function aggregate's parameter doesn't explain well, especially parameter "zeroValue".
It's helpful to let junior scala user know that "zeroValue" attend both "seqOp" and "combOp" phase.

Author: Tommy YU <tummyyu@163.com>

Closes #10587 from Wenpei/rdd_aggregate_doc.

(cherry picked from commit 9f0995b)
Signed-off-by: Sean Owen <sowen@cloudera.com>
@asfgit asfgit closed this in 9f0995b Jan 12, 2016
@Wenpei Wenpei deleted the rdd_aggregate_doc branch January 13, 2016 05:36
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.

4 participants