alamb commented on code in PR #18866:
URL: https://github.com/apache/datafusion/pull/18866#discussion_r2554026795
##########
datafusion/substrait/tests/cases/roundtrip_logical_plan.rs:
##########
@@ -1266,6 +1268,49 @@ async fn roundtrip_values_no_columns() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn roundtrip_values_with_scalar_function() -> Result<()> {
+ let ctx = create_context().await?;
+ let expr = map(vec![lit("a")], vec![lit(1)]);
+ let plan = LogicalPlanBuilder::values(vec![vec![expr]])?.build()?;
+ let expected = ctx.state().optimize(&plan)?;
+
+ let actual = substrait_roundtrip(&plan, &ctx).await?;
+
+ let normalize = |plan: &LogicalPlan| match plan {
Review Comment:
can you add some comments about why `normalize` is needed? It looks to me
like it just clones the input expressions and I had to look carefully to find
the one line that removes aliases
##########
datafusion/substrait/tests/cases/roundtrip_logical_plan.rs:
##########
@@ -1266,6 +1268,49 @@ async fn roundtrip_values_no_columns() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn roundtrip_values_with_scalar_function() -> Result<()> {
+ let ctx = create_context().await?;
+ let expr = map(vec![lit("a")], vec![lit(1)]);
+ let plan = LogicalPlanBuilder::values(vec![vec![expr]])?.build()?;
+ let expected = ctx.state().optimize(&plan)?;
+
+ let actual = substrait_roundtrip(&plan, &ctx).await?;
+
+ let normalize = |plan: &LogicalPlan| match plan {
+ LogicalPlan::Values(values_plan) => {
+ let rows = values_plan
+ .values
+ .iter()
+ .map(|row| {
+ row.iter()
+ .map(|expr| match expr {
Review Comment:
This looks like it is stripping the aliases -- maybe you could use
https://docs.rs/datafusion/latest/datafusion/prelude/enum.Expr.html#method.unalias
instead?
You could probably also use the tree node APIs, for example
https://docs.rs/datafusion-expr/latest/datafusion_expr/logical_plan/enum.LogicalPlan.html#method.map_expressions
to rewrite all expressions in the node more succinctly
##########
datafusion/sqllogictest/src/engines/datafusion_substrait_roundtrip_engine/runner.rs:
##########
@@ -152,6 +132,7 @@ async fn run_query_substrait_round_trip(
| LogicalPlan::Explain(_)
| LogicalPlan::Dml(_)
| LogicalPlan::Copy(_)
+ | LogicalPlan::DescribeTable(_)
Review Comment:
Can you please add a test for converting DESCRIBE to substrait?
##########
datafusion/substrait/tests/cases/roundtrip_logical_plan.rs:
##########
@@ -1266,6 +1268,49 @@ async fn roundtrip_values_no_columns() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn roundtrip_values_with_scalar_function() -> Result<()> {
+ let ctx = create_context().await?;
+ let expr = map(vec![lit("a")], vec![lit(1)]);
Review Comment:
It might help to call out here that `map` is a function call.
--
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]
