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-17013][SQL] Parse negative numeric literals #14608

Closed
wants to merge 3 commits into from

Conversation

petermaxlee
Copy link
Contributor

What changes were proposed in this pull request?

This patch updates the SQL parser to parse negative numeric literals as numeric literals, instead of unary minus of positive literals.

This allows the parser to parse the minimal value for each data type, e.g. "-32768S".

How was this patch tested?

Updated test cases.

@SparkQA
Copy link

SparkQA commented Aug 11, 2016

Test build #63634 has finished for PR 14608 at commit 1ae7a3b.

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

@petermaxlee
Copy link
Contributor Author

petermaxlee commented Aug 11, 2016

I found the SQL generation for expression pretty weird with parentheses.

The current design adds parentheses at the outside of each expression. For example, "1+2" becomes "(1+2)". There was this subtle bug in UnaryMinus for negative numeric literal, as the generated SQL becomes "--number", which is a comment in SQL. I fixed it here by injecting a space.

It might be better to change these to put parentheses in the inside, e.g.

class Add {
  def sql = wrap(left) + " + " + wrap(right) 
}

class UnaryMinus {
  def sql = s"-($child.sql)"
}

def wrap(node) = if node is Literal node.sql else s"(${node.sql})"

@cloud-fan, @hvanhovell, @liancheng what do you think?

@SparkQA
Copy link

SparkQA commented Aug 12, 2016

Test build #63644 has finished for PR 14608 at commit 154abba.

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

@cloud-fan
Copy link
Contributor

adding a space LGTM. I think UnaryMinus is the only special case? Adding parentheses at the outside of each expression should work for all other cases.

@SparkQA
Copy link

SparkQA commented Aug 12, 2016

Test build #63652 has finished for PR 14608 at commit 908253b.

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

@cloud-fan
Copy link
Contributor

LGTM

@rxin
Copy link
Contributor

rxin commented Aug 12, 2016

Shouldn't we wrap literals with () to be safe?

@rxin
Copy link
Contributor

rxin commented Aug 12, 2016

I'm going to merge this one in master/2.0.

asfgit pushed a commit that referenced this pull request Aug 12, 2016
## What changes were proposed in this pull request?
This patch updates the SQL parser to parse negative numeric literals as numeric literals, instead of unary minus of positive literals.

This allows the parser to parse the minimal value for each data type, e.g. "-32768S".

## How was this patch tested?
Updated test cases.

Author: petermaxlee <petermaxlee@gmail.com>

Closes #14608 from petermaxlee/SPARK-17013.

(cherry picked from commit 00e103a)
Signed-off-by: Reynold Xin <rxin@databricks.com>
@asfgit asfgit closed this in 00e103a Aug 12, 2016
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.

4 participants