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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
