[SPARK-20316][SQL] Val and Var should strictly follow the Scala syntax

[ouyangxiaochen]

What changes were proposed in this pull request?

val and var should strictly follow the Scala syntax

How was this patch tested?

manual test and exisiting test cases

[gatorsmile]

ok to test

[viirya]

This is very old code. Looks this change is okay. cc @liancheng

[HyukjinKwon]

It is too trivial as a separate PR and I guess we don't need a JIRA for this change. I think we should find the similar instances at least (I don't think we want swarming PRs fixing such things). Also, I think we can fold #17629 into this here.

[viirya]

It's always good to merge many such changes together into one single PR.

[SparkQA]

Test build #75759 has finished for PR 17628 at commit 32b12a0.

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

[ouyangxiaochen]

OK, I'll merge such changes together into this PR.

[SparkQA]

Test build #75762 has started for PR 17628 at commit cef0405.

[srowen]

I would support this but only if we try to fix all such instances. The val/var changes has some value. However you're also removing 'return' and while that's probably OK as cleanup it's not actually improving much, so am not sure I'd do that.

[ouyangxiaochen]

This is a very heavy task. So far, I found a problem with this when I was reading SparkSQL source code.
So, whether we need to merge to the master or not firstly? @srowen @viirya
As for the keyword "return", I found that there are so many scala methods use this keyword. So, this problem can be reverted to the origin.

[srowen]

Yeah, I'm surprised IntelliJ doesn't have an inspection for it. It's otherwise quite hard to find instances that can be changed.

I would look for instances of var foo = "string" as these are often instances where someone meant to declare a constant. There are about 100 of those. If you scan those and address any issues you see, that's a pretty good effort.

IntelliJ can find all the return instances, and there are 39 of them. I'm kind of neutral on cleaning that up, but it would be nice. They occur in places that don't seem likely to cause merge conflicts. Maybe OK to finally touch it up, OK.

[SparkQA]

Test build #75773 has finished for PR 17628 at commit b259471.

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

[ouyangxiaochen]

I tried to serach the instance of var foo = "string" with the RegEx var [a-z|A-Z] = "* in whole project and there are 740 of them. The usage of them are almost reasonable.
As @viirya commented above, this is very old code. maybe no committer attention it . @srowen

[srowen]

(You're matching no quotes. Match at least one. It will give you a smaller set to review.)

[ouyangxiaochen]

OK, I will work on this again, Thanks.

[ouyangxiaochen]

I serached the instance of var foo = "string" with the RegEx \s*var [a-z|A-Z]* = "*" in whole project and there are 31 of them. The usage of them are almost reasonable except the file SparkSQLCLIDriver.
@srowen cc

[viirya]

I will be surprised if we find much more of this in current branch. :-)

[srowen]

@ouyangxiaochen that still isn't the right regex. Maybe there are no other instances but I'd ask for a minimal effort to find similar instances

[ouyangxiaochen]

@srowen Do u mean that my regex can not match all variable name? because scala's variable name can be consisted with letter|digital|_|$? I serached the instances of var foo="string" with new regex \s*var [a-z|A-Z|$|_]* = "*" and found 33 of them. Maybe really there are no other instances.

[srowen]

"*" does not mean what you intend -- you mean ".*" at least, among other things. It's maybe not worth pursuing further, but that's the kind of quick check I'd expect when proposing to adjust one small instance of something.

[ouyangxiaochen]

OK, I got it.

[srowen]

Merged to master