alamb commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1922419742
##########
datafusion/sql/src/expr/mod.rs:
##########
@@ -165,13 +177,126 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
schema: &DFSchema,
planner_context: &mut PlannerContext,
) -> Result<Expr> {
- let mut expr = self.sql_expr_to_logical_expr(sql, schema,
planner_context)?;
+ // The location of the original SQL expression in the source code
+ let mut expr =
+ self.sql_expr_to_logical_expr(sql.clone(), schema,
planner_context)?;
Review Comment:
Having to copy the entire AST just to get the span information on error is
non ideal (we are trying to keep planning reasonably faster)
I understand that `Expr` doesn't have the `Span` (and adding it is quite
potentially intrusive). However, I wonder if you have considered using
[`PlannerContext`](https://github.com/apache/datafusion/blob/e718c1a5c5770c071c9c2e14a7681a7f1a2f3f23/datafusion/sql/src/planner.rs#L103-L117)?
Specifically, since the `PlannerContext `is already threaded through
most/all planning methods, if you could add the "current span" on the
PlannerContext, you would have the necessary span information when you
generated an error.
```rust
#[derive(Debug, Clone)]
pub struct PlannerContext {
...
/// the current span of the expression or statement being planned
/// Note not all statements have span information yet
/// see <https://github.com/apache/datafusion-sqlparser-rs/issues/1548>
current_span: Option<Span>,
...
}
```
Then rather than calling `sql_expr_to_logical_expr` twice, you could have
the error generated in `sql_expr_to_logical_expr` include the span information.
The key would to manage setting/restoring the spans during the planing
process. Maybe it could be something like
```rust
...
// set the `current_span` field in the planner context
// if sql has a span, otherwise use the existing span
let planner_context = planner_context.with_span(&sql);
self.sql_expr_to_logical_expr(sql.clone(), schema, planner_context)?;
...
```
--
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
