alamb commented on code in PR #11186:
URL: https://github.com/apache/datafusion/pull/11186#discussion_r1667742546
##########
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:
> 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 think this is an excellent summary of the issue and given these two
different goals there would be two different ideal outcomes.
Perhaps as @MohamedAbdeen21 suggests we can add some sort of API to allow
the user to decide. We could potentially use `Unparser` for this (though I
don't think `Unparser` is part of the public API yet).
https://github.com/apache/datafusion/blob/45599ce310aa6270813091a5c3288abcd7541f59/datafusion/sql/src/unparser/mod.rs#L30-L32
So something like
```rust
// default unparser generates fully parenthesized sql:
let unparser = Unparser::default();
// can also create an unparser that generates pretty sql
let unparser = Unparser::default()
// enable pretty-printing of the sql easier suited to human consumption
.with_pretty(true);
--
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]
