kevinmingtarja commented on issue #10256: URL: https://github.com/apache/datafusion/issues/10256#issuecomment-2080644645
Yeah I was kinda wondering about this too last time (https://github.com/apache/datafusion/issues/9726#issuecomment-2032753357). I was thinking we don't change the SQL parser too. Instead, I think given that the "sources" of the conversion to `Expr` is not only `ast::Expr` objects, having its return type be only `ast::Expr` is perhaps too restrictive? As we've seen in this case. If I understand correctly, I think another case is `Expr::AggregateFunction`, where we can't really convert it back to [ast::Function](https://docs.rs/sqlparser/latest/sqlparser/ast/struct.Function.html#) as it's also not an enum item in `ast::Expr`. So what if we extend the return type of `expr_to_sql` to more than `ast::Expr`? I think it makes sense to extend it to all the "sources" of conversions from `ast::*` to `Expr`. For example in here, we know that we are converting a `ast::OrderByExpr` to a `Expr::Sort`, so I think it makes sense that `ast::OrderByExpr` should also be a valid return type for `expr_to_sql`, as that is really the "source" data model of `Expr::Sort`. https://github.com/apache/datafusion/blob/f8c623fe045d70a87eac8dc8620b74ff73be56d5/datafusion/sql/src/expr/order_by.rs#L24-L36 -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
