Fixed coding style issues in Spark SQL

[liancheng]

This PR addresses various coding style issues in Spark SQL, including but not limited to those mentioned by @mateiz in PR #146.

As this PR affects lots of source files and may cause potential conflicts, it would be better to merge this as soon as possible after PR #205 (In-memory columnar representation for Spark SQL) is merged.

[liancheng]

All comments from @marmbrus were addressed, thanks for all the suggestions! Hope this can be a good foundation for future contributions :)

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[AmplabJenkins]

Merged build finished.

[AmplabJenkins]

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/13364/

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[liancheng]

@pwendell Merged most recent master, should be ready to be merged if Jenkins is happy.

[marmbrus]

LGTM

[AmplabJenkins]

Merged build finished.

[AmplabJenkins]

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/13372/

[pwendell]

Thanks, merged.

[rxin]

To make it consistent with the rest of Spark, can you also submit a PR to update the "package" definitions to use fully qualified packages?

[marmbrus]

They are fully qualified, as there is only only possible path that will define the package for a given file. For this reason I'm not sure if the logic behind avoiding relative imports applies here.

There are just breaks placed in to implicitly include things from the package object of parent packages. I think this is a useful construct to include system-wide things like logging or type aliases without adding verbosity to every file in the project.

[rxin]

package org.apache.spark.sql
package catalyst

vs

package org.apache.spark.sql.catalyst

There are three reasons I think it'd be better to just have one package statement in a file.

1. It is easier to understand by most programmers, especially the ones that come from Java land (I was chatting with another committer just now and he got confused by the meaning of having two or three package statements in a Scala file).
2. It requires explicit import to open up the parent package scope and avoids polluting the namespace (there is no difference in terms of line count here since you add one import but remove one package)
3. It is more consistent with the rest of Spark code base.

Now this is a highly subjective topic, so we should get others to chime in.

[pwendell]

Hey I just want to make sure I understand. It seems like there are two options here:

package org.apache.spark.sql
package catalyst

and

package org.apache.spark.sql.catalyst

import org.apache.spark.sql._

And these accomplish the same goal. Is that correct?

If the trade-off is to save ~15 characters vs having something more explicit, I definitely prefer just being explicit. At least in the past we've tried to err on the side of making things accessible.

[liancheng]

@pwendell When I started on this PR, I was also puzzled which option to use at first, until I saw the same usage in Scala standard library. But I should confess that I wasn't 100% sure about what the first option exactly mean until I played around both of them a bit.

I think the most significant "advantage" of the first option is that we open parent packages implicitly. But since we forbid relative package imports, this is not exactly an advantage any more. So I vote for the second option now.

[liancheng]

I think a point here is whether we should assume all contributors use IDEs like IntelliJ and their automated features. At least, I find in most scenarios, default behaviours of IntelliJ match Spark coding convention well. Exceptions include indentation and false positive suggestions about adding/removing parenthesis to/from Java getter/setter methods.

Maybe we can suggest developers (including ourselves) to rely on IDE more and even provide a default IntelliJ configuration that match Spark coding convention better?

[shivaram]

FWIW I'd vote for package org.apache.spark.sql.catalyst and not assuming that everybody uses an IDE

[aarondav]

Also vote for package org.apache.spark.sql.catalyst, for new developers and to keep consistent with the rest of the Spark codebase.
edit: prematurely submitted

[alig]

Also +1 for package org.apache.spark.sql.catalyst, just because it's simpler to understand for the majority of the programmers in the world ;)

[liancheng]

Thanks for all your votes! I'll fix this in a separate PR ASAP.