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 location concern still stands, and the new code made it clearer why.
#### The flag names one case but enables three
| Case | Example | Today's failure |
|---|---|---|
| signed numbers | `CHAR(-1)`, `CHAR(+10)` | `Expected: literal int,
found: -` |
| fractional modifiers | `VARCHAR(2.5)`, `NUMERIC(10.5)` | `Could not
parse '2.5' as u64` |
| oversized precision | `DECIMAL(99999999999999999999999, 2)` | `number
too large to fit in target type` |
Only the first is new syntax. The other two already tokenize as a single
number and fail inside `parse_literal_uint`, because `DataType` stores
precision as `u64`. A boolean flag has
to describe that slice in its name. A parse hook does not.
#### Why I'd still prefer the `parse_data_type` hook
- The lookahead and custom parse move to `sqlite.rs` next to the
`parse_statement` override. The common parser keeps one line.
- `supports_signed_type_modifier` goes away, so there is no naming problem.
- Every helper the two functions use is already `pub`, so nothing else
changes.
<details>
<summary><b>Scope note</b></summary>
This path produces `DataType::Custom`, so `INT(10)` and `INT(-1)` get
different AST shapes. That is fine for SQLite, which ignores modifier values,
but it is not a base for other
dialects. Supporting these modifiers generally means a precision type
richer than `u64` in `DataType`, which is a separate AST refactor. Keeping this
SQLite-only behind a hook avoids
committing the common parser to `Custom` before that decision.
</details>
--
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]
