Fix parsing of negative values #1419

agscpp · 2024-09-09T11:32:48Z

Currently, when passing negative numbers, the sql parser returns an error. While the PostgreSQL syntax allows you to specify negative values when creating a sequence.

Example:

CREATE SEQUENCE seq START -100 INCREMENT -10 MINVALUE -1000 MAXVALUE 1;

Error text:

ParserError("Expected: a value, found: -")

Link to PostgreSQL documentation:
https://www.postgresql.org/docs/current/sql-createsequence.html

agscpp · 2024-09-10T08:08:03Z

Hi @alamb .

Can you please take a look at my pull request?

git-hulk · 2024-09-10T09:00:41Z

src/parser/mod.rs

@@ -7274,6 +7274,16 @@ impl<'a> Parser<'a> {
                let placeholder = tok.to_string() + &ident.value;
                Ok(Value::Placeholder(placeholder))
            }
+            tok @ Token::Minus => {


Should handle + as well? And for the sign number value, another place represents it with UnaryOp, guess should keep consistent? cc @iffyio

Code that can handle prefixes is not well suited for handling numbers.

I added a handler for "+"

Oh, what I mean is if we should return the negative number as the UnaryOp expression like the previous implementation.

git-hulk · 2024-09-10T09:01:46Z

tests/sqlparser_postgres.rs

@@ -277,6 +277,16 @@ fn parse_create_sequence() {
        "CREATE TEMPORARY SEQUENCE IF NOT EXISTS name3 INCREMENT 1 NO MINVALUE MAXVALUE 20 OWNED BY NONE",
    );

+    let sql7 = "CREATE SEQUENCE name4


This change didn't guard with the PostgreSQL dialect guard, so should move test case to common.

Can I put the entire "parse_create_sequence" function in a common module?

I think it should be good to keep it inside PostgreSQL unless more dialects support this syntax.

alamb · 2024-09-10T11:27:44Z

src/parser/mod.rs

@@ -7274,6 +7274,16 @@ impl<'a> Parser<'a> {
                let placeholder = tok.to_string() + &ident.value;
                Ok(Value::Placeholder(placeholder))
            }
+            tok @ Token::Minus | tok @ Token::Plus => {


Normally I would expect that the - is included as part of the token itself otherwise negative numbers wouldn't be able to be parsed at all anywhere

I wonder if the issue is that the of token being handled by create sequence needs to be expanded 🤔

@alamb Not sure if I understand your point correctly. token::Minus might be a sign symbol for a number or a binary operation, and this cannot be determined when parsing it into a token.

My primary concern is that this change seems to have much larger potential scope than simply supporting negative numbers for CREATE SEQUENCE -- it seems like it would change parsing of all values.

However, I am clearly confused -- I added a test that shows that -10 is parsed as -(10) (aka a unary minus). See #1421. That test still passes on this PR, so it must not have as much of an impact as I thought

My primary concern is that this change seems to have much larger potential scope than simply supporting negative numbers for CREATE SEQUENCE -- it seems like it would change parsing of all values.

It did affect the parsing of all values.

However, I am clearly confused -- I added a test that shows that -10 is parsed as -(10) (aka a unary minus). See #1421. That test still passes on this PR, so it must not have as much of an impact as I thought

Yes, I also mentioned this in the comment #1419 (comment), the negative number will be parsed into an unary operation. So I'm wondering if we should keep it consistent by parsing it as an unary operation in PR as well.

It seems when parsing expressions, the signed number literals are handled by the precedence logic when looking to parse a sub expr here is where we usually land. From what I could tell, we don't end up hitting the new code when parsing expressions due to calling parse_prefix beforehand - though I can't tell how well the change works with dialect overrides to the precedence logic so there might be some potential for surprising behavior. Also can't say for sure if there are other considerations other than when parsing subexpressions

The doc for Token::Number at least explicitly says it contains unsigned number and since that's what Value::Number maps to, I think it might be reasonable to continue to always represent signed numbers as unary Exprs vs sometimes baked into the Value.

For this PR, issue seems that the clauses in CREATE SEQUENCE specifically are looking to parse number literals but the parse_number_value helper currently looks to parse exactly a Value::Number, maybe they can instead be updated to call parse_expr, expecially since the expected types on the enum are already Expr.

There are few similar calls to parse_number_value() I suspect they could have similar issue of rejecting signed numbers, so if we end up keeping the current behavior, a follow up might be to make it explicit what parse_number_value returns/rejects.

Thanks for your important comments. I have made some edits. Please check again.

@iffyio @alamb @git-hulk

alamb · 2024-09-10T11:28:01Z

I started the CI tests and there appear to be some problems with this PR

coveralls · 2024-09-10T11:29:16Z

Pull Request Test Coverage Report for Build 10881901043

Details

24 of 27 (88.89%) changed or added relevant lines in 3 files are covered.
12 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.007%) to 89.336%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser/mod.rs	13	14	92.86%
tests/sqlparser_postgres.rs	4	6	66.67%

Files with Coverage Reduction	New Missed Lines	%
src/parser/mod.rs	12	93.33%

Totals
Change from base Build 10817360923:	0.007%
Covered Lines:	29446
Relevant Lines:	32961

💛 - Coveralls

alamb

Thank you @agscpp -- this is a very nice contribution.

I think this PR needs a test with the originally reported SQL error (and some examples without spaces between minus signs) but is otherwise good to go from my perspective.

I would appreciate someone else taking a look too (cc @iffyio @jmhain ) as I can't stop worrying this will have unintended consequences

alamb · 2024-09-10T20:02:47Z

tests/sqlparser_common.rs

@@ -2819,6 +2819,29 @@ fn parse_window_function_null_treatment_arg() {
    );
 }

