alamb commented on PR #15444:
URL: https://github.com/apache/datafusion/pull/15444#issuecomment-2759206571

   > And there are many more functions like this, I can't simply change them as 
they accepts dynamically generated `plan_str` and `plan.schema()`. Changing 
them into `assert_snapshot!` with inline snapshot may change their original 
behaviour.
   > 
   > Just want to know if I need to migrate those non-trivial test cases as 
well, thanks.
   
   In general I suggest we get the basic easy to port tests done and then work 
on the others as a follow on PR
   
   For those changes, I think you would have to restructure the tests
   
   From 
   
   ```rust
           let expected_plan_str = format!(
               "Projection: count(Int64(1)) AS {syntax}\
           \n  Aggregate: groupBy=[[]], aggr=[[count(Int64(1))]]\
           \n    Projection: \
           \n      LeftSemi Join: data.a = data2.a\
           \n        Aggregate: groupBy=[[data.a]], aggr=[[]]\
           \n          TableScan: data projection=[a]\
           \n        TableScan: data2 projection=[a]"
           );
   
           assert_expected_plan(
               &format!("SELECT {syntax} FROM (SELECT data.a FROM data 
INTERSECT SELECT data2.a FROM data2);"),
               &expected_plan_str,
               true
           ).await
   ```
   
   
   Perhaps something more like
   
   
   ```rust
            assert_snapshot!(
           do_conversion(&format!("SELECT {syntax} FROM (SELECT data.a FROM 
data INTERSECT SELECT data2.a FROM data2);),
           @r#"
               "Projection: count(Int64(1)) AS {syntax}\
           \n  Aggregate: groupBy=[[]], aggr=[[count(Int64(1))]]\
           \n    Projection: \
           \n      LeftSemi Join: data.a = data2.a\
           \n        Aggregate: groupBy=[[data.a]], aggr=[[]]\
           \n          TableScan: data projection=[a]\
           \n        TableScan: data2 projection=[a]"
           );
               "#
                   );
           Ok(())
   ```
   
   Where `do_conversion` returns a string and does what assert-epected_plan 
does, something like
   
   ```rust
   async fn do_conversion(
       sql: &str,
       assert_schema: bool,
   ) -> Result<String> {
       let ctx = create_context().await?;
       let df = ctx.sql(sql).await?;
       let plan = df.into_optimized_plan()?;
       let proto = to_substrait_plan(&plan, &ctx.state())?;
       let plan2 = from_substrait_plan(&ctx.state(), &proto).await?;
       let plan2 = ctx.state().optimize(&plan2)?;
   
       if assert_schema {
           assert_eq!(plan.schema(), plan2.schema());
       }
   
       Ok(format!("{plan2}");)
   }
   ```


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

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org

Reply via email to