eliaperantoni commented on PR #13664: URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2598471076
@alamb I'm very sorry for the delay. I think your points about not wanting to change the logical types are valid, and I confess it was a bit of a pain to make all the changes to make my previous iteration compile and route the spans through. I tried again, this time using your suggestion of: > What do you think about initially focusing on locations from the errors out of the sql planner (SqlToRel) and postpone adding Spans to the plan nodes? I think this was a lot easier and pollutes the source code a lot less. So far I've implemented: - Table not found - Unqualified column not found - Qualified column not found - Mismatch in number of columns in set operation I think the "column not found" is the most interesting, since when I get an unresolved `datafusion::Column`, I now have to walk down a tree of `sqlparser::Expr` and fine one that produces a matching `datafusion::Column`. i.e. since I can't store data in the logical node, when I get a troublesome one I have to transform the AST tree again to figure out which AST node produces the problematic logical node. That works okay. Performance might not be great but it only happens in case of errors, so I think it might be okay. I'd like to hear your opinion. But then I started implementing the "column missing from GROUP BY clause" error and that was a bit more difficult (or unreadable) because there's many more layers of functions in between that which has access the AST tree, and the one that checks a single logical expr in the SELECT part of the query and throws the error. I could of course pass down the AST tree, but need I need a `DFSchema` and a `PlannerContext` to normalise the identifiers and correctly go from AST to logical nodes. I find that a bit cumbersome and error prone. I feel like it would be so much easier if we could agree on a way to store `Span` in the logical `Column`. -- 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
