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

Fix parsing of negative values #1419

Merged
merged 6 commits into from
Sep 16, 2024
Merged

Fix parsing of negative values #1419

Show file tree
Hide file tree
Changes from 4 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
16 changes: 16 additions & 0 deletions src/parser/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7274,6 +7274,22 @@ impl<'a> Parser<'a> {
let placeholder = tok.to_string() + &ident.value;
Ok(Value::Placeholder(placeholder))
}
tok @ Token::Minus | tok @ Token::Plus => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 🤔

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@iffyio @alamb @git-hulk

let next_token = self.next_token();
match next_token.token {
Token::Number(n, l) => {
if tok == Token::Minus {
Ok(Value::Number(
Self::parse(tok.to_string() + &n, location)?,
l,
))
} else {
Ok(Value::Number(Self::parse(n, location)?, l))
}
}
_ => self.expected("number", next_token),
}
}
unexpected => self.expected(
"a value",
TokenWithLocation {
Expand Down
32 changes: 32 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,38 @@ fn parse_window_function_null_treatment_arg() {
);
}

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

Choose a reason for hiding this comment

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

I double checked that this syntax is indeed valid in postgres (🤯 )

postgres=# CREATE SEQUENCE name1
    AS BIGINT
    INCREMENT   -15
    MINVALUE - 2000  MAXVALUE -50
    START WITH -   60;
CREATE SEQUENCE

postgres=# SELECT *
FROM information_schema.sequences
ORDER BY sequence_name;
 sequence_catalog | sequence_schema | sequence_name | data_type | numeric_precision | numeric_precision_radix | numeric_scale | start_value | minimum_value | maximum_value | increment | cycle_option
------------------+-----------------+---------------+-----------+-------------------+-------------------------+---------------+-------------+---------------+---------------+-----------+--------------
 postgres         | public          | name1         | bigint    |                64 |                       2 |             0 | -60         | -2000         | -50           | -15       | NO
(1 row)

AS BIGINT
INCREMENT -15
MINVALUE - 2000 MAXVALUE -50
START WITH - 60";
one_statement_parses_to(
sql1,
"CREATE SEQUENCE name1 AS BIGINT INCREMENT -15 MINVALUE -2000 MAXVALUE -50 START WITH -60",
);

let sql2 = "CREATE SEQUENCE name2
AS BIGINT
INCREMENT +10
MINVALUE + 30 MAXVALUE +5000
START WITH + 45";
one_statement_parses_to(
sql2,
"CREATE SEQUENCE name2 AS BIGINT INCREMENT 10 MINVALUE 30 MAXVALUE 5000 START WITH 45",
);

let sql3 = "CREATE SEQUENCE name3 INCREMENT -10 MINVALUE -1000 MAXVALUE 1 START -100;";
one_statement_parses_to(
sql3,
"CREATE SEQUENCE name3 INCREMENT -10 MINVALUE -1000 MAXVALUE 1 START -100",
);

let sql4 = "SELECT -1";
one_statement_parses_to(sql4, "SELECT -1");
}
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.


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