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: [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]
