[GitHub] [arrow-datafusion] mslapek opened a new issue, #5400: std::cmp::Eq reflexivity assumptions are violated

["mslapek (via GitHub)" <gi...@apache.org>]

mslapek opened a new issue, #5400:
URL: https://github.com/apache/arrow-datafusion/issues/5400

   **Describe the bug**
   A clear and concise description of what the bug is.
   
   Type [datafusion::logical_expr::Subquery](https://docs.rs/datafusion-expr/18.0.0/datafusion_expr/struct.Subquery.html) is declared to implement [std::cmp::Eq](https://doc.rust-lang.org/std/cmp/trait.Eq.html).
   
   However, it **violates** reflexivity property.
   
   Notice, that the type **respects** [std::cmp::PartialEq](https://doc.rust-lang.org/std/cmp/trait.PartialEq.html), which is nevertheless a **distinct** trait!
   
   **To Reproduce**
   Steps to reproduce the behavior:
   
   ```rust
   use datafusion::logical_expr::Subquery;
   use datafusion::prelude::*;
   
   #[tokio::main]
   async fn main() -> datafusion::error::Result<()> {
       let ctx = SessionContext::new();
       let df = ctx
           .read_csv(
               "datafusion/core/tests/tpch-csv/orders.csv",
               CsvReadOptions::default(),
           )
           .await?;
       let plan = df.logical_plan().clone();
   
       // There starts the show...
       let a = Subquery::new(plan);
       check_eq_reflexive(&a);    // Boom! PANICS!
   
       Ok(())
   }
   
   fn check_eq_reflexive<T: Eq>(a: &T) {
       assert!(a.eq(a), "not reflexive!");
   }
   ```
   
   **Expected behavior**
   A clear and concise description of what you expected to happen.
   
   Type `datafusion::logical_expr::Subquery` should:
   
   a) do NOT implement `std::cmp::Eq` 
   or
   b) have reflexive `eq(...)`
   
   **Additional context**
   Add any other context about the problem here.
   
   This is a blocker of #3892, which would benefit from `Eq` and `Hash`. Already in the optimizer loop we have tricks, like dumping to strings, to compare `LogicalPlan`s.
   
   ⭐️ **Subjective "How fix it?"** ⭐️
   
   The problem is that [datafusion_expr::logical_plan::LogicalPlan](https://docs.rs/datafusion-expr/18.0.0/datafusion_expr/logical_plan/enum.LogicalPlan.html#) has inside `dyn` objects like functions or `TableProvider`s.
   
   `LogicalPlan` should contain only strings referring to functions/`TableProviders` from an **external dictionary** (some context object). This would give a healthier architecture.
   
   I'm eager to help with the rearchitecting. 👋


-- 
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@arrow.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] alamb closed issue #5400: std::cmp::Eq reflexivity assumptions are violated

["alamb (via GitHub)" <gi...@apache.org>]

alamb closed issue #5400: std::cmp::Eq reflexivity assumptions are violated
URL: https://github.com/apache/arrow-datafusion/issues/5400


-- 
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@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org