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.

Sign up for GitHub

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

Jump to bottom

[SPARK-6740][SQL] Fix NOT operator precedence. #6326

Closed
wants to merge 1 commit into from
Closed

[SPARK-6740][SQL] Fix NOT operator precedence. #6326

wants to merge 1 commit into from

Conversation

smola
Copy link
Contributor

@smola smola commented May 21, 2015

NOT has lower precedence than comparison operations.

@smola smola force-pushed the feature/operator-precedence branch 2 times, most recently from 58a62a7 to 3d8bf9a Compare May 21, 2015 18:55
chenghao-intel
chenghao-intel reviewed May 25, 2015
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/SqlParser.scala
@@ -228,7 +228,12 @@ class SqlParser extends AbstractSparkSQLParser with DataTypeParser {
andExpression * (OR ^^^ { (e1: Expression, e2: Expression) => Or(e1, e2) })

protected lazy val andExpression: Parser[Expression] =
comparisonExpression * (AND ^^^ { (e1: Expression, e2: Expression) => And(e1, e2) })
booleanFactor * (AND ^^^ { (e1: Expression, e2: Expression) => And(e1, e2) })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of bind the rule booleanFactor with andExpression, a better place probably be expression rule. (https://github.com/smola/spark/blob/feature/operator-precedence/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/SqlParser.scala#L225)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chenghao-intel Not sure how. Adding the NOT clause in the expression rule would break precedence rules. Also, binding expression -> orExpression -> andExpression -> booleanFactor -> comparison is pretty much they way it is expressed in the grammars for standard SQL.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, my bad, I just picked up some corner case, it works.

@marmbrus
Copy link
Contributor

Sorry for the delay here. Mind bringing this up to date?

@smola smola force-pushed the feature/operator-precedence branch from 3d8bf9a to f9e9021 Compare June 18, 2015 13:59
@smola
Copy link
Contributor Author

smola commented Jun 18, 2015

@marmbrus Done.

@marmbrus
Copy link
Contributor

ok to test

@SparkQA
Copy link

SparkQA commented Jun 18, 2015

Test build #35151 has finished for PR 6326 at commit f9e9021.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@marmbrus
Copy link
Contributor

Minor style issues:

/home/jenkins/workspace/SparkPullRequestBuilder/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/SqlParser.scala:221:40: No space before token =>
[error] (catalyst/compile:scalastyle) errors exist
[error] Total time: 8 s, completed Jun 18, 2015 12:51:02 PM
[error] /home/jenkins/workspace/SparkPullRequestBuilder/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/SqlParser.scala:221:40: No space before token =>
[error] (catalyst/compile:scalastyle) errors exist

@smola smola force-pushed the feature/operator-precedence branch from f9e9021 to 75b7c95 Compare June 24, 2015 16:56
@smola
[SPARK-6740][SQL] Fix NOT operator precedence.
fbc1815 
NOT has lower precedence than comparison operations.
@smola smola force-pushed the feature/operator-precedence branch from 75b7c95 to fbc1815 Compare June 24, 2015 16:57
@SparkQA
Copy link

SparkQA commented Jun 24, 2015

Test build #35700 has finished for PR 6326 at commit 75b7c95.

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

@SparkQA
Copy link

SparkQA commented Jun 24, 2015

Test build #35701 has finished for PR 6326 at commit fbc1815.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • s"Using output committer class $
    • logInfo(s"Using user defined output committer class $
    • s"Using output committer class $

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@rxin
Copy link
Contributor

rxin commented Jul 18, 2015

@smola do you have time to fix the test failure so we can merge this for 1.5?

@marmbrus
Copy link
Contributor

marmbrus commented Sep 3, 2015

Thanks for working on this @smola. I propose we close this issue until you have time to fix the test failures.

@asfgit asfgit closed this in 804a012 Sep 4, 2015
@cloud-fan cloud-fan mentioned this pull request Sep 5, 2015
Closed
asfgit pushed a commit that referenced this pull request Oct 20, 2015
@cloud-fan @davies
[SPARK-6740] [SQL] correctly parse NOT operator with comparison opera…
478c7ce 
…tions

We can't parse `NOT` operator with comparison operations like `SELECT NOT TRUE > TRUE`, this PR fixed it.

Takes over #6326.

Author: Wenchen Fan <cloud0fan@outlook.com>

Closes #8617 from cloud-fan/not.
kiszk pushed a commit to kiszk/spark-gpu that referenced this pull request Dec 26, 2015
@cloud-fan @kiszk
[SPARK-6740] [SQL] correctly parse NOT operator with comparison opera…
db5b319 
…tions

We can't parse `NOT` operator with comparison operations like `SELECT NOT TRUE > TRUE`, this PR fixed it.

Takes over apache/spark#6326.

Author: Wenchen Fan <cloud0fan@outlook.com>

Closes #8617 from cloud-fan/not.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@smola @marmbrus @SparkQA @AmplabJenkins @rxin @chenghao-intel