+#[test]
+fn parse_signed_value() {
+    let sql1 = "CREATE SEQUENCE name1


alamb · 2024-09-10T20:16:19Z

src/parser/mod.rs

@@ -7274,6 +7274,16 @@ impl<'a> Parser<'a> {
                let placeholder = tok.to_string() + &ident.value;
                Ok(Value::Placeholder(placeholder))
            }
+            tok @ Token::Minus | tok @ Token::Plus => {


My primary concern is that this change seems to have much larger potential scope than simply supporting negative numbers for CREATE SEQUENCE -- it seems like it would change parsing of all values.

However, I am clearly confused -- I added a test that shows that -10 is parsed as -(10) (aka a unary minus). See #1421. That test still passes on this PR, so it must not have as much of an impact as I thought

alamb · 2024-09-10T20:28:02Z

tests/sqlparser_common.rs

+        "CREATE SEQUENCE name2 AS BIGINT INCREMENT 10 MINVALUE 30 MAXVALUE 5000 START WITH 45",
+    );
+}
+


Can you also please add a test for your original reproducer?

CREATE SEQUENCE seq START -100 INCREMENT -10 MINVALUE -1000 MAXVALUE 1;

I tried adding it locally and it doesn't seem to pass for me

CREATE SEQUENCE seq START -100 INCREMENT -10 MINVALUE -1000 MAXVALUE 1;: ParserError("Expected: end of statement, found: INCREMENT") thread 'parse_create_sequence' panicked at src/test_utils.rs:119:61: CREATE SEQUENCE seq START -100 INCREMENT -10 MINVALUE -1000 MAXVALUE 1;: ParserError("Expected: end of statement, found: INCREMENT") stack backtrace:

I tested like this:

andrewlamb@Andrews-MacBook-Pro-2:~/Software/sqlparser-rs$ git diff 99b98ca86cf0591a89a644e06742758a70aa04ea diff --git a/tests/sqlparser_postgres.rs b/tests/sqlparser_postgres.rs index 1ebb5d5..89aee71 100644 --- a/tests/sqlparser_postgres.rs +++ b/tests/sqlparser_postgres.rs @@ -277,6 +277,14 @@ fn parse_create_sequence() { "CREATE TEMPORARY SEQUENCE IF NOT EXISTS name3 INCREMENT 1 NO MINVALUE MAXVALUE 20 OWNED BY NONE", ); + // negative increments + let sql7 = "CREATE SEQUENCE seq START -100 INCREMENT -10 MINVALUE -1000 MAXVALUE 1;"; + pg().one_statement_parses_to( + sql7, + "CREATE TEMPORARY SEQUENCE IF NOT EXISTS name3 INCREMENT 1 NO MINVALUE MAXVALUE 20 OWNED BY NONE", + ); + + assert!(matches!( pg().parse_sql_statements("CREATE SEQUENCE foo INCREMENT 1 NO MINVALUE NO"), Err(ParserError::ParserError(_))

Then ran with

cargo test --test sqlparser_postgres

The example from the task description does not work, since the order of options is important for sqlparser-rs.

Try running it like this

CREATE SEQUENCE seq INCREMENT -10 MINVALUE -1000 MAXVALUE 1 START -100;

#1102

Thanks -- could you please add at least one test with this syntax (no space between - and the numbers)? I think this is important coverage as goes down a different parsing path

