findepi commented on code in PR #13660:
URL: https://github.com/apache/datafusion/pull/13660#discussion_r1877905372
##########
datafusion/sql/src/unparser/plan.rs:
##########
@@ -678,7 +692,11 @@ impl Unparser<'_> {
)
}
LogicalPlan::EmptyRelation(_) => {
- relation.empty();
+ // An EmptyRelation could be behind a UNNEST node. If the
dialect supports UNNEST as a table factor,
Review Comment:
a UNNEST -> an UNNEST
##########
datafusion/sql/src/unparser/dialect.rs:
##########
@@ -157,6 +157,15 @@ pub trait Dialect: Send + Sync {
fn full_qualified_col(&self) -> bool {
false
}
+
+ /// Allow to unparse the unnest plan as [ast::TableFactor::UNNEST].
+ ///
+ /// Some dialects like BigQuery require UNNEST to be used in the FROM
clause but
+ /// the LogicalPlan planner always put UNNEST in the SELECT clause. This
flag allows
Review Comment:
put -> puts
well, UNNEST is a lateral relational operator. It doesn't belong to the
select clause.
It's probably yet another witness of the problem that the LP is half way
between AST and a good IR (which we don't have yet)
##########
datafusion/sql/tests/cases/plan_to_sql.rs:
##########
@@ -525,6 +525,60 @@ fn roundtrip_statement_with_dialect() -> Result<()> {
parser_dialect: Box::new(GenericDialect {}),
unparser_dialect: Box::new(SqliteDialect {}),
},
+ TestStatementWithDialect {
+ sql: "SELECT * FROM UNNEST([1,2,3])",
+ expected: r#"SELECT * FROM UNNEST([1, 2, 3])"#,
+ parser_dialect: Box::new(GenericDialect {}),
+ unparser_dialect:
Box::new(CustomDialectBuilder::default().with_unnest_as_table_factor(true).build()),
Review Comment:
the added tests all seem to use `with_unnest_as_table_factor(true)`
if the case of `with_unnest_as_table_factor(false)` already covered
elsewhere?
##########
datafusion/sql/src/unparser/plan.rs:
##########
@@ -708,6 +726,38 @@ impl Unparser<'_> {
}
}
+ /// Try to find the placeholder column name generated by
`RecursiveUnnestRewriter`
+ /// Only match the pattern
`Expr::Alias(Expr::Column("unnest_placeholder(...)"))`
+ fn is_unnest_placeholder(expr: &Expr) -> bool {
+ if let Expr::Alias(Alias { expr, .. }) = expr {
+ if let Expr::Column(Column { name, .. }) = expr.as_ref() {
+ return name.starts_with("unnest_placeholder");
Review Comment:
is this a well known constant name? should it be defined somewhere?
also, internally create names should be something a user is very unlikely to
have in their data
perhaps starting with `$` or at least `_`
--
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]
