s5dsn-eqee commented on code in PR #2557:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2557#discussion_r4323705911
##########
src/dialect/sqlite.rs:
##########
@@ -147,3 +151,57 @@ impl Dialect for SQLiteDialect {
true
}
}
+
+fn peek_nonstandard_type_modifiers(parser: &Parser) -> bool {
+ if !matches!(parser.peek_token_ref().token, Token::Word(_))
Review Comment:
SQLite allows multi-word type names before the modifiers ([type-name
grammar](https://www.sqlite.org/syntax/type-name.html),
[datatypes](https://www.sqlite.org/datatype3.html)), but
the peek only accepts a single word before `(`, so these still fail while
`sqlite3` accepts them:
```sql
CREATE TABLE t (a DOUBLE PRECISION(-1));
CREATE TABLE t (a CHARACTER VARYING(-1));
Could the peek skip a run of words and join them into the custom name?
```
##########
src/parser/mod.rs:
##########
@@ -12964,6 +12964,9 @@ impl<'a> Parser<'a> {
let _guard = self.recursion_counter.try_decrease()?;
let dialect = self.dialect;
+ if let Some(data_type) = dialect.parse_data_type(self) {
Review Comment:
The new hook also runs during the speculative `parse_data_type` in
`parse_prefix_inner`, so `abs(-1)` now comes back as `DataType::Custom` and
relies on the `Custom` arm to rewind.
It works, but nothing pins it. Could you add a regression test?
```rust
sqlite().verified_stmt("SELECT abs(-1), round(1, 1.5), char(+1, -2)");
##########
tests/sqlparser_sqlite.rs:
##########
@@ -1022,6 +1022,47 @@ fn test_non_bmp_identifiers() {
);
}
+#[test]
Review Comment:
Could you extend the test to cover the edges? These all parse and round-trip
on the branch, and `sqlite3` 3.51 accepts
them:
```rust
for sql in [
"CREATE TABLE t (a CHAR(0.0))",
"CREATE TABLE t (a CHAR(-0))",
"CREATE TABLE t (a CHAR(1.))",
"CREATE TABLE t (a CHAR(1e3))",
"CREATE TABLE t (a CHAR(+1.5e-3))",
"CREATE TABLE t (a CHAR(+1, +2))",
"CREATE TABLE t (a CHAR(-1) NOT NULL DEFAULT NULL)",
"CREATE TABLE t (a CHAR(1.5), b CHAR(10))",
"CREATE TABLE t (a char(-1))",
r#"CREATE TABLE t (a "CHAR"(-1))"#,
"CREATE TABLE t (a [CHAR](-1))",
"SELECT 1::CHAR(-1)",
] {
sqlite().verified_stmt(sql);
}
sqlite().one_statement_parses_to("CREATE TABLE t (a CHAR ( - 1 ))",
"CREATE TABLE t (a CHAR(-1))");
sqlite().one_statement_parses_to("CREATE TABLE t (a CHAR(-1,-2))", "CREATE
TABLE t (a CHAR(-1, -2))");
Negative cases: CHAR(1.5,), CHAR(+-1), CHAR(NULL). Also worth asserting
that CHAR(10) still yields DataType::Char, not Custom, and that
all_dialects_except(|d|
d.is::<SQLiteDialect>()) rejects CHAR(+10).
##########
tests/sqlparser_sqlite.rs:
##########
@@ -1022,6 +1022,47 @@ fn test_non_bmp_identifiers() {
);
}
+#[test]
Review Comment:
Plus, AGENTS.md asks for a fuzz seed per fix:
fuzz/fuzz_seeds/sqlite_2557_signed_type_modifiers.sql.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
