goldmedal commented on code in PR #21593:
URL: https://github.com/apache/datafusion/pull/21593#discussion_r3081093390
##########
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:
> Specifically in the test cases I've added I've built unnest from the
result of a UDF (consider `select UNNEST(ExtractArrayFromCol(col)) from table`
and other cases where the DF optimizer is putting Limit, Sort, multiple layers
of SubqueryAlias and other nodes inside the DF logical plan. Handing all of
these felt like something to big and invasive to add to the dialect traints.
>
Agreed. The plan-tree traversal logic to find the `UNNEST` belongs in the
core unparser — that's generic behavior, not dialect-specific, and it would be
too invasive to push into the trait.
> But as I said, the real motivation was to keep the dialect as slim as
possible (like today)
>
I'm not sure if it's the unparser dialect's goal now 🤔 (maybe you have
discussed this with other people?). In `Dialect`, there are many methods that
produce a result (e.g. `scalar_function_to_sql_overrides`,
`col_alias_overrides`, `timestamp_with_tz_to_string`,.. ) instead of a bool
flag.
My suggestion is more about separating the two concerns:
- Plan traversal stays in the unparser, as you've done
- SQL rendering (the specific `LATERAL FLATTEN(INPUT => expr, OUTER =>
bool)` syntax) → delegated to the dialect via a trait method
The core unparser would still do all the heavy lifting — the dialect only
receives the prepared context and decides how to render the table factor. This
keeps the dialect slim (just the rendering logic) while avoiding
Snowflake-specific AST construction in `plan.rs`.
WDYT?
--
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]
