DataFrame parse_sql_expr does not handle aliases

[milenkovicm]

Describe the bug

df.parse_sql_expr does not handle expression aliases, for expressions like

df.parse_sql_expr("col0 as new_col0")

it will return col(col0`) expression

To Reproduce

statement

df.parse_sql_expr("SUM(int_col) as sum_int_col")

from

datafusion/datafusion-examples/examples/parse_sql_expr.rs

Line 125 in aeca7ea

"| double_col | sum(?table?.int_col) |",

can be used to demonstrate this issue

as return column has sum(?table?.int_col) as the name

Expected behavior

it is expected to have expression alias handled

Additional context

No response

[Eason0729]

I spent some time on tracing code but found unable to fix it.
Maybe we need to change some code in sqlparser, correct me if I am wrong.

Detail

To be specific, I run following with debugger attached

let df = ctx.sql("SELECT 1 as col0").await?;
let expr = df.parse_sql_expr("1 + 3 * col0 as new_col0")?;

And found this line, whose value return from sqlparser to be following.

It doesn't contain new_col0.

BinaryOp { left: Value(Number("1", false)), op: Plus, right: BinaryOp { left: Value(Number("3", false)), op: Multiply, right: Identifier(Ident { value: "col0", quote_style: None }) } }

[Eason0729]

apache/datafusion-sqlparser-rs#1439

[milenkovicm]

I think you're right @Eason0729 I did not have much time to investigate but initial investigation lead to sqlparser

[Eason0729]

There is a private method parse_expr_with_alias in sqlparser, which returns ExprWithAlias.

I would propose changing the return type from sqlparser::ast::Expr on call path to sqlparser::ast::ExprWithAlias, use enum datafusion_expr::expr::Expr::Alias in return type.

Writing a example change might be better if more description in needed

This could cause other method to change(including some public methods on SessionState).

If above is okay, I would begin working on it.

[milenkovicm]

parse_sql_expr returns expression, alias is an expression, why would you need to change public method in SessionState?

[Eason0729]

Mainly because it could be done without changing sqlparser.

I think I should start writing code right away, as it would be better to communicate with code.

I would do it in my fork and let you know when I am ready, then I might learn how to do it without changing public method in SessionState.

[Eason0729]

I just finish it in my fork.

It's not meant to be a pull request, just to showcase what I would like to change.

Here is the public API I change: diff

Also, I change sqlparser of my fork to make Parser::parse_expr_with_alias public: https://github.com/Eason0729/sqlparser-rs/commit/c6360b159f9e9baade08ab2e4d7a12083d24a567

[milenkovicm]

It does look good, @Eason0729.
maybe @alamb and @comphead can give you better advice than me when they get some time

[Eason0729]

Okay, then I will remove thing that doesn't belong to pull request and submit pull request.

[alamb]

PR in sqlparser: apache/datafusion-sqlparser-rs#1444

[Eason0729]

Let's wait for apache/datafusion-sqlparser-rs#1444, then I will submit a PR for datafusion.

[alamb]

Thank you @Eason0729 -- we'll try and release a sqlparser release shortly

[milenkovicm]

Hey @Eason0729 any interested in continuing this issue?

[Eason0729]

@milenkovicm
Yes, I am interested in working on this issue.

I am waiting for next release of sqlparser, so parse_expr_with_alias method on sqlparser::parser::Parser can be make public.

[milenkovicm]

My bad, I thought it's released

[alamb]

My bad, I thought it's released

It is on my queue: apache/datafusion-sqlparser-rs#1423

[Eason0729]

I just see sqlparser release 0.52.0. (which was released 6 days ago 😄 )

I will start working on that tomorrow.