You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "eliaperantoni (via GitHub)" <gi...@apache.org> on 2024/12/05 14:52:04 UTC
[PR] Add related source code locations to errors [datafusion]
eliaperantoni opened a new pull request, #13664:
URL: https://github.com/apache/datafusion/pull/13664
After the PoC that I did in https://github.com/eliaperantoni/datafusion/tree/eper/inline-diagnostics, in this PR I'm attempting to build a more robust solution, with the goal of eventually merging it.
Still, so far I've only added diagnostics to the `Cannot coerce arithmetic expression .. to valid types` error to get feedback and approval on the general implementation. That way I can make tweaks quickly, before I go extend this to the rest of the repo.
## Which issue does this PR close?
Closes #13662.
## What changes are included in this PR?
I introduce the `Diagnostic`, `DiagnosticEntry`, and `DiagnosticEntryKind` types to enrich errors with better user-facing information, mainly by referring to the portions of the SQL code that caused it.
A `DiagnosticEntry` is used to refer to one particular portion of SQL code and describe its involvement in the error with a string message and a `kind`. A `Diagnostic` is simply a collection of `DiagnosticEntry`.
For example, in `SELECT a_string + an_integer` we'd have:
- Entry `kind=Error` `message=Incompatible types in binary expression` Wrapping the whole `a_string + an_integer` expression.
- Entry `kind=Note` `message=Is of type Utf8` Wrapping just the left side.
- Entry `kind=Note` `message=Is of type Int64` Wrapping just the right side.
A new `DataFusionError::Diagnostic` variant is used to attach `Diagnostic` to errors. You can call `with_diagnostic` on any `DataFusionError` to do so. They can later be retrieved by calling `DataFusionError::get_diagnostics`.
It is possible to attach multiple `Diagnostic` while an error is being returned along the function call stack. The rationale here is that different functions might have different levels of detail, but they might all have useful diagnostics to add. e.g. the function that computes the output type of an expression has no idea about why it's being called, but calling functions have no idea about the intricacies of the error. They all know different things.
I also added a `span: Span` to `Column`. This is the first step of a (probably) long process of enriching the types used during logical planning with the `Span`s from the parsed AST nodes.
Added `WithSpan<T>`. The idea is that we want to get `Span` information down to functions called deep in the call stack, but without breaking existing code too much so that we can ensure a smooth and incremental transition to `Diagnostic`-ed errors. `WithSpan<T>` was my attempt to do so because any `T` implements `Into<WithSpan<T>>` by simply attaching `Span::empty` (but of course, if a `Span` is available it should be used instead). This means that any time a function wants to start spawning `Diagnostic`-ed errors, it can change some of its arguments by wrapping their types in `WithSpan<T>` and existing calls will keep on working.
`WithSpan<T>` should be used when the argument type is something that can loosely be traced back to a location in the source code, but is not really part of any AST node, or anything that is isomorphic to it. A good example is `DataType` which is taken by `signature(DataType, Operator, DataType) -> DataType` to get the output type of a binary expression. That is the function that spawns the error, and the caller doesn't have enough information to provide a good `Diagnostic`. But at the same time, it would be (in my opinion) cumbersome to add `Span` arguments to the function. So instead, the function can wrap the `DataType`s for the two sides of the expression in `WithSpan<T>`. Some call points can be updated to pass a `Span`-enriched `WithSpan<DataType>`, and all the others will keep on working without changes required.
## Are these changes tested?
Not yet, while this a draft to gather feedback.
## Are there any user-facing changes?
Yes. The user can call `DataFusionError::get_diagnostics` to get an iterator over all the `Diagnostic` that have been attached to the error chain. Those contain source code locations that relate to the error, and each location comes with a message.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931814763
##########
datafusion/expr/src/expr_rewriter/mod.rs:
##########
@@ -181,10 +178,14 @@ pub fn create_col_from_scalar_expr(
Some::<TableReference>(subqry_alias.into()),
name,
)),
- Expr::Column(Column { relation: _, name }) => Ok(Column::new(
- Some::<TableReference>(subqry_alias.into()),
+ Expr::Column(Column {
+ relation: _,
name,
- )),
+ spans,
+ }) => Ok(
+ Column::new(Some::<TableReference>(subqry_alias.into()), name)
+ .with_spans(spans.clone()),
+ ),
Review Comment:
Good idea!
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1932058075
##########
datafusion/common/src/column.rs:
##########
@@ -254,6 +288,19 @@ impl Column {
.collect(),
})
}
+
+ pub fn spans(&self) -> &Spans {
Review Comment:
Added :)
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2629041356
And merged up again due to another conflict... 🤦
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2628924982
Update here is we are very close to cutting the 45 release branch. See
- More details on https://github.com/apache/datafusion/issues/14008#issuecomment-2628923232
Once we do that I'll plan to merge this PR
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1882461644
##########
datafusion/common/src/column.rs:
##########
@@ -254,6 +305,23 @@ impl Column {
.collect(),
})
}
+
+ /// Attaches a [`Span`] to the [`Column`], i.e. its location in the source
+ /// SQL query.
+ pub fn with_span(mut self, span: Span) -> Self {
+ self.spans = vec![span];
Review Comment:
should this perhaps append a span to the Column rather than overwrite it?
##########
datafusion/common/src/dfschema/mod.rs:
##########
@@ -113,6 +117,10 @@ pub struct DFSchema {
field_qualifiers: Vec<Option<TableReference>>,
/// Stores functional dependencies in the schema.
functional_dependencies: FunctionalDependencies,
+ /// The location in the source code where the fields are defined (e.g. in
Review Comment:
This seems somewhat strange to me as the schema may not be defined in the query
Maybe we can avoid adding this as part of V1
##########
datafusion/common/src/column.rs:
##########
@@ -18,22 +18,35 @@
//! Column
use arrow_schema::{Field, FieldRef};
+use sqlparser::tokenizer::Span;
use crate::error::_schema_err;
use crate::utils::{parse_identifiers_normalized, quote_identifier};
use crate::{DFSchema, Result, SchemaError, TableReference};
+use derivative::Derivative;
use std::collections::HashSet;
use std::convert::Infallible;
use std::fmt;
use std::str::FromStr;
/// A named reference to a qualified field in a schema.
-#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
+#[derive(Debug, Clone, Derivative)]
+#[derivative(PartialEq, Eq, Hash, PartialOrd, Ord)]
pub struct Column {
/// relation/table reference.
pub relation: Option<TableReference>,
/// field/column name.
pub name: String,
+ #[derivative(
Review Comment:
From my perspective some design goals should be:
1. Make it easy for someone reading the DataFusion source code to understand what is going on
2. If people are not interested in the span / diagnostic information they can ignore it
From my perspective, using new crates does reduce the amount of code in the DataFusion crate, but can often increase cognitive load (as now to understand the DataFusion code you need to understand the crates)
> I think the AttachedToken approach that we took in sqlparser could be a functionally equivalent alternative. And I understand that not everybody might be familiar with derivative.
Yeah, that is why I think AttachedToken might be better in this case
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1922380453
##########
datafusion/common/src/error.rs:
##########
@@ -481,8 +488,56 @@ impl DataFusionError {
Cow::Owned(format!("{desc}\ncaused by\n{}", *err))
}
DataFusionError::Substrait(ref desc) => Cow::Owned(desc.to_string()),
+ DataFusionError::Diagnostic(_, ref err) => Cow::Owned(err.to_string()),
}
}
+
+ /// Wraps the error with contextual information intended for end users
+ pub fn with_diagnostic(self, diagnostic: Diagnostic) -> Self {
+ Self::Diagnostic(diagnostic, Box::new(self))
+ }
+
+ /// Wraps the error with contextual information intended for end users.
+ /// Takes a function that inspects the error and returns the diagnostic to
+ /// wrap it with.
+ pub fn with_diagnostic_fn<F: FnOnce(&DataFusionError) -> Diagnostic>(
+ self,
+ f: F,
+ ) -> Self {
+ let diagnostic = f(&self);
+ self.with_diagnostic(diagnostic)
+ }
+
+ pub fn get_diagnostics(&self) -> impl Iterator<Item = &Diagnostic> + '_ {
Review Comment:
Minor -- I think a more consistent API would be to call this
```suggestion
pub fn diagnostics(&self) -> impl Iterator<Item = &Diagnostic> + '_ {
```
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1873007250
##########
datafusion/common/src/column.rs:
##########
@@ -18,22 +18,35 @@
//! Column
use arrow_schema::{Field, FieldRef};
+use sqlparser::tokenizer::Span;
use crate::error::_schema_err;
use crate::utils::{parse_identifiers_normalized, quote_identifier};
use crate::{DFSchema, Result, SchemaError, TableReference};
+use derivative::Derivative;
use std::collections::HashSet;
use std::convert::Infallible;
use std::fmt;
use std::str::FromStr;
/// A named reference to a qualified field in a schema.
-#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
+#[derive(Debug, Clone, Derivative)]
+#[derivative(PartialEq, Eq, Hash, PartialOrd, Ord)]
pub struct Column {
/// relation/table reference.
pub relation: Option<TableReference>,
/// field/column name.
pub name: String,
+ #[derivative(
Review Comment:
I think the [`AttachedToken`](https://github.com/apache/datafusion-sqlparser-rs/blob/main/src/ast/helpers/attached_token.rs) approach that we took in `sqlparser` could be a functionally equivalent alternative. And I understand that not everybody might be familiar with `derivative`.
What I don't 100% love about it is that the `PartialEq` implementation on `AttachedToken` is a bit "impure". In that, it serves the one specific purpose of making it ignored when used in a struct field, but prevents you from actually comparing two instances of it because they would always are equal (the implementation returns always `true`), which is something you might want to do at some point. Especially because the name `AttachedToken` doesn't clearly convey that intent, and does more than just being a passthrough for `PartialEq` since its main purpose is tying together a `Span` and a `Token`.
When looking at a struct that uses it:
```rust
#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
pub struct Select {
/// Token for the `SELECT` keyword
pub select_token: AttachedToken,
// ...
}
```
it's not clear to me that `AttachedToken` is being ignored. I only realise that when I go look at the implementation of `PartialEq`.
Overall, the `derivative` approach to me more clearly lets me keep the sensible implementation of `PartialEq` for `Span`, while also conveying that "when used as a field in this particular struct, I want it to not influence the struct's comparison". But in other structs, I might want it to influence the comparison.
I personally find this approach more readable and flexible, but I'm open to converting to a wrapper type :)
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1872916109
##########
datafusion/common/src/error.rs:
##########
@@ -131,6 +132,7 @@ pub enum DataFusionError {
/// Errors from either mapping LogicalPlans to/from Substrait plans
/// or serializing/deserializing protobytes to Substrait plans
Substrait(String),
+ Diagnostic(Diagnostic, Box<DataFusionError>),
Review Comment:
Thank you! Yes, I think `DataFusion::Diagnostic` can convey a superset of the information that `DataFusion::Context` can. Any wrapping such as:
```rust
let error = DataFusionError::Context(message, Box::new(error));
```
can be converted to:
```rust
let error = DataFusionError::Diagnostic(
Diagnostic {
entries: vec![
DiagnosticEntry {
span: Span::empty(),
kind: DiagnosticEntryKind::Error,
message,
}
]
},
Box::new(error)
);
```
And of course, we can provide a `DataFusionError::with_simple_diagnostic` function to avoid the boilerplate. At that point, `DataFusion::Context` could be removed.
This also enables progressively adding `Span` information to what was previously simply a string message.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931247591
##########
datafusion/common/src/column.rs:
##########
@@ -254,6 +288,19 @@ impl Column {
.collect(),
})
}
+
+ pub fn spans(&self) -> &Spans {
Review Comment:
Please add comments for public methods
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2629058844
🚀
BTW @mkarbo and @eliaperantoni it would be great to write a blog post about this feature / solicit some additional help
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2539384317
Sorry for my radio silence here -- I am reviewing this now. Thank you for your patience
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2630794284
Thank you for the feedback and for guiding us through the process @alamb 🙏
I just filed an epic ticket with those errors that I mentioned earlier.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931825255
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -68,11 +70,57 @@ impl Signature {
}
}
-/// Returns a [`Signature`] for applying `op` to arguments of type `lhs` and `rhs`
-fn signature(lhs: &DataType, op: &Operator, rhs: &DataType) -> Result<Signature> {
- use arrow::datatypes::DataType::*;
- use Operator::*;
- match op {
+pub struct BinaryTypeCoercer<'a> {
+ lhs: &'a DataType,
+ op: &'a Operator,
+ rhs: &'a DataType,
+
+ lhs_spans: Spans,
+ op_spans: Spans,
+ rhs_spans: Spans,
+}
+
+impl<'a> BinaryTypeCoercer<'a> {
+ pub fn new(lhs: &'a DataType, op: &'a Operator, rhs: &'a DataType) -> Self {
+ Self {
+ lhs,
+ op,
+ rhs,
+ lhs_spans: Spans::new(),
+ op_spans: Spans::new(),
+ rhs_spans: Spans::new(),
+ }
+ }
+
+ pub fn set_lhs_spans(&mut self, spans: Spans) {
+ self.lhs_spans = spans;
+ }
+
+ pub fn set_op_spans(&mut self, spans: Spans) {
+ self.op_spans = spans;
+ }
+
+ pub fn set_rhs_spans(&mut self, spans: Spans) {
+ self.rhs_spans = spans;
+ }
+
+ fn span(&self) -> Span {
+ Span::union_iter(
+ [
+ self.lhs_spans.first_or_empty(),
Review Comment:
Because in this scenario I expect the `Spans` to only have one element anyway, since we're looking at one expression in a binary expression.
`Spans` can have multiple spans in other scenarios (e.g. if the `Column` comes out of a CTE with a `UNION`), but that's not expected here.
I don't think it would make a difference to change it.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931890521
##########
datafusion/common/src/column.rs:
##########
@@ -18,10 +18,11 @@
//! Column
use arrow_schema::{Field, FieldRef};
+use sqlparser::tokenizer::Span;
Review Comment:
> That being said the only alternative I have is to basically add a Span in DataFusion that is basically a copy/paste version of the one in SQLParser
Did that 😊
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-5937833126
> Thanks @alamb I was planning to attempt [datafusion-contrib/datafusion-dft#266](https://github.com/datafusion-contrib/datafusion-dft/issues/266) - that's the reason for my question. Do I understand it right that Diagnostic is not used anywhere yet?
I am not sure
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "qrilka (via GitHub)" <gi...@apache.org>.
qrilka commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-5936993833
Thanks @alamb
I was planning to attempt https://github.com/datafusion-contrib/datafusion-dft/issues/266 - that's the reason for my question.
Do I understand it right that Diagnostic is not used anywhere yet?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2602392563
I am starting to check this PR out
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931822190
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -68,11 +70,57 @@ impl Signature {
}
}
-/// Returns a [`Signature`] for applying `op` to arguments of type `lhs` and `rhs`
-fn signature(lhs: &DataType, op: &Operator, rhs: &DataType) -> Result<Signature> {
- use arrow::datatypes::DataType::*;
- use Operator::*;
- match op {
+pub struct BinaryTypeCoercer<'a> {
Review Comment:
Added
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931249289
##########
datafusion/common/src/diagnostic.rs:
##########
@@ -0,0 +1,112 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use sqlparser::tokenizer::Span;
+
+/// Additional contextual information intended for end users, to help them
+/// understand what went wrong by providing human-readable messages, and
+/// locations in the source query that relate to the error in some way.
+///
+/// You can think of a single [`Diagnostic`] as a single "block" of output from
+/// rustc. i.e. either an error or a warning, optionally with some notes and
+/// help messages.
+///
+/// If the diagnostic, a note, or a help message doesn't need to point to a
+/// specific location in the original SQL query (or the [`Span`] is not
+/// available), use [`Span::empty`].
+///
+/// Example:
+///
+/// ```rust
+/// # use datafusion_common::Diagnostic;
+/// # use sqlparser::tokenizer::Span;
+/// # let span = Span::empty();
+/// let diagnostic = Diagnostic::new_error("Something went wrong", span)
+/// .with_help("Have you tried turning it on and off again?", Span::empty());
+/// ```
+#[derive(Debug, Clone)]
+pub struct Diagnostic {
+ pub kind: DiagnosticKind,
+ pub message: String,
+ pub span: Span,
+ pub notes: Vec<DiagnosticNote>,
+ pub helps: Vec<DiagnosticHelp>,
+}
+
+#[derive(Debug, Clone)]
+pub struct DiagnosticNote {
Review Comment:
I think we need to document all the public structures and methods to let the user/dev faster get the abstractions
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931797862
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -68,11 +70,57 @@ impl Signature {
}
}
-/// Returns a [`Signature`] for applying `op` to arguments of type `lhs` and `rhs`
-fn signature(lhs: &DataType, op: &Operator, rhs: &DataType) -> Result<Signature> {
- use arrow::datatypes::DataType::*;
- use Operator::*;
- match op {
+pub struct BinaryTypeCoercer<'a> {
+ lhs: &'a DataType,
+ op: &'a Operator,
+ rhs: &'a DataType,
+
+ lhs_spans: Spans,
Review Comment:
We need the spans of both sides individually though, to produce diagnostics like the following:
Maybe it's not super clear from the screenshot but each `note: has type X` is highlighting just one side of the expression.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931802942
##########
datafusion/sql/src/set_expr.rs:
##########
@@ -36,8 +40,17 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
right,
set_quantifier,
} => {
- let left_plan = self.set_expr_to_plan(*left, planner_context)?;
- let right_plan = self.set_expr_to_plan(*right, planner_context)?;
+ let left_plan = self.set_expr_to_plan(*left.clone(), planner_context)?;
Review Comment:
Yes absolutely
##########
datafusion/sql/src/expr/mod.rs:
##########
@@ -165,7 +165,9 @@ 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:
It works! Thank you :)
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1873112786
##########
datafusion/common/src/with_span.rs:
##########
@@ -0,0 +1,96 @@
+use std::{cmp::Ordering, fmt::{self, Debug, Display}, ops::{Deref, DerefMut}};
+
+use sqlparser::tokenizer::Span;
+
+/// A wrapper type for entitites that somehow refer to a location in the source
Review Comment:
> Did you consider a trait like was done in sqlparser that retrieves spans?
Yes, but to implement one such trait I'd have to put the data in `DataType`. And I think that's a bit pushing the limits of what `DataType` is meant to do. I quite like the idea of the `WithSpan` (can rename) functioning like a sidecar, to limit big changes in the code.
Maybe we could have a `Spanned` trait in datafusion as well, that is automatically implemented for all `WithSpan<T>`, and also by types that natively carry a `Span`?
> Though maybe now would be a good time to make a proper structure for coercing types, (struct TypeCoercer or something) where we could attach the spans 🤔
Not too familiar about the history of that component. But I think that's just one example, and there's probably many more cases where changes like that would be needed to accommodate the presence of `Span`s.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931790064
##########
datafusion/common/src/config.rs:
##########
@@ -250,6 +250,11 @@ config_namespace! {
/// specified. The Arrow type system does not have a notion of maximum
/// string length and thus DataFusion can not enforce such limits.
pub support_varchar_with_length: bool, default = true
+
+ /// When set to true, the source locations relative to the original SQL
Review Comment:
And that's okay I think, I wanted this PR to be more digestible and a PoC for what can be achieved with `Diagnostic`. Rolling it out to more places will come later.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931899154
##########
datafusion/common/src/diagnostic.rs:
##########
@@ -0,0 +1,112 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use sqlparser::tokenizer::Span;
+
+/// Additional contextual information intended for end users, to help them
+/// understand what went wrong by providing human-readable messages, and
+/// locations in the source query that relate to the error in some way.
+///
+/// You can think of a single [`Diagnostic`] as a single "block" of output from
+/// rustc. i.e. either an error or a warning, optionally with some notes and
+/// help messages.
+///
+/// If the diagnostic, a note, or a help message doesn't need to point to a
+/// specific location in the original SQL query (or the [`Span`] is not
+/// available), use [`Span::empty`].
+///
+/// Example:
+///
+/// ```rust
+/// # use datafusion_common::Diagnostic;
+/// # use sqlparser::tokenizer::Span;
+/// # let span = Span::empty();
+/// let diagnostic = Diagnostic::new_error("Something went wrong", span)
+/// .with_help("Have you tried turning it on and off again?", Span::empty());
+/// ```
+#[derive(Debug, Clone)]
+pub struct Diagnostic {
+ pub kind: DiagnosticKind,
+ pub message: String,
+ pub span: Span,
Review Comment:
So after this suggestion and the one about adding a `Span` to `datafusion-common`, I ended up designing it so that a `Span` is always a known location. And then there's a `Span::try_from_sqlparser_span(span: sqlparser::tokenizer::Span) -> Option<Span>` function to do the conversion.
I think it's a lot easier to reason about the lack of information when it's `Option<Span>` and not `sqlparser::tokenizer::Span::EMPTY`
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931896684
##########
datafusion/expr-common/Cargo.toml:
##########
@@ -41,3 +41,4 @@ arrow = { workspace = true }
datafusion-common = { workspace = true }
itertools = { workspace = true }
paste = "^1.0"
+sqlparser = { workspace = true }
Review Comment:
It's not necessary anymore since, after your feedback, I made a semi-copy of `Span` in `datafusion-common` 🥳
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2612103530
Added tests. I tried to find an approach that makes tests easy to read and write so that we don't have to manually edit the numbers of lines and columns like:
```rust
assert_eq!(span, Span {
start: Location {
line: 1,
column: 13
},
start: Location {
line: 1,
column: 18
},
});
```
Instead, you do:
```rust
let query = "SELECT /*a*/namex/*a*/ FROM users";
let spans = get_spans(query);
assert_eq!(span, spans["a"]);
```
The spans are delimited in the query with comments and you can retrieve them easily.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1933723286
##########
datafusion/common/src/diagnostic.rs:
##########
@@ -0,0 +1,112 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use sqlparser::tokenizer::Span;
+
+/// Additional contextual information intended for end users, to help them
+/// understand what went wrong by providing human-readable messages, and
+/// locations in the source query that relate to the error in some way.
+///
+/// You can think of a single [`Diagnostic`] as a single "block" of output from
+/// rustc. i.e. either an error or a warning, optionally with some notes and
+/// help messages.
+///
+/// If the diagnostic, a note, or a help message doesn't need to point to a
+/// specific location in the original SQL query (or the [`Span`] is not
+/// available), use [`Span::empty`].
+///
+/// Example:
+///
+/// ```rust
+/// # use datafusion_common::Diagnostic;
+/// # use sqlparser::tokenizer::Span;
+/// # let span = Span::empty();
+/// let diagnostic = Diagnostic::new_error("Something went wrong", span)
+/// .with_help("Have you tried turning it on and off again?", Span::empty());
+/// ```
+#[derive(Debug, Clone)]
+pub struct Diagnostic {
+ pub kind: DiagnosticKind,
+ pub message: String,
+ pub span: Span,
+ pub notes: Vec<DiagnosticNote>,
+ pub helps: Vec<DiagnosticHelp>,
+}
+
+#[derive(Debug, Clone)]
+pub struct DiagnosticNote {
+ pub message: String,
+ pub span: Span,
+}
+
+#[derive(Debug, Clone)]
+pub struct DiagnosticHelp {
Review Comment:
perhaps that could be added in comments as it may be non obvious to other readers
```suggestion
/// Suggestions as to how to solve the error
///
/// This is different than [`DiagnosticNote`] which explains *why* the error occurred.
/// For exmaple that a non aggregate expression is not in the `GROUP BY` clause.
/// `DiagnosticHelp` contains a suggestion to solve the error, for example
/// add the expression to the `GROUP BY`.
pub struct DiagnosticHelp {
```
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931145846
##########
datafusion/sqllogictest/test_files/create_external_table.slt:
##########
@@ -33,23 +33,23 @@ statement error DataFusion error: SQL error: ParserError\("Missing LOCATION clau
CREATE EXTERNAL TABLE t STORED AS CSV
# Option value is missing
-statement error DataFusion error: SQL error: ParserError\("Expected: string or numeric value, found: \)"\)
+statement error DataFusion error: SQL error: ParserError\("Expected: string or numeric value, found: \) at Line: 1, Column: 66"\)
Review Comment:
😍
##########
datafusion/common/src/column.rs:
##########
@@ -18,10 +18,11 @@
//! Column
use arrow_schema::{Field, FieldRef};
+use sqlparser::tokenizer::Span;
Review Comment:
I think this direct dependence on `sqlparser` is non ideal (because it makes the DataFuson `Expr` directly dependent on the SQL parser version.
That being said the only alternative I have is to basically add a `Span` in `DataFusion` that is basically a copy/paste version of the one in SQLParser
##########
datafusion/sql/tests/cases/diagnostic.rs:
##########
@@ -0,0 +1,231 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::collections::HashMap;
+
+use datafusion_common::{Diagnostic, Result};
+use datafusion_sql::planner::{ParserOptions, SqlToRel};
+use regex::Regex;
+use sqlparser::{
+ dialect::GenericDialect,
+ parser::Parser,
+ tokenizer::{Location, Span},
+};
+
+use crate::{MockContextProvider, MockSessionState};
+
+fn do_query(sql: &'static str) -> Diagnostic {
+ let dialect = GenericDialect {};
+ let statement = Parser::new(&dialect)
+ .try_with_sql(sql)
+ .expect("unable to create parser")
+ .parse_statement()
+ .expect("unable to parse query");
+ let options = ParserOptions {
+ collect_spans: true,
+ ..ParserOptions::default()
+ };
+ let state = MockSessionState::default();
+ let context = MockContextProvider { state };
+ let sql_to_rel = SqlToRel::new_with_options(&context, options);
+ match sql_to_rel.sql_statement_to_plan(statement) {
+ Ok(_) => panic!("expected error"),
+ Err(err) => match err.diagnostic() {
+ Some(diag) => diag.clone(),
+ None => panic!("expected diagnostic"),
+ },
+ }
+}
+
+/// Given a query that contains tag delimited spans, returns a mapping from the
+/// span name to the [`Span`]. Tags are comments of the form `/*tag*/`. In case
+/// you want the same location to open two spans, or close open and open
+/// another, you can use '+' to separate the names (the order matters). The
+/// names of spans can only contain letters, digits, underscores, and dashes.
+///
+///
+/// ## Example
+///
+/// ```rust
+/// let spans = get_spans("SELECT /*myspan*/car/*myspan*/ FROM cars");
Review Comment:
this is very cool
##########
datafusion/common/src/config.rs:
##########
@@ -250,6 +250,11 @@ config_namespace! {
/// specified. The Arrow type system does not have a notion of maximum
/// string length and thus DataFusion can not enforce such limits.
pub support_varchar_with_length: bool, default = true
+
+ /// When set to true, the source locations relative to the original SQL
Review Comment:
Is the idea that we will keep this off by default at first as we evaluate the impact it has on performance?
##########
datafusion/expr-common/Cargo.toml:
##########
@@ -41,3 +41,4 @@ arrow = { workspace = true }
datafusion-common = { workspace = true }
itertools = { workspace = true }
paste = "^1.0"
+sqlparser = { workspace = true }
Review Comment:
this new dependecy is also unfortunate, but I see it is required as the BinaryTypeCoercer needs to know about spans
##########
datafusion/expr/src/expr.rs:
##########
@@ -1663,6 +1667,13 @@ impl Expr {
| Expr::Placeholder(..) => false,
}
}
+
+ pub fn spans(&self) -> Option<&Spans> {
Review Comment:
Can you please add comments about what `Spans` means in this context?
##########
datafusion/common/src/column.rs:
##########
@@ -34,6 +35,7 @@ pub struct Column {
pub relation: Option<TableReference>,
/// field/column name.
pub name: String,
+ pub spans: Spans,
Review Comment:
```suggestion
/// Original source code location, if known
pub spans: Spans,
```
##########
datafusion/common/src/diagnostic.rs:
##########
@@ -0,0 +1,112 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use sqlparser::tokenizer::Span;
+
+/// Additional contextual information intended for end users, to help them
+/// understand what went wrong by providing human-readable messages, and
+/// locations in the source query that relate to the error in some way.
+///
+/// You can think of a single [`Diagnostic`] as a single "block" of output from
+/// rustc. i.e. either an error or a warning, optionally with some notes and
+/// help messages.
+///
+/// If the diagnostic, a note, or a help message doesn't need to point to a
+/// specific location in the original SQL query (or the [`Span`] is not
+/// available), use [`Span::empty`].
+///
+/// Example:
+///
+/// ```rust
+/// # use datafusion_common::Diagnostic;
+/// # use sqlparser::tokenizer::Span;
+/// # let span = Span::empty();
+/// let diagnostic = Diagnostic::new_error("Something went wrong", span)
+/// .with_help("Have you tried turning it on and off again?", Span::empty());
+/// ```
+#[derive(Debug, Clone)]
+pub struct Diagnostic {
+ pub kind: DiagnosticKind,
+ pub message: String,
+ pub span: Span,
Review Comment:
Since we don't always have a `Span` I think this would make more sense to be
```suggestion
pub span: Option<Span>,
```
Which would make it easier to tell when we didn't have any Span information (and would likely avoid the use of `first_or_empty`)
##########
datafusion/sql/tests/cases/diagnostic.rs:
##########
@@ -0,0 +1,231 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::collections::HashMap;
Review Comment:
This is a really nice way to test things
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -68,11 +70,57 @@ impl Signature {
}
}
-/// Returns a [`Signature`] for applying `op` to arguments of type `lhs` and `rhs`
-fn signature(lhs: &DataType, op: &Operator, rhs: &DataType) -> Result<Signature> {
- use arrow::datatypes::DataType::*;
- use Operator::*;
- match op {
+pub struct BinaryTypeCoercer<'a> {
+ lhs: &'a DataType,
+ op: &'a Operator,
+ rhs: &'a DataType,
+
+ lhs_spans: Spans,
Review Comment:
What about simply having a single `spans: Span` that is computed on construction? Is there any reason to keep around the different set of spans?
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -68,11 +70,57 @@ impl Signature {
}
}
-/// Returns a [`Signature`] for applying `op` to arguments of type `lhs` and `rhs`
-fn signature(lhs: &DataType, op: &Operator, rhs: &DataType) -> Result<Signature> {
- use arrow::datatypes::DataType::*;
- use Operator::*;
- match op {
+pub struct BinaryTypeCoercer<'a> {
Review Comment:
I like the idea of using this struct rather than a free function. If we are going to make it a `pub struct` I think it should have comments explaining what it does
##########
datafusion/expr/src/expr_rewriter/mod.rs:
##########
@@ -181,10 +178,14 @@ pub fn create_col_from_scalar_expr(
Some::<TableReference>(subqry_alias.into()),
name,
)),
- Expr::Column(Column { relation: _, name }) => Ok(Column::new(
- Some::<TableReference>(subqry_alias.into()),
+ Expr::Column(Column {
+ relation: _,
name,
- )),
+ spans,
+ }) => Ok(
+ Column::new(Some::<TableReference>(subqry_alias.into()), name)
+ .with_spans(spans.clone()),
+ ),
Review Comment:
You might be able to make this look nicer (and less likely to miss the span) via something like
```rust
Expr::Column(col) => col.with_qualifier(subqry_alias.into())
```
##########
datafusion/sql/src/expr/mod.rs:
##########
@@ -165,7 +165,9 @@ 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:
We can now remove this clone, right? I tried it locally and it worked fine
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -68,11 +70,57 @@ impl Signature {
}
}
-/// Returns a [`Signature`] for applying `op` to arguments of type `lhs` and `rhs`
-fn signature(lhs: &DataType, op: &Operator, rhs: &DataType) -> Result<Signature> {
- use arrow::datatypes::DataType::*;
- use Operator::*;
- match op {
+pub struct BinaryTypeCoercer<'a> {
+ lhs: &'a DataType,
+ op: &'a Operator,
+ rhs: &'a DataType,
+
+ lhs_spans: Spans,
+ op_spans: Spans,
+ rhs_spans: Spans,
+}
+
+impl<'a> BinaryTypeCoercer<'a> {
+ pub fn new(lhs: &'a DataType, op: &'a Operator, rhs: &'a DataType) -> Self {
+ Self {
+ lhs,
+ op,
+ rhs,
+ lhs_spans: Spans::new(),
+ op_spans: Spans::new(),
+ rhs_spans: Spans::new(),
+ }
+ }
+
+ pub fn set_lhs_spans(&mut self, spans: Spans) {
+ self.lhs_spans = spans;
+ }
+
+ pub fn set_op_spans(&mut self, spans: Spans) {
+ self.op_spans = spans;
+ }
+
+ pub fn set_rhs_spans(&mut self, spans: Spans) {
+ self.rhs_spans = spans;
+ }
+
+ fn span(&self) -> Span {
+ Span::union_iter(
+ [
+ self.lhs_spans.first_or_empty(),
Review Comment:
Why use `first_or_empty` if you are just going to union them together?
Would this be the same?
```rust
Span::union_iter(
self.lhs_spans.iter().chain(self.op_spans.iter()).chain(self.rhs_spans.iter())
.filter(|&&span| span != Span::empty())
.copied()
)
```
(no change needed, just checking my understanding)
##########
datafusion/sql/src/set_expr.rs:
##########
@@ -36,8 +40,17 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
right,
set_quantifier,
} => {
- let left_plan = self.set_expr_to_plan(*left, planner_context)?;
- let right_plan = self.set_expr_to_plan(*right, planner_context)?;
+ let left_plan = self.set_expr_to_plan(*left.clone(), planner_context)?;
Review Comment:
Can we please try and avoid this clone ? It looks like only the left and right spans are needed so we could copy them out rather than the entire set expr?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-5936302275
> Hi @alamb @mkarbo I suppose there was no blog post about Diagnostic, only the section https://datafusion.apache.org/blog/2025/03/24/datafusion-46.0.0/#improved-diagnostics ?
yes unfortunately I don't think we wrote anything more on it and @eliaperantoni has since moved on to other projects from what I can tell and this one has lost steam
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2612172667
THanks @eliaperantoni -- I started the CI and will checkout this PR in the next day or two
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2610077654
@alamb
As agreed, I implemented all the errors that I previously implemented but in a less invasive way, only adding `Span` to `Column`. Here's some examples:
<img width="500" src="https://github.com/user-attachments/assets/3affccc3-3f9f-4d37-8b4c-f04e2164c139"/>
<img width="500" src="https://github.com/user-attachments/assets/574aa784-0c7d-43fe-a552-2b2342ea53ab"/>
<img width="500" src="https://github.com/user-attachments/assets/51582c07-66e4-40aa-a52b-76a6ee810f0b"/>
<img width="500" src="https://github.com/user-attachments/assets/3fd368f4-ac68-41f8-822e-60e47288d408"/>
<img width="700" src="https://github.com/user-attachments/assets/72a8dc89-b803-4eb6-852d-cdf3e65c6f1f"/>
<img width="700" src="https://github.com/user-attachments/assets/32efeb4f-aad9-41c9-a1e3-600a00d525c3"/>
<img width="500" src="https://github.com/user-attachments/assets/7a845f8d-b1fe-47da-8060-1ee85dbf7f15"/>
Note that the particular way that we show the diagnostics here is something that we have implemented and is not part of the PR. We think DataFusion shouldn't be too opinionated about the way the data is displayed to the user, but only provide it to a frontend. So CLI output like this can be built just as well as an LSP, a VSCode extension, a bare JSON report, etc.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931789193
##########
datafusion/common/src/config.rs:
##########
@@ -250,6 +250,11 @@ config_namespace! {
/// specified. The Arrow type system does not have a notion of maximum
/// string length and thus DataFusion can not enforce such limits.
pub support_varchar_with_length: bool, default = true
+
+ /// When set to true, the source locations relative to the original SQL
Review Comment:
Yeah and also until we produce diagnostics for more of the most common errors, or so I thought. Right now it's very few errors.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1933729644
##########
datafusion/sql/src/utils.rs:
##########
@@ -90,12 +90,35 @@ pub(crate) fn rebase_expr(
.data()
}
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
Review Comment:
this is quite nice
##########
datafusion/expr/src/expr_rewriter/mod.rs:
##########
@@ -181,10 +178,7 @@ pub fn create_col_from_scalar_expr(
Some::<TableReference>(subqry_alias.into()),
name,
)),
- Expr::Column(Column { relation: _, name }) => Ok(Column::new(
- Some::<TableReference>(subqry_alias.into()),
- name,
- )),
+ Expr::Column(col) => Ok(col.with_relation(subqry_alias.into())),
Review Comment:
😍
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "mkarbo (via GitHub)" <gi...@apache.org>.
mkarbo commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2629303588
We are planning to write a blog on this and on the sqlparser work in the very near future!
And yes we can file that during the week.
Thanks for the help everyone!
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2629059247
@eliaperantoni / @mkarbo -- any chance you can file a `"EPIC" ticket listing the various subtasks you highlighted on https://github.com/apache/datafusion/pull/13664#issuecomment-2620192416
?
That would be super helpful 🙏
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2629032156
I have made a release branch for 45 so let's get this one merged now
- https://github.com/apache/datafusion/issues/14008#issuecomment-2628923232
I merged up from main to resolve a conflict
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "Nyrox (via GitHub)" <gi...@apache.org>.
Nyrox commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1872954407
##########
datafusion/common/src/column.rs:
##########
@@ -18,22 +18,35 @@
//! Column
use arrow_schema::{Field, FieldRef};
+use sqlparser::tokenizer::Span;
use crate::error::_schema_err;
use crate::utils::{parse_identifiers_normalized, quote_identifier};
use crate::{DFSchema, Result, SchemaError, TableReference};
+use derivative::Derivative;
use std::collections::HashSet;
use std::convert::Infallible;
use std::fmt;
use std::str::FromStr;
/// A named reference to a qualified field in a schema.
-#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
+#[derive(Debug, Clone, Derivative)]
+#[derivative(PartialEq, Eq, Hash, PartialOrd, Ord)]
pub struct Column {
/// relation/table reference.
pub relation: Option<TableReference>,
/// field/column name.
pub name: String,
+ #[derivative(
Review Comment:
It is indeed interesting. 😅 I was going to use derivative for this problem in sqlparser initially actually, but I wasn't sure if you would want to add a new dependency. If that is not a concern, I think using derivative for cases like this is pretty nice as it is also clear at the usage site that it is being ignored, unlike `AttachedToken` which kind of hides the behavior. If a wrapper type is preferred, I now think actually maybe the struct in sqlparser should have been called DiagnosticOnly as well 😂
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1932071276
##########
datafusion/sqllogictest/test_files/unnest.slt:
##########
@@ -899,7 +899,7 @@ logical_plan
07)--------Unnest: lists[__unnest_placeholder(outer_ref(u.column1))|depth=1] structs[]
08)----------Projection: outer_ref(u.column1) AS __unnest_placeholder(outer_ref(u.column1))
09)------------EmptyRelation
-physical_plan_error This feature is not implemented: Physical plan does not support logical expression OuterReferenceColumn(List(Field { name: "item", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), Column { relation: Some(Bare { table: "u" }), name: "column1" })
+physical_plan_error This feature is not implemented: Physical plan does not support logical expression OuterReferenceColumn(List(Field { name: "item", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), Column { relation: Some(Bare { table: "u" }), name: "column1", spans: Spans([]) })
Review Comment:
Hmm yeah in general I think I agree that we don't need `Column::Debug` to print the spans. I'll change it.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931798459
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -68,11 +70,57 @@ impl Signature {
}
}
-/// Returns a [`Signature`] for applying `op` to arguments of type `lhs` and `rhs`
-fn signature(lhs: &DataType, op: &Operator, rhs: &DataType) -> Result<Signature> {
- use arrow::datatypes::DataType::*;
- use Operator::*;
- match op {
+pub struct BinaryTypeCoercer<'a> {
+ lhs: &'a DataType,
+ op: &'a Operator,
+ rhs: &'a DataType,
+
+ lhs_spans: Spans,
Review Comment:
They are also merged for the overarching error though, that's true
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "matthewmturner (via GitHub)" <gi...@apache.org>.
matthewmturner commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2619154203
Thanks for the ping @xudong963. Indeed, this is really cool - I haven't had the chance to read the whole thread yet but from what I see I do think this could be a good addition to dft. I've created an [issue](https://github.com/datafusion-contrib/datafusion-dft/issues/266) for it
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2615898483
Ah sorry @alamb those tests didn't fail before. Is there any way I can trigger CI myself?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1923538093
##########
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:
Also it's `validate_schema_satisfies_exprs` that throws the error, not `sql_expr_to_logical_expr`. And in that place I have access to the SQL expression, but the problem still is that I don't know which subexpression was the problem
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1923512091
##########
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:
That's a very interesting idea. I see a few issues with it though:
1.
What if `sql` is a compound expression like `a + b` but only `b` is unresolved? Ideally, I would want `sql_expr_to_logical_expr` to be able to return diagnostics that highlight `b` only. But does it do that if it only know the `Span` of the entire `sql` expression?
2.
What if a diagnostic needs to highlight various parts of a query. e.g. if a non-aggregated column is missing from the `GROUP BY` clause, a good diagnostic would highlight both the column and the `GROUP BY` clause. But planner context has only one `Span`, and which is the "current span" in this case?
I thought that I could solve the above issues by putting a `HashMap<String, Vec<Span>>` in `PlannerContext`. The key would be to label different parts of the query, e.g. `group_by_clause`, `set_operator`, `left_side_of_expr`, etc.. So that I could do something like:
```
error.with_diagnostic(Diagnostic::new().with_error(
"column missing from GROUP BY",
planner_context.span("expr").last(), // Highlight the column
).with_note(
"GROUP BY is here",
planner_context.span("group_by").last(), // Highlight the GROUP BY
))
```
The `Vec<_>` is just to have stacks, so that in something like `SELECT ... UNION (SELECT ... UNION SELECT ...)` the `set_operator` key could point to the outer `UNION`, then the inner one, then restore the outer one when we're done planning the query in parentheses.
But then I encountered another issue:
3.
Some functions where `PlannerContext` is threaded through (e.g. `sql_expr_to_logical_expr` and `sql_expr_to_logical_expr_internal`) take `&mut PlannerContext`. What if I need to add new spans?
For example, in the body of `sql_expr_to_logical_expr` I would like to do:
<img width="874" alt="image" src="https://github.com/user-attachments/assets/82b4d8c8-2d78-4367-8340-50e1dcc067de" />
But, what if `sql_expr_to_logical_expr_internal` needs to mutate the given `&mut PlannerContext`? I can't simply clone `PlannerContext` and add a new span because I then break the ability to mutate downstream.
Then it seems like a pattern like:
```
pattern_context.with_span("expr", sql_expr.span(), move |planner_context| {
let expr = self.sql_expr_to_logical_expr_internal(
*sql_expr,
schema,
planner_context,
)?;
});
```
might be necessary, to add a `Span` to the given `PlannerContext` but also remove it. Idk if that's less invasive than adding `Span` to `Expr` though.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1873102490
##########
datafusion/common/src/column.rs:
##########
@@ -18,22 +18,35 @@
//! Column
use arrow_schema::{Field, FieldRef};
+use sqlparser::tokenizer::Span;
use crate::error::_schema_err;
use crate::utils::{parse_identifiers_normalized, quote_identifier};
use crate::{DFSchema, Result, SchemaError, TableReference};
+use derivative::Derivative;
use std::collections::HashSet;
use std::convert::Infallible;
use std::fmt;
use std::str::FromStr;
/// A named reference to a qualified field in a schema.
-#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
+#[derive(Debug, Clone, Derivative)]
+#[derivative(PartialEq, Eq, Hash, PartialOrd, Ord)]
pub struct Column {
/// relation/table reference.
pub relation: Option<TableReference>,
/// field/column name.
pub name: String,
+ #[derivative(
Review Comment:
> I also see WithSpan has a similar approach (but that is for wrapping things with Spans 🤔 )
Ah yes good catch! I could definitely have used `derivative` for `WithSpan` too. And about `WithSpan`: do you like the general idea of it?
It's meant to be a "sidecar" to get a `Span` all the way to where it's needed to create a meaningful diagnostic, by attaching it to values that are sort-of-related, but wouldn't make a lot of sense to add as a struct field.
For example, the two `DataType`s in a binary expression come from the two expressions in the SQL query, so they (in some sense) relate to a portion of the code. But putting `span` as a field of `DataType` sounds wrong.
Maybe a better name could be `SpanRelated` or something like that?.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1932086369
##########
datafusion/sqllogictest/test_files/create_external_table.slt:
##########
@@ -33,23 +33,23 @@ statement error DataFusion error: SQL error: ParserError\("Missing LOCATION clau
CREATE EXTERNAL TABLE t STORED AS CSV
# Option value is missing
-statement error DataFusion error: SQL error: ParserError\("Expected: string or numeric value, found: \)"\)
+statement error DataFusion error: SQL error: ParserError\("Expected: string or numeric value, found: \) at Line: 1, Column: 66"\)
Review Comment:
Yes it does work for multiline statements, and it's the parser that outputs that error.
```sql
WITH cte1 AS (
SELECT 1 AS id
), cte2 AS (
SELECT 1 AS id
),
SELECT id FROM cte1, cte2, users
-- SQL error: ParserError("Expected: AS, found: id at Line: 6, Column: 8")
```
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2620192416
> I also think before/part of merging this PR would be to document what is needed before we would want to turn on collect_spans by default
In my opinion:
- Putting `Span` on more logical types in order to..
- Attach `Diagnostic` to more errors, until most of the typical errors that we can expect the end user to make are covered
I know it's a little vague but, as a starting point, I would consider:
- Reference to a function that doesn't exist
- Wrong number of parameters in a function call
- Wrong types of parameters in a function call
- Syntax errors
- Subquery that returns more than one column
- Type coercion in unary expressions
- Using `= NULL` instead of `IS NULL`, maybe that could be the first use case of `Diagnostic::new_warning`
- Alias two columns/tables with the same name
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1933725543
##########
datafusion/common/src/diagnostic.rs:
##########
@@ -0,0 +1,112 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use sqlparser::tokenizer::Span;
+
+/// Additional contextual information intended for end users, to help them
+/// understand what went wrong by providing human-readable messages, and
+/// locations in the source query that relate to the error in some way.
+///
+/// You can think of a single [`Diagnostic`] as a single "block" of output from
+/// rustc. i.e. either an error or a warning, optionally with some notes and
+/// help messages.
+///
+/// If the diagnostic, a note, or a help message doesn't need to point to a
+/// specific location in the original SQL query (or the [`Span`] is not
+/// available), use [`Span::empty`].
+///
+/// Example:
+///
+/// ```rust
+/// # use datafusion_common::Diagnostic;
+/// # use sqlparser::tokenizer::Span;
+/// # let span = Span::empty();
+/// let diagnostic = Diagnostic::new_error("Something went wrong", span)
+/// .with_help("Have you tried turning it on and off again?", Span::empty());
+/// ```
+#[derive(Debug, Clone)]
+pub struct Diagnostic {
+ pub kind: DiagnosticKind,
+ pub message: String,
+ pub span: Span,
+ pub notes: Vec<DiagnosticNote>,
+ pub helps: Vec<DiagnosticHelp>,
+}
+
+#[derive(Debug, Clone)]
+pub struct DiagnosticNote {
+ pub message: String,
+ pub span: Span,
+}
+
+#[derive(Debug, Clone)]
+pub struct DiagnosticHelp {
Review Comment:
NM -- I see you already did that
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931250291
##########
datafusion/common/src/diagnostic.rs:
##########
@@ -0,0 +1,112 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use sqlparser::tokenizer::Span;
+
+/// Additional contextual information intended for end users, to help them
+/// understand what went wrong by providing human-readable messages, and
+/// locations in the source query that relate to the error in some way.
+///
+/// You can think of a single [`Diagnostic`] as a single "block" of output from
+/// rustc. i.e. either an error or a warning, optionally with some notes and
+/// help messages.
+///
+/// If the diagnostic, a note, or a help message doesn't need to point to a
+/// specific location in the original SQL query (or the [`Span`] is not
+/// available), use [`Span::empty`].
+///
+/// Example:
+///
+/// ```rust
+/// # use datafusion_common::Diagnostic;
+/// # use sqlparser::tokenizer::Span;
+/// # let span = Span::empty();
+/// let diagnostic = Diagnostic::new_error("Something went wrong", span)
+/// .with_help("Have you tried turning it on and off again?", Span::empty());
+/// ```
+#[derive(Debug, Clone)]
+pub struct Diagnostic {
+ pub kind: DiagnosticKind,
+ pub message: String,
+ pub span: Span,
+ pub notes: Vec<DiagnosticNote>,
+ pub helps: Vec<DiagnosticHelp>,
+}
+
+#[derive(Debug, Clone)]
+pub struct DiagnosticNote {
+ pub message: String,
+ pub span: Span,
+}
+
+#[derive(Debug, Clone)]
+pub struct DiagnosticHelp {
Review Comment:
how DiagnosticNote and DiagnosticHelp are different, looks like the same structure?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2634530176
> Thank you for the feedback and for guiding us through the process @alamb 🙏
>
> I just filed an epic ticket with those errors that I mentioned earlier.
>
> EDIT: I think all of them are good first issues.
Amazing! Thank you @eliaperantoni -- I agree they are good first issues and I left comments and marked them as such.
You also seem to have already found takers for at least 3 of them 😎 🎣 -- very nice
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "qrilka (via GitHub)" <gi...@apache.org>.
qrilka commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-5913442958
Hi @alamb @mkarbo I suppose there was no blog post about Diagnostic, only the section https://datafusion.apache.org/blog/2025/03/24/datafusion-46.0.0/#improved-diagnostics ?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931793970
##########
datafusion/common/src/diagnostic.rs:
##########
@@ -0,0 +1,112 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use sqlparser::tokenizer::Span;
+
+/// Additional contextual information intended for end users, to help them
+/// understand what went wrong by providing human-readable messages, and
+/// locations in the source query that relate to the error in some way.
+///
+/// You can think of a single [`Diagnostic`] as a single "block" of output from
+/// rustc. i.e. either an error or a warning, optionally with some notes and
+/// help messages.
+///
+/// If the diagnostic, a note, or a help message doesn't need to point to a
+/// specific location in the original SQL query (or the [`Span`] is not
+/// available), use [`Span::empty`].
+///
+/// Example:
+///
+/// ```rust
+/// # use datafusion_common::Diagnostic;
+/// # use sqlparser::tokenizer::Span;
+/// # let span = Span::empty();
+/// let diagnostic = Diagnostic::new_error("Something went wrong", span)
+/// .with_help("Have you tried turning it on and off again?", Span::empty());
+/// ```
+#[derive(Debug, Clone)]
+pub struct Diagnostic {
+ pub kind: DiagnosticKind,
+ pub message: String,
+ pub span: Span,
Review Comment:
Yeah I agree with you that `Option` is nicer, but I wanted to avoid having two separate "null" values. We'd still have to check for spans that are equal to `Span::EMPTY` somewhere along the chain, because that's what sqlparser returns.
But I'll make the suggested change, let's see how it looks.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931787601
##########
datafusion/sql/tests/cases/diagnostic.rs:
##########
@@ -0,0 +1,231 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use std::collections::HashMap;
Review Comment:
Thank you! :)
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931255150
##########
datafusion/sqllogictest/test_files/unnest.slt:
##########
@@ -899,7 +899,7 @@ logical_plan
07)--------Unnest: lists[__unnest_placeholder(outer_ref(u.column1))|depth=1] structs[]
08)----------Projection: outer_ref(u.column1) AS __unnest_placeholder(outer_ref(u.column1))
09)------------EmptyRelation
-physical_plan_error This feature is not implemented: Physical plan does not support logical expression OuterReferenceColumn(List(Field { name: "item", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), Column { relation: Some(Bare { table: "u" }), name: "column1" })
+physical_plan_error This feature is not implemented: Physical plan does not support logical expression OuterReferenceColumn(List(Field { name: "item", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), Column { relation: Some(Bare { table: "u" }), name: "column1", spans: Spans([]) })
Review Comment:
I'm wondering if Spans brings a purpose here? Should we exclude it from error message, otherwise it might be confusing
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931253902
##########
datafusion/sqllogictest/test_files/create_external_table.slt:
##########
@@ -33,23 +33,23 @@ statement error DataFusion error: SQL error: ParserError\("Missing LOCATION clau
CREATE EXTERNAL TABLE t STORED AS CSV
# Option value is missing
-statement error DataFusion error: SQL error: ParserError\("Expected: string or numeric value, found: \)"\)
+statement error DataFusion error: SQL error: ParserError\("Expected: string or numeric value, found: \) at Line: 1, Column: 66"\)
Review Comment:
Thats really cool, does that work for multiline statements?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931818796
##########
datafusion/expr/src/expr.rs:
##########
@@ -1663,6 +1667,13 @@ impl Expr {
| Expr::Placeholder(..) => false,
}
}
+
+ pub fn spans(&self) -> Option<&Spans> {
Review Comment:
Added
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2621422574
I merged up from main for the PR to get a clean CI test run
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1923512091
##########
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:
That's a very interesting idea. I see a few issues with it though:
1.
What if `sql` is a compound expression like `a + b` but only `b` is unresolved? Ideally, I would want `sql_expr_to_logical_expr` to be able to return diagnostics that highlight `b` only. But how does it do that if it only know the `Span` of the entire `sql` expression?
2.
What if a diagnostic needs to highlight various parts of a query. e.g. if a non-aggregated column is missing from the `GROUP BY` clause, a good diagnostic would highlight both the column and the `GROUP BY` clause. But planner context has only one `Span`, and which is the "current span" in this case?
I thought that I could solve the above issues by putting a `HashMap<String, Vec<Span>>` in `PlannerContext`. The key would be to label different parts of the query, e.g. `group_by_clause`, `set_operator`, `left_side_of_expr`, etc.. So that I could do something like:
```
error.with_diagnostic(Diagnostic::new().with_error(
"column missing from GROUP BY",
planner_context.span("expr").last(), // Highlight the column
).with_note(
"GROUP BY is here",
planner_context.span("group_by").last(), // Highlight the GROUP BY
))
```
The `Vec<_>` is just to have stacks, so that in something like `SELECT ... UNION (SELECT ... UNION SELECT ...)` the `set_operator` key could point to the outer `UNION`, then the inner one, then restore the outer one when we're done planning the query in parentheses.
But then I encountered another issue:
3.
Some functions where `PlannerContext` is threaded through (e.g. `sql_expr_to_logical_expr` and `sql_expr_to_logical_expr_internal`) take `&mut PlannerContext`. What if I need to add new spans?
For example, in the body of `sql_expr_to_logical_expr` I would like to do:
<img width="874" alt="image" src="https://github.com/user-attachments/assets/82b4d8c8-2d78-4367-8340-50e1dcc067de" />
But, what if `sql_expr_to_logical_expr_internal` needs to mutate the given `&mut PlannerContext`? I can't simply clone `PlannerContext` and add a new span because I then break the ability to mutate downstream.
Then it seems like a pattern like:
```
pattern_context.with_span("expr", sql_expr.span(), move |planner_context| {
let expr = self.sql_expr_to_logical_expr_internal(
*sql_expr,
schema,
planner_context,
)?;
});
```
might be necessary, to add a `Span` to the given `PlannerContext` but also remove it. Idk if that's less invasive than adding `Span` to `Expr` though.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1871897489
##########
datafusion/common/src/error.rs:
##########
@@ -131,6 +132,7 @@ pub enum DataFusionError {
/// Errors from either mapping LogicalPlans to/from Substrait plans
/// or serializing/deserializing protobytes to Substrait plans
Substrait(String),
+ Diagnostic(Diagnostic, Box<DataFusionError>),
Review Comment:
This is an interesting idea -- one way to think about this is that it adds additional structured information to `DataFusionError::Context`
If we went with the `DataFusion::Diagnostic` approach, do you think we would be able to deprecate / remove `DataFusionError::Context` in a future release?
##########
datafusion/common/src/column.rs:
##########
@@ -18,22 +18,35 @@
//! Column
use arrow_schema::{Field, FieldRef};
+use sqlparser::tokenizer::Span;
use crate::error::_schema_err;
use crate::utils::{parse_identifiers_normalized, quote_identifier};
use crate::{DFSchema, Result, SchemaError, TableReference};
+use derivative::Derivative;
use std::collections::HashSet;
use std::convert::Infallible;
use std::fmt;
use std::str::FromStr;
/// A named reference to a qualified field in a schema.
-#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
+#[derive(Debug, Clone, Derivative)]
+#[derivative(PartialEq, Eq, Hash, PartialOrd, Ord)]
pub struct Column {
/// relation/table reference.
pub relation: Option<TableReference>,
/// field/column name.
pub name: String,
+ #[derivative(
Review Comment:
this is an interesting pattern -- it is different than the way sqlparser did it (which is to effectively wrap the span with a struct that is ignored when comparing eq, hash, etc
Did you consider a similar approach?
```rust
pub struct Column {
...
// Wrapped span that does not contribute to PartialEq, Eq, Hash, etc
pub span: DiagnosticOnly<Span>
}
```
I think this approach is less verbose and might be easier to understand for the casual reader
##########
datafusion/common/src/with_span.rs:
##########
@@ -0,0 +1,96 @@
+use std::{cmp::Ordering, fmt::{self, Debug, Display}, ops::{Deref, DerefMut}};
+
+use sqlparser::tokenizer::Span;
+
+/// A wrapper type for entitites that somehow refer to a location in the source
Review Comment:
Did you consider a trait like was done in sqlparser that retrieves spans? For example
```rust
pub trait Spanned {
/// return the span for this object, if known
fn span(&self) -> Option<&Span>
}
```
And then we would implement that trait for various DataFusion / Arrow tyoes (like `DataType`, etc)?
##########
datafusion/common/src/column.rs:
##########
@@ -18,22 +18,35 @@
//! Column
use arrow_schema::{Field, FieldRef};
+use sqlparser::tokenizer::Span;
use crate::error::_schema_err;
use crate::utils::{parse_identifiers_normalized, quote_identifier};
use crate::{DFSchema, Result, SchemaError, TableReference};
+use derivative::Derivative;
use std::collections::HashSet;
use std::convert::Infallible;
use std::fmt;
use std::str::FromStr;
/// A named reference to a qualified field in a schema.
-#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
+#[derive(Debug, Clone, Derivative)]
+#[derivative(PartialEq, Eq, Hash, PartialOrd, Ord)]
pub struct Column {
/// relation/table reference.
pub relation: Option<TableReference>,
/// field/column name.
pub name: String,
+ #[derivative(
Review Comment:
I also see `WithSpan` has a similar approach (but that is for wrapping things with Spans 🤔 )
##########
datafusion/common/src/with_span.rs:
##########
@@ -0,0 +1,96 @@
+use std::{cmp::Ordering, fmt::{self, Debug, Display}, ops::{Deref, DerefMut}};
+
+use sqlparser::tokenizer::Span;
+
+/// A wrapper type for entitites that somehow refer to a location in the source
Review Comment:
That would likely require implementing specific wrapper types like `DataTypeWithSpan` or something to attach Span information to DataTypes
I also see the need to somehow plumb through the span information into things like coerce-types 🤔
Though maybe now would be a good time to make a proper structure for coercing types, (`struct TypeCoercer` or something) where we could attach the spans 🤔
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "xudong963 (via GitHub)" <gi...@apache.org>.
xudong963 commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2606836221
> We think that this could be quite nicely integrated with [`datafusion-dft`](https://github.com/datafusion-contrib/datafusion-dft) and `datafusion-cli`to provide a richer experience for end users.
cc @matthewmturner Maybe you're interested in the ticket.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2528108553
@alamb here's a few examples of what the new diagnostics that I implemented so far can do:
Note that the rendering here is not part of my PR, but it consumes the `Diagnostic` data that is now produced by datafusion.
We think that this could be quite nicely integrated with [`datafusion-dt`](https://github.com/datafusion-contrib/datafusion-dft) and `datafusion-cli`to provide a richer experience for end users.
#13525
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2602394248
I am looking at this PR in detail now
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#issuecomment-2619984213
@alamb fixed ci issues
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #13664:
URL: https://github.com/apache/datafusion/pull/13664
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1931257186
##########
datafusion/sqllogictest/test_files/create_external_table.slt:
##########
@@ -33,23 +33,23 @@ statement error DataFusion error: SQL error: ParserError\("Missing LOCATION clau
CREATE EXTERNAL TABLE t STORED AS CSV
# Option value is missing
-statement error DataFusion error: SQL error: ParserError\("Expected: string or numeric value, found: \)"\)
+statement error DataFusion error: SQL error: ParserError\("Expected: string or numeric value, found: \) at Line: 1, Column: 66"\)
Review Comment:
I'm thinking should be that parser responsibility more the runtime engine's 🤔
##########
datafusion/sqllogictest/test_files/create_external_table.slt:
##########
@@ -33,23 +33,23 @@ statement error DataFusion error: SQL error: ParserError\("Missing LOCATION clau
CREATE EXTERNAL TABLE t STORED AS CSV
# Option value is missing
-statement error DataFusion error: SQL error: ParserError\("Expected: string or numeric value, found: \)"\)
+statement error DataFusion error: SQL error: ParserError\("Expected: string or numeric value, found: \) at Line: 1, Column: 66"\)
Review Comment:
I'm thinking should be that parser responsibility rather than the runtime engine's 🤔
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1932071772
##########
datafusion/common/src/diagnostic.rs:
##########
@@ -0,0 +1,112 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use sqlparser::tokenizer::Span;
+
+/// Additional contextual information intended for end users, to help them
+/// understand what went wrong by providing human-readable messages, and
+/// locations in the source query that relate to the error in some way.
+///
+/// You can think of a single [`Diagnostic`] as a single "block" of output from
+/// rustc. i.e. either an error or a warning, optionally with some notes and
+/// help messages.
+///
+/// If the diagnostic, a note, or a help message doesn't need to point to a
+/// specific location in the original SQL query (or the [`Span`] is not
+/// available), use [`Span::empty`].
+///
+/// Example:
+///
+/// ```rust
+/// # use datafusion_common::Diagnostic;
+/// # use sqlparser::tokenizer::Span;
+/// # let span = Span::empty();
+/// let diagnostic = Diagnostic::new_error("Something went wrong", span)
+/// .with_help("Have you tried turning it on and off again?", Span::empty());
+/// ```
+#[derive(Debug, Clone)]
+pub struct Diagnostic {
+ pub kind: DiagnosticKind,
+ pub message: String,
+ pub span: Span,
+ pub notes: Vec<DiagnosticNote>,
+ pub helps: Vec<DiagnosticHelp>,
+}
+
+#[derive(Debug, Clone)]
+pub struct DiagnosticNote {
+ pub message: String,
+ pub span: Span,
+}
+
+#[derive(Debug, Clone)]
+pub struct DiagnosticHelp {
Review Comment:
Their difference is only semantic, for now (I expect their structure might diverge too at some point). The former is meant to give more context as to _why the error occurred_ (e.g. a non aggregate expression is not in the `GROUP BY` clause) and the latter is meant to try to give a direct suggestion as to _how to solve the error_ (e.g. add the expression to the `GROUP BY`). `rustc` has the same.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on code in PR #13664:
URL: https://github.com/apache/datafusion/pull/13664#discussion_r1932058436
##########
datafusion/common/src/diagnostic.rs:
##########
@@ -0,0 +1,112 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use sqlparser::tokenizer::Span;
+
+/// Additional contextual information intended for end users, to help them
+/// understand what went wrong by providing human-readable messages, and
+/// locations in the source query that relate to the error in some way.
+///
+/// You can think of a single [`Diagnostic`] as a single "block" of output from
+/// rustc. i.e. either an error or a warning, optionally with some notes and
+/// help messages.
+///
+/// If the diagnostic, a note, or a help message doesn't need to point to a
+/// specific location in the original SQL query (or the [`Span`] is not
+/// available), use [`Span::empty`].
+///
+/// Example:
+///
+/// ```rust
+/// # use datafusion_common::Diagnostic;
+/// # use sqlparser::tokenizer::Span;
+/// # let span = Span::empty();
+/// let diagnostic = Diagnostic::new_error("Something went wrong", span)
+/// .with_help("Have you tried turning it on and off again?", Span::empty());
+/// ```
+#[derive(Debug, Clone)]
+pub struct Diagnostic {
+ pub kind: DiagnosticKind,
+ pub message: String,
+ pub span: Span,
+ pub notes: Vec<DiagnosticNote>,
+ pub helps: Vec<DiagnosticHelp>,
+}
+
+#[derive(Debug, Clone)]
+pub struct DiagnosticNote {
Review Comment:
Added
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add related source code locations to errors [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org