Make SQL strings generated from `Expr`s "prettier" about datafusion HOT 11 CLOSED

#10392 is the upgrade to sqlparser -- I think it is pretty close but @tisonkun hit an issue during upgrade.

We may need a 0.46.1 for resolving the regressions:

• sqlparser-rs/sqlparser-rs#1278
• And what's still unclear how to fix, #10392 (comment)

I've locally confirmed that array.slt is last failure for cargo test --test sqllogictests.

from datafusion.

Provide something I surveyed.

I think we can follow how Calcite handles the quoted issue. The SqlDialect of Calcite has a check rule identifierNeedsQuote.

https://github.com/apache/calcite/blob/aba64f0b217093b500629fe07a0befdc68293fbc/core/src/main/java/org/apache/calcite/sql/SqlDialect.java#L413-L415

It can be overridden according to the specific data source, such as BigQuery:

https://github.com/apache/calcite/blob/50a20824c4536450dcae963b5e757cf4bbc7e406/core/src/main/java/org/apache/calcite/sql/dialect/BigQuerySqlDialect.java#L106-L109

They implement some rules, such as regex patterns or reserved word lists. I think a dialect-specific rule is a nice choice.

from datafusion.

Indeed, there is already a function on the sqlparser::dialect trait that takes this into account:

https://github.com/sqlparser-rs/sqlparser-rs/blob/54184460b5d873a67c2801e8b7c6e4f145bc65df/src/dialect/mod.rs#L113-L116

The dialect specific implementations just need to be expanded on. For now they just always return a conservative quote character.

from datafusion.

https://github.com/sqlparser-rs/sqlparser-rs/blob/54184460b5d873a67c2801e8b7c6e4f145bc65df/src/dialect/mod.rs#L113-L116

The dialect specific implementations just need to be expanded on. For now they just always return a conservative quote character.

I think it's same as the dialect in datafusion::unparser::dialect

from datafusion.

As the mentioned in dialect.rs

I think we need to use the Dialect in sqlparser-rs instead and extract identifier_needs_quote in #10573 to sqlparser-rs. Just like sqlparser-rs/sqlparser-rs#1170

from datafusion.

Yes, these are basically the same object. The one in DataFusion was put there temporarily until the trait extension in the sqlparser repo is landed and pushed to crates.io. This may have happened in the meantime.

from datafusion.

#10392 is the upgrade to sqlparser -- I think it is pretty close but @tisonkun hit an issue during upgrade.

from datafusion.

I'm not sure but I think we can merge #10573 first because it also fix many unpasring tests. Then, I'll create PR for sqlparser to add the check rule in dialect.

from datafusion.

FWI #10573 is merged!

from datafusion.

Do we split off a ticket reduce the nr of brackets emitted?

from datafusion.

Do we split off a ticket reduce the nr of brackets emitted?

Excellent call -- I filed #10633

from datafusion.