alamb commented on code in PR #12481:
URL: https://github.com/apache/datafusion/pull/12481#discussion_r1761697129
##########
datafusion/expr/src/logical_plan/ddl.rs:
##########
@@ -284,6 +346,15 @@ pub struct CreateCatalogSchema {
pub schema: DFSchemaRef,
}
+impl PartialOrd for CreateCatalogSchema {
+ fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+ match self.schema_name.partial_cmp(&other.schema_name) {
Review Comment:
Why can't we derive `PartialOrd` for this structure?
##########
datafusion/expr/src/logical_plan/ddl.rs:
##########
@@ -232,8 +232,61 @@ impl Hash for CreateExternalTable {
}
}
+impl PartialOrd for CreateExternalTable {
Review Comment:
What requires this manual derivation? Is it some other extension trait would
need to also require `PartialOrd`?
I am thinking it would be really nice to file a ticket tracking what it
would take to use a derived `PartialOrd` to make this code more maintainable
##########
datafusion/expr/src/logical_plan/ddl.rs:
##########
@@ -284,6 +346,15 @@ pub struct CreateCatalogSchema {
pub schema: DFSchemaRef,
}
+impl PartialOrd for CreateCatalogSchema {
+ fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+ match self.schema_name.partial_cmp(&other.schema_name) {
Review Comment:
Perhaps you can add a comment explaining it is due to not comparing schema
ref
Another way you could do this might be with some inner struct like:
```rust
impl PartialOrd for CreateCatalogSchema {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
#[derive(PartialOrd)]
struct ComparableCreateCatalogSchema<'a> {
/// The table schema
pub schema_name: &'astr,
}
let comparable_self = ComparableCreateCatalogSchema { schema_name:
&self.schema_name}
let comparable_other= ComparableCreateCatalogSchema { schema_name:
&other.schema_name}
comparable_self.partial_cmp(comparable_other)
}
}
```
For this particular impl it probably doesn't matter but for
`CreateExternalTable` it might be easier to follow and verify it is correct
##########
datafusion/expr/src/expr.rs:
##########
@@ -1071,18 +1071,6 @@ impl PlannedReplaceSelectItem {
}
}
-/// Fixed seed for the hashing so that Ords are consistent across runs
Review Comment:
❤️
##########
datafusion/expr/src/logical_plan/extension.rs:
##########
@@ -215,7 +223,7 @@ impl Eq for dyn UserDefinedLogicalNode {}
///
[user_defined_plan.rs](https://github.com/apache/datafusion/blob/main/datafusion/core/tests/user_defined/user_defined_plan.rs)
/// file for an example of how to use this extension API.
pub trait UserDefinedLogicalNodeCore:
- fmt::Debug + Eq + Hash + Sized + Send + Sync + 'static
+ fmt::Debug + Eq + PartialOrd + Hash + Sized + Send + Sync + 'static
Review Comment:
this is technically a API change (but I think it is fine) -- I will mark
this PR as an API change
--
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]
