zvonimir-dd commented on code in PR #2590:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2590#discussion_r4429014425
##########
src/ast/mod.rs:
##########
@@ -1757,7 +1763,21 @@ impl fmt::Display for Expr {
Expr::Identifier(s) => write!(f, "{s}"),
Expr::Wildcard(_) => f.write_str("*"),
Expr::QualifiedWildcard(prefix, _) => write!(f, "{prefix}.*"),
- Expr::CompoundIdentifier(s) => write!(f, "{}",
display_separated(s, ".")),
+ Expr::CompoundIdentifier(idents) => {
Review Comment:
This changes how compound identifiers display in every dialect, and I think
it breaks existing tests. `parse_qualified_identifiers_with_numeric_prefix` in
`tests/sqlparser_mysql.rs` calls `verified_stmt` on `SELECT t.15to29 FROM
my_table AS t`, `SELECT t.15e29 FROM my_table AS t` and `SELECT
1db.1table.1column`, which would now display as `t . 15to29`, `t . 15e29` and
`1db . 1table . 1column`. The testing list in the description only covers the
hive and postgres test binaries, and CI hasn't run yet, which may be why
nothing flagged it.
I don't think compound identifiers need the spacing. In dialects with
`supports_numeric_prefix`, a `.` directly after a word always becomes `Period`
(see `test_tokenize_identifiers_numeric_prefix`, which covers `t.1two3`), and
that's why those MySQL tests round-trip today. #2589 only happens when the dot
follows `]` or `)`, and that goes through `AccessExpr::Dot`. Could you drop
this arm and keep only the `AccessExpr::Dot` change?
--
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]
