You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by HyukjinKwon <gi...@git.apache.org> on 2015/11/12 08:27:07 UTC
[GitHub] spark pull request: [SPARK-11687] Mixed usage of fold and foldLeft...
GitHub user HyukjinKwon opened a pull request:
https://github.com/apache/spark/pull/9655
[SPARK-11687] Mixed usage of fold and foldLeft, reduce and reduceLeft and reduceOption and reduceLeftOption
https://issues.apache.org/jira/browse/SPARK-11687
As can be seen here https://github.com/scala/scala/blob/2.12.x/src/library/scala/collection/TraversableOnce.scala, `fold`, `reduce` and `reduceOption` came out from scala 2.9.x. In addition, all the implementations of `fold`, `reduce` and `reduceOption` call actually `foldLeft`, `reduceLeft` and `reduceLeftOption` directly without any additional codes.
For developers who are not used to scala, the mixed usages of them might leave a question, for example, if it works as reduceRight or reduceLeft (as all know, the direction can change the results).
Even some usages can be found where they work identically the same things.
This is just a little nit but I think it would be better to use only `xxxLeft` for readability.
Fortunately, there are not so many usages of `xxx` so in this PR, I changed them to `xxxLeft`
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/HyukjinKwon/spark SPARK-11687
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/9655.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #9655
----
commit 7949222219ffdba242595bf3ad14dd9c53f68b82
Author: hyukjinkwon <gu...@gmail.com>
Date: 2015-11-12T07:19:35Z
[SPARK-11687][SQL] Mixed usage of fold and foldLeft, reduce and reduceLeft and reduceOption and reduceLeftOption
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11687] Mixed usage of fold and foldLeft...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon closed the pull request at:
https://github.com/apache/spark/pull/9655
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11687] Mixed usage of fold and foldLeft...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/9655#issuecomment-156159712
It's possible that the differing usages are intentional. Calling `reduce` makes sense for a truly commutative operation; calling `reduceLeft` suggests that the operation is not commutative and so an order must be specified.
So, I don't think in general you would _always_ call `reduce` _or_ `reduceLeft|Right`. There are usages for both. Therefore I don't think most of these changes are in the right direction as you're changing things like `reduce(_ || _)` to `reduceLeft(_ || _)`. It actually precludes the implementation from doing it in an optimal or parallel order (though it won't, in practice, in Scala).
(As an aside I suppose that particular first example is an instance of pointlessly verbose Scala -- `f.map(...).reduce(_ || _)` is just `f.exists(...)` but don't fix it.)
Can you find examples where `reduceLeft` is called but `reduce` is appropriate? _that_ could be a useful standardization.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11687] Mixed usage of fold and foldLeft...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the pull request:
https://github.com/apache/spark/pull/9655#issuecomment-156262080
I think I should have opened a thread in the mailing list. Sorry, closing this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11687] Mixed usage of fold and foldLeft...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9655#issuecomment-156024879
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11687] Mixed usage of fold and foldLeft...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9655#issuecomment-156058227
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/45720/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11687] Mixed usage of fold and foldLeft...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/9655#issuecomment-156025625
**[Test build #45720 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/45720/consoleFull)** for PR 9655 at commit [`7949222`](https://github.com/apache/spark/commit/7949222219ffdba242595bf3ad14dd9c53f68b82).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11687] Mixed usage of fold and foldLeft...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/9655#issuecomment-156250766
Do you mind closing this PR?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11687] Mixed usage of fold and foldLeft...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/9655#issuecomment-156083460
`fold` is not semantically the same as `foldLeft`. They have different signatures. `foldLeft` implies the order of the fold matters, and it mostly does not. Therefore writing it that way is less clear IMHO. I would not make this change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11687] Mixed usage of fold and foldLeft...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9655#issuecomment-156058225
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11687] Mixed usage of fold and foldLeft...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/9655#issuecomment-156058056
**[Test build #45720 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/45720/consoleFull)** for PR 9655 at commit [`7949222`](https://github.com/apache/spark/commit/7949222219ffdba242595bf3ad14dd9c53f68b82).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11687] Mixed usage of fold and foldLeft...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the pull request:
https://github.com/apache/spark/pull/9655#issuecomment-156087424
I agree with you. However, if you see the codes, they are used in a mixed way, even in the same class.
Mostly the usages are `_ and _` or ` + ` but `xxx` and `xxxLeft` are used in a mixed way even in the same class. Namely it looks this does not infer that the order is a matter.
If the order is apparently not a matter, then I think we better set all `xxx` instead of `xxxLeft`.
Anyhow, I would like to close if guys think this is inappropriate.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11687] Mixed usage of fold and foldLeft...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/9655#issuecomment-156223468
I'm inclined to agree with Sean, this seems like code churn for the sake of code churn.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-11687] Mixed usage of fold and foldLeft...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/9655#issuecomment-156024895
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org