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

[SPARK-17308]Improved the spark core code by replacing all pattern match on boolean value by if/else block. #14873

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
6 participants
@shiv4nsh
Copy link
Contributor

commented Aug 30, 2016

What changes were proposed in this pull request?

Improved the code quality of spark by replacing all pattern match on boolean value by if/else block.

How was this patch tested?

By running the tests

@@ -44,7 +44,9 @@ object Main extends Logging {

private def scalaOptionError(msg: String): Unit = {
hasErrors = true
//scalastyle:off

This comment has been minimized.

Copy link
@srowen

srowen Aug 30, 2016

Member

Is this needed / related? I thought we maybe didn't check style on this file because it's copied from scala

This comment has been minimized.

Copy link
@shiv4nsh

shiv4nsh Aug 30, 2016

Author Contributor

@srowen : In the IDE it was showing me error hence I put those there , if you suggest we can remove it

This comment has been minimized.

Copy link
@srowen

srowen Sep 2, 2016

Member

Yeah revert these. I am not sure we need to check or modify this file because it's a clone of Scala's code

@srowen

This comment has been minimized.

Copy link
Member

commented Aug 30, 2016

Jenkins test this please

@srowen

This comment has been minimized.

Copy link
Member

commented Aug 30, 2016

@shiv4nsh shiv4nsh changed the title Imporoved the spark core code by replacing all pattern match on boolean value by if/else block. [SPARK-17308]Imporoved the spark core code by replacing all pattern match on boolean value by if/else block. Aug 30, 2016

@shiv4nsh

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2016

@srowen : I have added the JIRA ticket in the title but I am confused for the component as it has changes for the sql , mllib, streaming and core .

@shiv4nsh shiv4nsh changed the title [SPARK-17308]Imporoved the spark core code by replacing all pattern match on boolean value by if/else block. [SPARK-17308]Improved the spark core code by replacing all pattern match on boolean value by if/else block. Aug 30, 2016

@SparkQA

This comment has been minimized.

Copy link

commented Aug 30, 2016

Test build #64648 has finished for PR 14873 at commit 1231164.

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

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2016

Why should we do this? What is wrong with a pattern match on a boolean? It is not like it is hard to understand what is going on.

@srowen

This comment has been minimized.

Copy link
Member

commented Aug 30, 2016

Yeah, it's not a big deal either way. I am not sure why you'd ever pattern-match a boolean though, as it's a more verbose and Scala-specific way of expressing the universally-understood if-else. It's harder to read and infinitesimally slower. I wouldn't go to the mat for this change, but it's within bounds of cleanups I'd favor.

@shiv4nsh

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2016

@srowen : We are good on merging on this Right? or do this PR require some additional changes !

@srowen

This comment has been minimized.

Copy link
Member

commented Aug 31, 2016

No, I'd leave it open for some time for comments in any event, especially if someone has expressed doubts about it. I'd like to wait some time for more comments before considering merging it.

@jerryshao

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2016

From my understanding it is more like a personal preference rather than code style issue. We may change the code for now, but how can we guarantee other people not to use pattern match in future? So IMO if we want to fix this, it would be better to add this as a rule for style check, though personally I don't think this change is so necessary.

@srowen

This comment has been minimized.

Copy link
Member

commented Sep 1, 2016

Although I'm slightly positive on it, I would not merge if there are two slightly negative reviews. I think it's a bit more than style preference, but not much more. Is there ever a benefit to pattern-matching a boolean?

@shiv4nsh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2016

@jerryshao : It is always better to not to use the pattern matching on the boolean AFAIK , and it reduces the bytecode too.. you can take a look here: http://stackoverflow.com/questions/9266822/pattern-matching-vs-if-else where Rex Kerr has explained it for the better understanding !

}

This comment has been minimized.

Copy link
@BryanCutler

BryanCutler Sep 1, 2016

Contributor

nit: remove empty line

assertAnalysisSuccess(plan, true)
case false =>
assertAnalysisError(plan, "expression `a` cannot be used as a grouping expression" :: Nil)

This comment has been minimized.

Copy link
@BryanCutler

BryanCutler Sep 1, 2016

Contributor

nit: remove empty line

assertAnalysisSuccess(plan, true)
} else {
assertAnalysisError(plan, "expression `a` cannot be used as a grouping expression" :: Nil)

This comment has been minimized.

Copy link
@BryanCutler

BryanCutler Sep 1, 2016

Contributor

nit: remove empty line

@shiv4nsh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2016

@BryanCutler : I have made the changes according to your comments ! Please re-verify the PR thanks :)

@shiv4nsh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2016

@srowen : Do we have to retest this build as I have done the changes implied by Byran !

@shiv4nsh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2016

@srowen : I have updated the PR according to your comments please review it ! Thanks

@srowen

This comment has been minimized.

Copy link
Member

commented Sep 2, 2016

Jenkins retest this please

@SparkQA

This comment has been minimized.

Copy link

commented Sep 2, 2016

Test build #64846 has finished for PR 14873 at commit 2141b1b.

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2016

@srowen : Can you please help me out here ? Why this test case fails, it did not fail on my local and my last changes include only removing space and comments ! Please help me out here !

@hvanhovell

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2016

restest this please

@hvanhovell

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2016

I am not really against this. I am just not a big fan of cosmetic code changes. The whole byte code argument would have made sense if this would be touching performance critical code.

I'll let Sean sign off on this one.

@SparkQA

This comment has been minimized.

Copy link

commented Sep 2, 2016

Test build #64851 has finished for PR 14873 at commit 2141b1b.

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2016

@srowen : Can we merge this now ? or does it require any additional changes ?

@srowen

This comment has been minimized.

Copy link
Member

commented Sep 4, 2016

I'm going to merge this for reasons above. I think it's more than a cosmetic change, but barely.

@asfgit asfgit closed this in e75c162 Sep 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.