hdimer commented on PR #2418:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2418#issuecomment-5549067979
Pushed. Both repros were real; here is where they landed.
**`SELECT [a]]]]b]`**: fixed. Dropped the "leave an already-doubled `]]`
alone" rule, so every `]` in the value is doubled unconditionally. That is the
only escaping that round-trips a value holding consecutive `]`.
The cost is that no-escape mode no longer round-trips brackets:
`with_unescape(false)` keeps the raw `]]` in the value, so `[a]]b]` now
serializes to `[a]]]]b]`. The two are mutually exclusive for a `Display` that
can see neither the dialect nor `ParserOptions`, and the old rule picked the
no-escape side at the price of corrupting every real identifier containing
`]]`. I removed the no-escape assertion I added last round, since it pinned the
behaviour this change rejects. Nothing on main covered it.
**Round-trip test**: replaced the hand-listed cases with an exhaustive one.
Every value of length 1..=5 over `]`, `"`, `a` and `é`, serialized and reparsed
against MSSQL and SQLite, asserting both the recovered AST and byte-identical
re-serialization. It fails against the previous formatter (smallest failing
value: `]]`) and against main, so it is doing work.
**`SELECT ["a]]b"]`**: I do not think this one can be fixed here, and it
needs your call.
In MSSQL that SQL gives the value `"a]b"`. In Redshift, `["=]"]` gives the
value `"=]"` through the nested-identifier path, where the inner `]` is literal
and must not be doubled. `test_parse_nested_quoted_identifier` pins exactly
that. Same shape of value, opposite required output, and `Ident::fmt` has no
dialect. Doubling makes that test fail with `["=]]"]`. I could not find a
content-based rule that separates the two honestly, since either value can
legitimately come from either dialect.
Worth knowing: that verbatim branch is already unsound on main, with no `]`
involved. On current main, `redshift().verified_stmt(r#"SELECT 1 AS ["a""b"]
FROM a"#)` fails. The nested path folds `""` to `"`, giving the value `"a"b"`,
which serializes to `["a"b"]` and no longer parses. Same root cause, predates
this PR, untested.
The structural fix, if you want it: have the Redshift nested form store
`["foo"]` as `Ident { value: "foo", quote_style: '"' }` instead of stuffing the
quotes into the value. `redshift.rs` already documents `foo`, `"foo"` and
`["foo"]` as meaning the same thing, so that is arguably the right AST
regardless. The special case in `Ident::fmt` then deletes itself and both of
your repros pass. The visible change is that `["foo"]` re-serializes as
`"foo"`, so `test_parse_nested_quoted_identifier` would move to
`one_statement_parses_to`. Happy to do it as a separate PR, but it changes
Redshift output so I did not fold it in here.
Comments trimmed.
--
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]
