phillipleblanc commented on code in PR #11186:
URL: https://github.com/apache/datafusion/pull/11186#discussion_r1667601690
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -314,3 +310,78 @@ fn test_table_references_in_plan_to_sql() {
"SELECT \"table\".id, \"table\".\"value\" FROM \"table\"",
);
}
+
+#[test]
+fn test_pretty_roundtrip() -> Result<()> {
+ let schema = Schema::new(vec![
+ Field::new("id", DataType::Utf8, false),
+ Field::new("age", DataType::Utf8, false),
+ ]);
+
+ let df_schema = DFSchema::try_from(schema)?;
+
+ let context = MockContextProvider::default();
+ let sql_to_rel = SqlToRel::new(&context);
+
+ let unparser = Unparser::default();
+
+ let sql_to_pretty_unparse = vec![
+ ("((id < 5) OR (age = 8))", "id < 5 OR age = 8"),
+ ("((id + 5) * (age * 8))", "(id + 5) * age * 8"),
+ ("(3 + (5 * 6) * 3)", "3 + 5 * 6 * 3"),
+ ("((3 * (5 + 6)) * 3)", "3 * (5 + 6) * 3"),
+ ("((3 AND (5 OR 6)) * 3)", "(3 AND (5 OR 6)) * 3"),
+ ("((3 + (5 + 6)) * 3)", "(3 + 5 + 6) * 3"),
+ ("((3 + (5 + 6)) + 3)", "3 + 5 + 6 + 3"),
+ ("3 + 5 + 6 + 3", "3 + 5 + 6 + 3"),
+ ("3 + (5 + (6 + 3))", "3 + 5 + 6 + 3"),
+ ("3 + ((5 + 6) + 3)", "3 + 5 + 6 + 3"),
+ ("(3 + 5) + (6 + 3)", "3 + 5 + 6 + 3"),
+ ("((3 + 5) + (6 + 3))", "3 + 5 + 6 + 3"),
+ (
+ "((id > 10) OR (age BETWEEN 10 AND 20))",
+ "id > 10 OR age BETWEEN 10 AND 20",
+ ),
+ (
+ "((id > 10) * (age BETWEEN 10 AND 20))",
+ "(id > 10) * (age BETWEEN 10 AND 20)",
+ ),
+ ("id - (age - 8)", "id - (age - 8)"),
+ ("((id - age) - 8)", "id - age - 8"),
+ ("(id OR (age - 8))", "id OR age - 8"),
+ ("(id / (age - 8))", "id / (age - 8)"),
+ ("((id / age) * 8)", "id / age * 8"),
+ ("((age + 10) < 20) IS TRUE", "(age + 10 < 20) IS TRUE"),
+ (
+ "(20 > (age + 5)) IS NOT FALSE",
+ "(20 > age + 5) IS NOT FALSE",
+ ),
+ ("(true AND false) IS FALSE", "(true AND false) IS FALSE"),
+ ("true AND (false IS FALSE)", "true AND false IS FALSE"),
+ ];
+
+ for (sql, pretty) in sql_to_pretty_unparse.iter() {
+ let sql_expr = Parser::new(&GenericDialect {})
+ .try_with_sql(sql)?
+ .parse_expr()?;
+ let expr =
+ sql_to_rel.sql_to_expr(sql_expr, &df_schema, &mut
PlannerContext::new())?;
+ let round_trip_sql = unparser.expr_to_sql(&expr)?.to_string();
+ assert_eq!(pretty.to_string(), round_trip_sql);
+
+ // verify that the pretty string parses to the same underlying Expr
+ let pretty_sql_expr = Parser::new(&GenericDialect {})
+ .try_with_sql(pretty)?
+ .parse_expr()?;
+
+ let pretty_expr = sql_to_rel.sql_to_expr(
+ pretty_sql_expr,
+ &df_schema,
+ &mut PlannerContext::new(),
+ )?;
+
+ assert_eq!(expr.to_string(), pretty_expr.to_string());
Review Comment:
I don't think maintaining structural equivalence is important as long as the
correct answer would still be generated if the SQL were run.
But maybe this is exposing two opposing(?) goals of the `unparser` - the
goal for using it to generate SQL that will be run in another SQL query engine.
And another goal for displaying it for human readability/debuggability. I'm in
the first camp - it doesn't matter to me what the resulting SQL looks like as
long as it runs correctly (although having nicer SQL does make debugging
easier).
However, I would expect that we could serve both goals with a single
`unparser` implementation. Any work that is done to improve the SQL for human
readability shouldn't come at the expense of making it invalid SQL for running
in a separate query engine. That is why I thought it would be better to have
just a single `expr_to_sql` function instead of splitting the functionality.
--
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]
