Make SQL strings generated from Exprs "prettier"

@backkem

Is your feature request related to a problem or challenge?

Part of #9494

As @backkem says #10528 (comment) on #10528

Currently, expressions from the DataFusion SQL unparser (aka expr --> String) are somewhat ugly

For example the expression col("a").eq(lit(5)) would be rendered as a = 5 by most poeple if they did it manaully, but DataFusion's unparser currently renders it like "a" = 5 (with extra quotes).

DataFusion also puts in quotes to make the order of operations explicit -- so instead of a < 5 AND b < 10 it would render ("a" < 5) AND ("b" < 10)

The current unparser is conservative and likely works well for when generating SQL for consumptions by other database systems. However, the SQL is not as nice for human consumption

Here is another instance from the example

datafusion/datafusion-examples/examples/plan_to_sql.rs

Lines 50 to 53 in 98647e8

    
           let expr = col("a").lt(lit(5)).or(col("a").eq(lit(8))); 
        
           let ast = expr_to_sql(&expr)?; 
        
           let sql = format!("{}", ast); 
        
           assert_eq!(sql, r#"(("a" < 5) OR ("a" = 8))"#);

Describe the solution you'd like

If we want to make the generated SQL easier to read by humans / more succint, these steps will have to be made "smarter".

Describe alternatives you've considered

Potential ideas:

We'll have to add in the math rules to avoid unneeded parentheses
(likely dialect specific) rules for determining of quoting is needed.

Note that the latter likely involves listing out the reserved keywords for each dialect.

Additional context

No response

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.

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.

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

datafusion/datafusion/sql/src/unparser/dialect.rs

Line 28 in e7858ff

fn identifier_quote_style(&self) -> Option<char> {

However, what we need is a checker to check if the identifier needs to be quoted.
I think I can make a PR for DefaultDialect first.

As the mentioned in dialect.rs

datafusion/datafusion/sql/src/unparser/dialect.rs

Line 19 in e7858ff

    
           /// Note: this trait will eventually be replaced by the Dialect in the SQLparser package

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

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.

@tisonkun

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

@tisonkun

#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:

Support: Databricks and generic: fix for values as table name datafusion-sqlparser-rs#1278
And what's still unclear how to fix, build(deps): upgrade sqlparser to 0.47.0 #10392 (comment)

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

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.

FWI #10573 is merged!

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

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

Excellent call -- I filed #10633

Navigation Menu

Repository navigation

Make SQL strings generated from `Expr`s "prettier" #10557

Is your feature request related to a problem or challenge?

Describe the solution you'd like

Describe alternatives you've considered

Additional context

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	let expr = col("a").lt(lit(5)).or(col("a").eq(lit(8)));
	let ast = expr_to_sql(&expr)?;
	let sql = format!("{}", ast);
	assert_eq!(sql, r#"(("a" < 5) OR ("a" = 8))"#);

Make SQL strings generated from Exprs "prettier" #10557

Description

Is your feature request related to a problem or challenge?

Describe the solution you'd like

Describe alternatives you've considered

Additional context

Activity

goldmedal commented on May 18, 2024

backkem commented on May 18, 2024

goldmedal commented on May 18, 2024

goldmedal commented on May 19, 2024

backkem commented on May 19, 2024

alamb commented on May 20, 2024

tisonkun commented on May 20, 2024

goldmedal commented on May 21, 2024

alamb commented on May 22, 2024

backkem commented on May 23, 2024

alamb commented on May 23, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Make SQL strings generated from `Expr`s "prettier" #10557