I added a test, but @git-hulk is comment made me think about the correctness of the solution. Perhaps the function really should return UnaryOp

By the way, do tests really need to be moved to a common module? Tests for sequences are currently written only for PostgreSQL

By the way, do tests really need to be moved to a common module? Tests for sequences are currently written only for PostgreSQL

That seems like it might be a good idea to make them more discoverable

agscpp · 2024-09-11T10:48:23Z

tests/sqlparser_common.rs

+    let sql4 = "SELECT -1";
+    one_statement_parses_to(sql4, "SELECT -1");
+}


My code doesn't affect this case, but I wrote a test for it so that the logic doesn't break in the future.

agscpp · 2024-09-11T12:37:05Z

src/parser/mod.rs

+                self.prev_token();
+                Ok(Expr::Value(self.parse_number_value()?))


This is done to avoid code duplication.

If there is no sign, it will not turn into "UnaryOp" so as not to break the old behavior.

agscpp · 2024-09-11T14:10:35Z

tests/sqlparser_common.rs

+    let sql1 = "SELECT -1";
+    one_statement_parses_to(sql1, "SELECT -1");


My code doesn't affect this case, but I wrote a test for it so that the logic doesn't break in the future.

iffyio · 2024-09-12T04:52:44Z

src/parser/mod.rs

@@ -7295,6 +7295,29 @@ impl<'a> Parser<'a> {
        }
    }

