You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by srowen <gi...@git.apache.org> on 2014/08/02 02:07:51 UTC
[GitHub] spark pull request: SPARK-2799 [CORE] Simplify some Scala operatio...
GitHub user srowen opened a pull request:
https://github.com/apache/spark/pull/1728
SPARK-2799 [CORE] Simplify some Scala operations for clarity, performance
For fun, here's a last minor suggestion for consideration before the 1.1 window closes.
There are a number of instances where Scala operations can be simplified, for clarity or even performance.
For example `getOrElse(null)` can be `orNull`. `filter(...).size` can be faster as `count(...)`. An expression like `!...someLongChain...isEmpty` can be `... .nonEmpty`. And so on.
These are from code inspections. The PR shows the changes broken down by type in separate commits. They should be quite safe.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/srowen/spark SPARK-2799
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/1728.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 #1728
----
commit 274df1bf9656f29f79d1b28d28e92a91b3c0b59e
Author: Sean Owen <sr...@gmail.com>
Date: 2014-08-01T21:09:59Z
getOrElse(null) -> orNull
commit fc407e65493cd17498d78d635a92b11907094eb0
Author: Sean Owen <sr...@gmail.com>
Date: 2014-08-01T21:10:56Z
reduce(_+_) -> sum
commit 76697a5b1a1e9d1b61849bf9c7bac3e407283216
Author: Sean Owen <sr...@gmail.com>
Date: 2014-08-01T21:11:15Z
filter + size -> count
commit 9ba0617507b702c18ef4c59c0607a9a65aeebdde
Author: Sean Owen <sr...@gmail.com>
Date: 2014-08-01T21:11:38Z
fold -> forall
commit 937fb8a769c37e217988671e01e0b9a6bdc56ac6
Author: Sean Owen <sr...@gmail.com>
Date: 2014-08-01T21:12:00Z
exists + == -> contains
commit aee8e2e048558ffa3c4748054d401b8b2a0af2dd
Author: Sean Owen <sr...@gmail.com>
Date: 2014-08-01T21:12:23Z
filter + headOption -> find
commit 408aea5f7ff5509f0451c991a59657d04a328e2f
Author: Sean Owen <sr...@gmail.com>
Date: 2014-08-01T21:12:45Z
map + getOrElse -> exists
commit 3014ef3de6d87999a3cb14511bfc2a7af3047fad
Author: Sean Owen <sr...@gmail.com>
Date: 2014-08-01T21:14:10Z
get + getOrElse -> getOrElse
commit fe1e97a332c006a4d2b90d29773ed2e71287bd92
Author: Sean Owen <sr...@gmail.com>
Date: 2014-08-01T21:15:58Z
! isEmpty -> nonEmpty
commit 9bd5e94b9ca15fb9d65e7e6d33c238e1d16cc91f
Author: Sean Owen <sr...@gmail.com>
Date: 2014-08-01T21:33:57Z
Revert change of Scala REPL class
commit 5981a116f603b37a0d1e2aea4053b958a928e3fa
Author: Sean Owen <sr...@gmail.com>
Date: 2014-08-01T21:38:57Z
Remove unused imports
commit 9428223e55bda043022a1c22ae65f8662a84b2d4
Author: Sean Owen <sr...@gmail.com>
Date: 2014-08-01T21:56:31Z
filter + nonEmpty -> exists
----
---
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.
---
[GitHub] spark pull request: SPARK-2799 [CORE] Simplify some Scala operatio...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/1728#issuecomment-50972433
Yup, I think cherry picking is a good point. The changes are trivial but it can be non-obvious later when picking apart why a merge conflict happens. Even a little hassle may not be worth it.
I had hoped to be able to zap some of these cross-cutting improvements right before a release as a good number are flagged in my IDE, but it may be at a velocity now where that's not feasible and too late for that. In any event, these can be reproduced automatically later if desired.
Yes definitely something to attack while editing nearby code anyway.
---
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-2799 [CORE] Simplify some Scala operatio...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1728#issuecomment-50958171
QA tests have started for PR 1728. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17765/consoleFull
---
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-2799 [CORE] Simplify some Scala operatio...
Posted by srowen <gi...@git.apache.org>.
Github user srowen closed the pull request at:
https://github.com/apache/spark/pull/1728
---
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-2799 [CORE] Simplify some Scala operatio...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1728#issuecomment-50950494
QA results for PR 1728:<br>- This patch PASSES unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17723/consoleFull
---
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.
---
[GitHub] spark pull request: SPARK-2799 [CORE] Simplify some Scala operatio...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/1728#issuecomment-50958152
So true, in fact, there were already 2 conflicts since I submitted it. Easy to fix, but still conflicts. I wouldn't commit this until important changes are already tucked in.
---
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-2799 [CORE] Simplify some Scala operatio...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1728#issuecomment-50947803
QA tests have started for PR 1728. This patch merges cleanly. <br>View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17723/consoleFull
---
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.
---
[GitHub] spark pull request: SPARK-2799 [CORE] Simplify some Scala operatio...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/1728#issuecomment-50959372
QA results for PR 1728:<br>- This patch FAILED unit tests.<br>- This patch merges cleanly<br>- This patch adds no public classes<br><br>For more information see test ouptut:<br>https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17765/consoleFull
---
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-2799 [CORE] Simplify some Scala operatio...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/1728#issuecomment-50950861
This looks technically sound, but I'm concerned that it might cause merge conflicts with outstanding pull requests if we were to merge it now.
---
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.
---
[GitHub] spark pull request: SPARK-2799 [CORE] Simplify some Scala operatio...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/1728#issuecomment-50969004
Even if we manage to merge this into master without causing a bunch of PR merge conflicts, it would still lead to conflicts when cherry-picking changes into maintenance release branches. Since this seems like it might lead to more maintenance headaches, I'm -1 on this PR as it stands now.
These are technically good fixes and I'd still encourage folks to perform this type of cleanup in code that they're modifying anyways as part of larger PRs.
---
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