Skip to content

Fix parsing of negative values #1419

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

Merged
merged 6 commits into from
Sep 16, 2024
Merged

Fix parsing of negative values #1419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a0873ca
Fix parsing of negative values
Sep 9, 2024
216d721
Move tests to a common module and add a handler for '+'
Sep 10, 2024
99b98ca
Fix parser for BigDecimal values
Sep 10, 2024
accee6d
Add new tests
Sep 11, 2024
4eef71b
Fix parsing of negative values
Sep 11, 2024
826fb60
Fix function name
Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 28 additions & 25 deletions src/parser/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7284,6 +7284,7 @@ impl<'a> Parser<'a> {
}
}

/// Parse an unsigned numeric literal
pub fn parse_number_value(&mut self) -> Result<Value, ParserError> {
match self.parse_value()? {
v @ Value::Number(_, _) => Ok(v),
Expand All @@ -7295,6 +7296,26 @@ impl<'a> Parser<'a> {
}
}

/// Parse a numeric literal as an expression. Returns a [`Expr::UnaryOp`] if the number is signed,
/// otherwise returns a [`Expr::Value`]
pub fn parse_number(&mut self) -> Result<Expr, ParserError> {
let next_token = self.next_token();
match next_token.token {
Token::Plus => Ok(Expr::UnaryOp {
op: UnaryOperator::Plus,
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()?)),
}),
_ => {
self.prev_token();
Ok(Expr::Value(self.parse_number_value()?))
Comment on lines +7313 to +7314
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done to avoid code duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
}
}

fn parse_introduced_string_value(&mut self) -> Result<Value, ParserError> {
let next_token = self.next_token();
let location = next_token.location;
Expand Down Expand Up @@ -11608,53 +11629,35 @@ impl<'a> Parser<'a> {
//[ INCREMENT [ BY ] increment ]
if self.parse_keywords(&[Keyword::INCREMENT]) {
if self.parse_keywords(&[Keyword::BY]) {
sequence_options.push(SequenceOptions::IncrementBy(
Expr::Value(self.parse_number_value()?),
true,
));
sequence_options.push(SequenceOptions::IncrementBy(self.parse_number()?, true));
} else {
sequence_options.push(SequenceOptions::IncrementBy(
Expr::Value(self.parse_number_value()?),
false,
));
sequence_options.push(SequenceOptions::IncrementBy(self.parse_number()?, false));
}
}
//[ MINVALUE minvalue | NO MINVALUE ]
if self.parse_keyword(Keyword::MINVALUE) {
sequence_options.push(SequenceOptions::MinValue(Some(Expr::Value(
self.parse_number_value()?,
))));
sequence_options.push(SequenceOptions::MinValue(Some(self.parse_number()?)));
} else if self.parse_keywords(&[Keyword::NO, Keyword::MINVALUE]) {
sequence_options.push(SequenceOptions::MinValue(None));
}
//[ MAXVALUE maxvalue | NO MAXVALUE ]
if self.parse_keywords(&[Keyword::MAXVALUE]) {
sequence_options.push(SequenceOptions::MaxValue(Some(Expr::Value(
self.parse_number_value()?,
))));
sequence_options.push(SequenceOptions::MaxValue(Some(self.parse_number()?)));
} else if self.parse_keywords(&[Keyword::NO, Keyword::MAXVALUE]) {
sequence_options.push(SequenceOptions::MaxValue(None));
}

//[ START [ WITH ] start ]
if self.parse_keywords(&[Keyword::START]) {
if self.parse_keywords(&[Keyword::WITH]) {
sequence_options.push(SequenceOptions::StartWith(
Expr::Value(self.parse_number_value()?),
true,
));
sequence_options.push(SequenceOptions::StartWith(self.parse_number()?, true));
} else {
sequence_options.push(SequenceOptions::StartWith(
Expr::Value(self.parse_number_value()?),
false,
));
sequence_options.push(SequenceOptions::StartWith(self.parse_number()?, false));
}
}
//[ CACHE cache ]
if self.parse_keywords(&[Keyword::CACHE]) {
sequence_options.push(SequenceOptions::Cache(Expr::Value(
self.parse_number_value()?,
)));
sequence_options.push(SequenceOptions::Cache(self.parse_number()?));
}
// [ [ NO ] CYCLE ]
if self.parse_keywords(&[Keyword::NO, Keyword::CYCLE]) {
Expand Down
12 changes: 12 additions & 0 deletions tests/sqlparser_common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2819,6 +2819,18 @@ fn parse_window_function_null_treatment_arg() {
);
}

#[test]
fn parse_negative_value() {
let sql1 = "SELECT -1";
one_statement_parses_to(sql1, "SELECT -1");
Comment on lines +2824 to +2825
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


let sql2 = "CREATE SEQUENCE name INCREMENT -10 MINVALUE -1000 MAXVALUE 15 START -100;";
one_statement_parses_to(
sql2,
"CREATE SEQUENCE name INCREMENT -10 MINVALUE -1000 MAXVALUE 15 START -100",
);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

#[test]
fn parse_create_table() {
let sql = "CREATE TABLE uk_cities (\
Expand Down
20 changes: 20 additions & 0 deletions tests/sqlparser_postgres.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,26 @@ 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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

AS BIGINT
INCREMENT -15
MINVALUE - 2000 MAXVALUE -50
START WITH - 60";
pg().one_statement_parses_to(
sql7,
"CREATE SEQUENCE name4 AS BIGINT INCREMENT -15 MINVALUE -2000 MAXVALUE -50 START WITH -60",
);

let sql8 = "CREATE SEQUENCE name5
AS BIGINT
INCREMENT +10
MINVALUE + 30 MAXVALUE +5000
START WITH + 45";
pg().one_statement_parses_to(
sql8,
"CREATE SEQUENCE name5 AS BIGINT INCREMENT +10 MINVALUE +30 MAXVALUE +5000 START WITH +45",
);

assert!(matches!(
pg().parse_sql_statements("CREATE SEQUENCE foo INCREMENT 1 NO MINVALUE NO"),
Err(ParserError::ParserError(_))
Expand Down
Loading