s5dsn-eqee commented on code in PR #2557:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2557#discussion_r4306685232
##########
src/parser/mod.rs:
##########
@@ -12904,6 +12904,9 @@ impl<'a> Parser<'a> {
let _guard = self.recursion_counter.try_decrease()?;
let dialect = self.dialect;
+ if let Some(custom) = self.maybe_parse(|p|
p.parse_type_with_signed_modifier())? {
Review Comment:
Thanks for the update.
The second point still stands, and the new code made it clearer why.
**The flag covers three cases, not one**
1. signed numbers: `CHAR(-1)`, `CHAR(+10)`
2. fractional modifiers: `VARCHAR(2.5)`, `NUMERIC(10.5)`
3. oversized precision: `DECIMAL(99999999999999999999999, 2)`
Only 1 is new syntax. 2 and 3 are valid number tokens that fail because
`DataType` stores precision as `u64`.
**It is a second type grammar**
- `peek_type_with_signed_modifier` and `parse_type_with_signed_modifier`
must agree on the accepted slice by hand.
- The AST shape depends on the modifier value: `INT(10)` is `DataType::Int`,
`INT(-1)` is `DataType::Custom`.
**Why I'd still prefer the `parse_data_type` hook**
- The hook name does not have to describe which modifiers it accepts.
- Lookahead and custom parse move to `sqlite.rs` next to the
`parse_statement` override, the common parser keeps one line.
- All helpers used are already `pub`, nothing else changes.
- `parse_object_name` + `parse_optional_type_modifiers` already produce
`DataType::Custom`, so the impl could reuse them and only add the sign handling
instead of a second parse
loop.
--
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]
