Skip to content

[SQL] Kill dangerous trailing space in query string #2619

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

Closed
wants to merge 1 commit into from

Conversation

liancheng
Copy link
Contributor

MD5 of query strings in createQueryTest calls are used to generate golden files, leaving trailing spaces there can be really dangerous. Got bitten by this while working on #2616: my "smart" IDE automatically removed a trailing space and makes Jenkins fail.

(Really should add "no trailing space" to our coding style guidelines!)

@SparkQA
Copy link

SparkQA commented Oct 1, 2014

QA tests have started for PR 2619 at commit 034f119.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 1, 2014

QA tests have finished for PR 2619 at commit 034f119.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/21133/

@marmbrus
Copy link
Contributor

marmbrus commented Oct 1, 2014

Good catch! Merged to master.

@asfgit asfgit closed this in 8cc70e7 Oct 1, 2014
@nchammas
Copy link
Contributor

nchammas commented Oct 1, 2014

(Really should add "no trailing space" to our coding style guidelines!)

FWIW, pep8 is enforcing this rule for us for our Python codebase.

@marmbrus
Copy link
Contributor

marmbrus commented Oct 1, 2014

+1 @nchammas

I'd be in favor of adding such a check to our scala style checks (though I think it won't fit into the current framework and would have to be done separately). Perhaps suggest this on the dev list?

@liancheng liancheng deleted the kill-trailing-space branch October 2, 2014 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants