Complete support for Expr --> String

[alamb]

Part of #9494

Is your feature request related to a problem or challenge?

This ticket tracks the remaining work to complete #9495

As @devinjdangelo says in #9494 (comment)

We now have a solid foundation for converting Exprs --> SQL (see #9495 for why this is valuable).

It should now be straightforward to add support for the remaining Expr types and doing so is a great way to get more familiar with DataFusion's core data structures and optimization algorithms without already having expertise in database internals.

Describe the solution you'd like

The basic task is to:

1. Pick one (or a few) of the expressions below
2. Create a PR to Implement the Expr --> AST reverse code (in this match statement)
3. Add a test (for example, here)

Here is the remaining list of exprs (just note on the ticket which you plan to work on)

• ScalarVariable Expr --> String Support #10518
• IsNull / IsNotNull Expr --> String Support #10519
• Placeholder Expr --> String Support #10522
• TryCast Expr --> String Support #10520
• GroupingSet Expr --> String Support #10521
• OuterColumnReference Expr --> String Support #10523
• Allow expr_to_sql unparsing with no quotes #10197
• Support OrderBy and Sort in Expr->String #10256

Describe alternatives you've considered

No response

Additional context

No response

[Lordworms]

take IsTrue, IsFalse, IsUnknown

[sebastian2296]

I'd like to work on isNotNull, Not and Between

[yyy1000]

take ScalarFunction and InList

[yyy1000]

take Case

[Weijun-H]

take Like

[kevinmingtarja]

take Sort, Exists

[pratikpugalia]

I'd like to work on Negative

[alamb]

It looks like there are a few things not yet complete in https://github.com/apache/arrow-datafusion/blob/main/datafusion/sql/src/unparser/expr.rs#L52

• Window functions
• user defined functions
• (maybe more)

[yyy1000]

take SimilarTo, IsNotTrue, IsNotUnknown,Negative

[yyy1000]

Here is a list that has not been done yet, for interested contributor to better view:

• GetIndexedField
• ScalarVariable
• TryCast
• Sort
• Exists
• Wildcard
• GroupingSet
• Placeholder
• OuterReferenceColumn
• Unnest
• WindowFunction

[yyy1000]

Also Aggregate Function only supports BuiltInAggregateFunction, and it didn't use filter field.
Maybe we should use AggregateExpressionWithFilter when there's a filter? See: https://github.com/sqlparser-rs/sqlparser-rs/blob/e747c9c2af08f4ea12e8d1692adf95998209e2a1/src/ast/mod.rs#L643-L649

[kevinmingtarja]

I noticed that in the sqlparser-rs crate, OrderByExpr is not part of the sqlparser::ast::Expr enum (which is the return type for expr_to_sql), which is a problem when I was implementing this for Expr::Sort.

So I wanted to get some thoughts on how best to proceed, should we make a change in sqlparser-rs, or should we just create a new enum to encapsulate the two, or anything else.

[alamb]

I noticed that in the sqlparser-rs crate, OrderByExpr is not part of the sqlparser::ast::Expr enum (which is the return type for expr_to_sql), which is a problem when I was implementing this for Expr::Sort.

So I wanted to get some thoughts on how best to proceed, should we make a change in sqlparser-rs, or should we just create a new enum to encapsulate the two, or anything else.

I think when converting Expr::Sort to an sql::ast::Expr it should just return its inner Expr (and igore the ORDER BY information)

The ORDER BY information is required when turning the expression back into an entire query I think...

But at first just converting to sql::ast::Expr by ignoring the ORDER BY is probably correct in most cases

[devinjdangelo]

Thank you everyone who has contributed so far 🙏. I just filed a PR to update datafusion-federation to point upstream for plan->SQL going forward datafusion-contrib/datafusion-federation#30. The additional expr implementations upstream here will enable datafusion-federation to handle many more queries than before.

[devanbenz]

Here is a list that has not been done yet, for interested contributor to better view:

• GetIndexedField
• ScalarVariable
• TryCast
• Sort
• Exists
• Wildcard
• GroupingSet
• Placeholder
• OuterReferenceColumn
• Unnest
• WindowFunction

Take ScalarVariable if this is still needed.

[alamb]

Thank you very much @devanbenz -- that would be awesome. Yes in general completing this list (or determining it is not possible) is very much needed

[devanbenz]

Hi @devanbenz , I think ScalarVariable has not been done yet and you could have a try, though it may not be possible.🤔 I think maybe it should be converted to ScalarValue first, then call scalar_to_sql function.

Taking a look at this today. Will give it a try and update this thread with any thoughts, observations, or problems I run in to :)

[devanbenz]

Here are some thoughts to help people figure out what SQL matches to what expression:

• GetIndexedField --> `SELECT col['field_name']
• ScalarVariable --> Not sure (maybe @@foo@@?
• TryCast * --> CAST (COL AS <datatype>>
• Sort --> col ASC or col DESC NULLS FIRST
• Exists --> col exists (SELECT 1 from foo)
• Wildcard --> COUNT(*)
• GroupingSet --> Not sure
• Placeholder --> SELECT $1
• OuterReferenceColumn --> not sure (I think this is the same as a column)
• Unnest --> UNNEST(SELECT col FROM bar)
• WindowFunction --> first_value(x ORDER BY y PARTITION BY z)

@alamb I beleive ScalarVariable would just be something like @variable.

[alamb]

Thanks @devanbenz !

[devanbenz]

@alamb if you could please take a look at sqlparser-rs/sqlparser-rs#1260 -- it's linked to this issue :)

[alamb]

@alamb if you could please take a look at sqlparser-rs/sqlparser-rs#1260 -- it's linked to this issue :)

Thanks @devanbenz -- can you explain how this is related to this issue? (Sorry I am sure it is obvious but I can't keep everything in my head anymore )

[devanbenz]

@alamb if you could please take a look at sqlparser-rs/sqlparser-rs#1260 -- it's linked to this issue :)

Thanks @devanbenz -- can you explain how this is related to this issue? (Sorry I am sure it is obvious but I can't keep everything in my head anymore )

All good - this is for the Unparser expr_to_sql match statement

datafusion/datafusion/sql/src/unparser/expr.rs

Line 391 in 89443bf

Expr::ScalarVariable(_, _) => {

I need to have a supported ast::Expr type for ScalarVariable from sql-parser. 👍

[alamb]

It seems to me that Expr::ScalarVariable currently is parsed from ast::Identifier (that starts with @)-- perhaps instead of adding a new ast node, you could just create an equivalent ast::Identifier 🤔

datafusion/datafusion/sql/src/expr/identifier.rs

Lines 28 to 43 in accce97

	pub(super) fn sql_identifier_to_expr(
	&self,
	id: Ident,
	schema: &DFSchema,
	planner_context: &mut PlannerContext,
	) -> Result<Expr> {
	if id.value.starts_with('@') {
	// TODO: figure out if ScalarVariables should be insensitive.
	let var_names = vec![id.value];
	let ty = self
	.context_provider
	.get_variable_type(&var_names)
	.ok_or_else(|| {
	plan_datafusion_err!("variable {var_names:?} has no type information")
	})?;
	Ok(Expr::ScalarVariable(ty, var_names))

[devanbenz]

It seems to me that Expr::ScalarVariable currently is parsed from ast::Identifier (that starts with @)-- perhaps instead of adding a new ast node, you could just create an equivalent ast::Identifier 🤔

datafusion/datafusion/sql/src/expr/identifier.rs

Lines 28 to 43 in accce97

	pub(super) fn sql_identifier_to_expr(
	&self,
	id: Ident,
	schema: &DFSchema,
	planner_context: &mut PlannerContext,
	) -> Result<Expr> {
	if id.value.starts_with('@') {
	// TODO: figure out if ScalarVariables should be insensitive.
	let var_names = vec![id.value];
	let ty = self
	.context_provider
	.get_variable_type(&var_names)
	.ok_or_else(|| {
	plan_datafusion_err!("variable {var_names:?} has no type information")
	})?;
	Ok(Expr::ScalarVariable(ty, var_names))

Sounds good - I'll have some time to come back to this in the next couple days! Will modify per your suggestion :)

[alamb]

I filed tickets for the remaining issues

These ones are likely straightforward.

• ScalarVariable Expr --> String Support #10518
• IsNull / IsNotNull Expr --> String Support #10519
• Placeholder Expr --> String Support #10522

The others are likely more involved as I don't know how to make a reproducer

[alamb]

With the completion of #10555 from @xinlifoobar I think this epic is now done!