+    pub fn parse_number_value_with_sign(&mut self) -> Result<Expr, ParserError> {


Suggested change

pub fn parse_number_value_with_sign(&mut self) -> Result<Expr, ParserError> {

pub fn parse_number(&mut self) -> Result<Expr, ParserError> {

Also thinking especially since this is made a public function, that we should write a doc comment about what it does? and potentially mention that the return type for signed is a Unary Expr

OK, I'll add a comment

Can we update the method name as well? the current parse_number_value_with_sign somewhat suggests that only signed numbers will be parsed which might be misleading

Which name is better?
I can rename the second method to "parse_unsigned_number_value" to make this "parse_number_value"

Ah so I was thinking as in the original suggestion - if it would make sense to just call this parse_number()?

iffyio · 2024-09-12T04:55:46Z

src/parser/mod.rs

+            tok @ Token::Minus | tok @ Token::Plus => {
+                if tok == Token::Plus {
+                    Ok(Expr::UnaryOp {
+                        op: UnaryOperator::Plus,
+                        expr: Box::new(Expr::Value(self.parse_number_value()?)),
+                    })
+                } else {
+                    Ok(Expr::UnaryOp {
+                        op: UnaryOperator::Minus,
+                        expr: Box::new(Expr::Value(self.parse_number_value()?)),
+                    })
+                }
+            }


Suggested change

tok @ Token::Minus | tok @ Token::Plus => {

if tok == Token::Plus {

Ok(Expr::UnaryOp {

op: UnaryOperator::Plus,

expr: Box::new(Expr::Value(self.parse_number_value()?)),

})

} else {

Ok(Expr::UnaryOp {

op: UnaryOperator::Minus,

expr: Box::new(Expr::Value(self.parse_number_value()?)),

})

}

}

Token::Minus => {

Ok(Expr::UnaryOp {

op: UnaryOperator::Minus,

expr: Box::new(Expr::Value(self.parse_number_value()?)),

})

}

Token::Plus => {

Ok(Expr::UnaryOp {

op: UnaryOperator::Plus,

expr: Box::new(Expr::Value(self.parse_number_value()?)),

})

}

thinking we can avoid the double work of match+comparison?

Yes, you are right. I did write the code suboptimally.

iffyio

Left a couple minor comments, otherwise LGTM!

iffyio · 2024-09-14T07:04:13Z

src/parser/mod.rs

+    /// Parse a numeric literal and return Expr::UnaryOp if the number is signed,
+    /// otherwise return Expr::Value


Suggested change

/// Parse a numeric literal and return Expr::UnaryOp if the number is signed,

/// otherwise return Expr::Value

/// Parse a numeric literal as an expression. Returns a [`Expr::UnaryOp`] if the number is signed,

/// otherwise returns a [`Expr::Value`]

iffyio · 2024-09-14T07:05:01Z

src/parser/mod.rs

@@ -7295,6 +7295,29 @@ impl<'a> Parser<'a> {
        }
    }

+    pub fn parse_number_value_with_sign(&mut self) -> Result<Expr, ParserError> {


Can we update the method name as well? the current parse_number_value_with_sign somewhat suggests that only signed numbers will be parsed which might be misleading

alamb

Thank you @agscpp as well as @iffyio and @git-hulk for the review

I think there are a few more suggestions about comments from @iffyio but otherwise I think this PR is good to go

alamb · 2024-09-14T10:30:04Z

src/parser/mod.rs

@@ -11609,29 +11630,29 @@ impl<'a> Parser<'a> {
        if self.parse_keywords(&[Keyword::INCREMENT]) {
            if self.parse_keywords(&[Keyword::BY]) {
                sequence_options.push(SequenceOptions::IncrementBy(
-                    Expr::Value(self.parse_number_value()?),
+                    self.parse_number_value_with_sign()?,


alamb · 2024-09-14T10:30:43Z

tests/sqlparser_common.rs

+        "CREATE SEQUENCE name2 AS BIGINT INCREMENT 10 MINVALUE 30 MAXVALUE 5000 START WITH 45",
+    );
+}
+


By the way, do tests really need to be moved to a common module? Tests for sequences are currently written only for PostgreSQL

That seems like it might be a good idea to make them more discoverable

iffyio

LGTM!

alamb · 2024-09-16T21:04:19Z

🚀 -- thanks everyone

Co-authored-by: Agaev Huseyn <h.agaev@vkteam.ru>

git-hulk reviewed Sep 10, 2024

View reviewed changes

alamb reviewed Sep 10, 2024

View reviewed changes

Agaev Huseyn added 2 commits September 10, 2024 15:26

Fix parsing of negative values

a0873ca

Move tests to a common module and add a handler for '+'

216d721

agscpp force-pushed the fix-parse-sequence-options branch 2 times, most recently from 50936e0 to 2648917 Compare September 10, 2024 14:43

Fix parser for BigDecimal values

99b98ca

agscpp force-pushed the fix-parse-sequence-options branch from 2648917 to 99b98ca Compare September 10, 2024 15:00

alamb mentioned this pull request Sep 10, 2024

Add a test showing how negative constants are parsed #1421

Merged

alamb reviewed Sep 10, 2024

View reviewed changes

Add new tests

accee6d

agscpp commented Sep 11, 2024

View reviewed changes

agscpp force-pushed the fix-parse-sequence-options branch from 01c9823 to 62e12f4 Compare September 11, 2024 13:35

agscpp commented Sep 11, 2024

View reviewed changes

iffyio reviewed Sep 12, 2024

View reviewed changes

agscpp force-pushed the fix-parse-sequence-options branch 2 times, most recently from 0fefac1 to bf1412a Compare September 12, 2024 07:17

agscpp requested review from alamb, iffyio and git-hulk September 13, 2024 07:47

iffyio approved these changes Sep 14, 2024

View reviewed changes

alamb approved these changes Sep 14, 2024

View reviewed changes

agscpp force-pushed the fix-parse-sequence-options branch from bf1412a to 3f0621b Compare September 16, 2024 07:42

agscpp requested a review from iffyio September 16, 2024 07:44

Fix parsing of negative values

4eef71b

agscpp force-pushed the fix-parse-sequence-options branch from 3f0621b to 4eef71b Compare September 16, 2024 07:45

Fix function name

826fb60

iffyio approved these changes Sep 16, 2024

View reviewed changes

git-hulk approved these changes Sep 16, 2024

View reviewed changes

alamb merged commit 246838a into apache:main Sep 16, 2024
10 checks passed

agscpp added a commit to agscpp/datafusion-sqlparser-rs that referenced this pull request Oct 22, 2024

Fix parsing of negative values (apache#1419)

4015ada

Co-authored-by: Agaev Huseyn <h.agaev@vkteam.ru>

		self.prev_token();
		Ok(Expr::Value(self.parse_number_value()?))

		let sql1 = "SELECT -1";
		one_statement_parses_to(sql1, "SELECT -1");

	pub fn parse_number_value_with_sign(&mut self) -> Result<Expr, ParserError> {
	pub fn parse_number(&mut self) -> Result<Expr, ParserError> {

		/// Parse a numeric literal and return Expr::UnaryOp if the number is signed,
		/// otherwise return Expr::Value

Fix parsing of negative values #1419

Fix parsing of negative values #1419

Conversation

agscpp commented Sep 9, 2024 • edited Loading

agscpp commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Sep 10, 2024

coveralls commented Sep 10, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10881901043

Details

💛 - Coveralls

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iffyio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iffyio left a comment

Choose a reason for hiding this comment

alamb commented Sep 16, 2024

agscpp commented Sep 9, 2024 •

edited

Loading

coveralls commented Sep 10, 2024 •

edited

Loading