hdimer commented on PR #2418: URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2418#issuecomment-5233613351
Thanks — you're right the last iteration still had gaps. The root issue is that `Ident` doesn't record which escape mode produced its `value`, so one `Display` can't round-trip both: - default (`unescape=true`): `[a]]]]b]` parses to value `a]]b`, which must serialize back to `[a]]]]b]`. - no-escape (`unescape=false`): `[a]]b]` parses to value `a]]b`, which must serialize back to `[a]]b]`. Same `value`, two required outputs — so the "leave `]]` intact" approach I pushed is wrong for the first case (confirmed: `[a]]]]b]` round-trips to `[a]]b]` today). I'd propose making `[` consistent with `escape_quoted_string` for `"`: always double a literal `]`. That fixes default-mode round-tripping for every identifier (the actual #2409 bug), and mirrors `"`, whose no-escape mode is already display-lossy — so this makes `[` consistent rather than a special case. I'll back it with the round-trip property test you suggested. Prefer that, or would you rather keep no-escape round-tripping for `[` (accepting that an identifier containing consecutive `]]` can't be represented in default mode)? -- 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]
