goldmedal commented on code in PR #21593:
URL: https://github.com/apache/datafusion/pull/21593#discussion_r3106581001
##########
datafusion/sql/src/unparser/ast.rs:
##########
@@ -162,9 +162,28 @@ pub struct SelectBuilder {
qualify: Option<ast::Expr>,
value_table_mode: Option<ast::ValueTableMode>,
flavor: Option<SelectFlavor>,
+ /// Counter for generating unique LATERAL FLATTEN aliases within this
SELECT.
+ flatten_alias_counter: usize,
}
impl SelectBuilder {
+ /// Generate a unique alias for a LATERAL FLATTEN relation
+ /// (`_unnest_1`, `_unnest_2`, …). Each call returns a fresh name.
+ pub fn next_flatten_alias(&mut self) -> String {
+ self.flatten_alias_counter += 1;
+ format!("_unnest_{}", self.flatten_alias_counter)
Review Comment:
Could you add a test for the multiple unnest case?
##########
datafusion/sql/src/unparser/ast.rs:
##########
@@ -162,9 +162,28 @@ pub struct SelectBuilder {
qualify: Option<ast::Expr>,
value_table_mode: Option<ast::ValueTableMode>,
flavor: Option<SelectFlavor>,
+ /// Counter for generating unique LATERAL FLATTEN aliases within this
SELECT.
+ flatten_alias_counter: usize,
}
impl SelectBuilder {
+ /// Generate a unique alias for a LATERAL FLATTEN relation
+ /// (`_unnest_1`, `_unnest_2`, …). Each call returns a fresh name.
+ pub fn next_flatten_alias(&mut self) -> String {
+ self.flatten_alias_counter += 1;
+ format!("_unnest_{}", self.flatten_alias_counter)
Review Comment:
It's better to use a constant to present the name. Something like
```
pub const UNNEST_PREFIX: &str = "__unnest_";
```
##########
datafusion/sql/src/unparser/dialect.rs:
##########
@@ -211,6 +206,15 @@ pub trait Dialect: Send + Sync {
false
}
+ /// Unparse the unnest plan as `LATERAL FLATTEN(INPUT => expr, ...)`.
+ ///
+ /// Snowflake uses FLATTEN as a table function instead of the SQL-standard
UNNEST.
+ /// When this returns `true`, the unparser emits
+ /// `LATERAL FLATTEN(INPUT => <col>, OUTER => <bool>)` in the FROM clause.
+ fn unnest_as_lateral_flatten(&self) -> bool {
+ false
+ }
Review Comment:
> A long term option to really think if there's a more generic way to do
dialect specific transforms, and bring _both_ the structural analysis (plan
traversal, unpeeling, etc) and AST builder into the dialects, but I feel this
is a pretty invasive change with a large blast radius that I don't want to bind
into this PR. Consider, for example, how BQ handles `UNNEST` is also not
entirely similar to how Postgres (for example) handles `UNNEST`s
>
Indeed, providing a generic way is a big change for the unparser and the
dialect. I still think #20648 is on the right track for the generic solution,
but I think we can have another follow-up PR to enhance it. Let's focus on the
Snowflake flatten now.
--
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]
