goldmedal commented on code in PR #11527:
URL: https://github.com/apache/datafusion/pull/11527#discussion_r1682891322
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -240,6 +240,63 @@ fn roundtrip_statement_with_dialect() -> Result<()> {
parser_dialect: Box::new(GenericDialect {}),
unparser_dialect: Box::new(UnparserDefaultDialect {}),
},
+ TestStatementWithDialect {
+ sql: "SELECT j1_string from j1 order by j1_id",
Review Comment:
I tried adding another test case using join, and it works well, too. 👍
```rust
TestStatementWithDialect {
sql: "SELECT j1_string from j1 join j2 on j1.j1_id = j2.j2_id
order by j1_id",
expected: r#"SELECT j1.j1_string FROM j1 JOIN j2 ON (j1.j1_id =
j2.j2_id) ORDER BY j1.j1_id ASC NULLS LAST"#,
parser_dialect: Box::new(GenericDialect {}),
unparser_dialect: Box::new(UnparserDefaultDialect {}),
},
```
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -240,6 +240,63 @@ fn roundtrip_statement_with_dialect() -> Result<()> {
parser_dialect: Box::new(GenericDialect {}),
unparser_dialect: Box::new(UnparserDefaultDialect {}),
},
+ TestStatementWithDialect {
+ sql: "SELECT j1_string from j1 order by j1_id",
+ expected: r#"SELECT j1.j1_string FROM j1 ORDER BY j1.j1_id ASC
NULLS LAST"#,
+ parser_dialect: Box::new(GenericDialect {}),
+ unparser_dialect: Box::new(UnparserDefaultDialect {}),
+ },
+ // Test query with derived tables that put distinct,sort,limit on the
wrong level
Review Comment:
It also works for grouping. 👍
```rust
TestStatementWithDialect {
sql: "
SELECT
j1_string,
j2_string
FROM
(
SELECT
j1_id,
j1_string,
j2_string
from
j1
INNER join j2 ON j1.j1_id = j2.j2_id
group by
j1_id,
j1_string,
j2_string
order by
j1.j1_id desc
limit
10
) abc
ORDER BY
abc.j2_string",
expected: r#"SELECT abc.j1_string, abc.j2_string FROM (SELECT
j1.j1_id, j1.j1_string, j2.j2_string FROM j1 JOIN j2 ON (j1.j1_id = j2.j2_id)
GROUP BY j1.j1_id, j1.j1_string, j2.j2_string ORDER BY j1.j1_id DESC NULLS
FIRST LIMIT 10) AS abc ORDER BY abc.j2_string ASC NULLS LAST"#,
parser_dialect: Box::new(GenericDialect {}),
unparser_dialect: Box::new(UnparserDefaultDialect {}),
},
```
##########
datafusion/sql/src/unparser/rewrite.rs:
##########
@@ -99,3 +99,61 @@ fn rewrite_sort_expr_for_union(exprs: Vec<Expr>) ->
Result<Vec<Expr>> {
Ok(sort_exprs)
}
+
+// Rewrite logic plan for query that order by columns are not in projections
+// Plan before rewrite:
+//
+// Projection: j1.j1_string, j2.j2_string
+// Sort: j1.j1_id DESC NULLS FIRST, j2.j2_id DESC NULLS FIRST
+// Projection: j1.j1_string, j2.j2_string, j1.j1_id, j2.j2_id
+// Inner Join: Filter: j1.j1_id = j2.j2_id
+// TableScan: j1
+// TableScan: j2
+//
+// Plan after rewrite
+//
+// Sort: j1.j1_id DESC NULLS FIRST, j2.j2_id DESC NULLS FIRST
+// Projection: j1.j1_string, j2.j2_string
+// Inner Join: Filter: j1.j1_id = j2.j2_id
+// TableScan: j1
+// TableScan: j2
+//
+// This prevents the original plan generate query with derived table but
missing alias.
+pub(super) fn rewrite_plan_for_sort_on_non_projected_fields(
+ p: &Projection,
+) -> Option<LogicalPlan> {
+ let mut collects = vec![];
+
+ for expr in p.expr.clone() {
+ collects.push(expr.clone());
+ }
Review Comment:
```suggestion
p.expr.iter().cloned().for_each(|expr| {
collects.push(expr);
});
```
I think this way could save one time to clone all expressions.
--
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]
