andygrove opened a new issue, #26298: URL: https://github.com/apache/datafusion/issues/26298
### Is your feature request related to a problem or challenge? [`Unparser::select_to_sql_recursively` (unparser/plan.rs#L847-L1947)](https://github.com/apache/datafusion/blob/0d79252f61bd8777990b2749c216ffd07079c30d/datafusion/sql/src/unparser/plan.rs#L847-L1947) is a single 896-line function with cognitive complexity 272 and cyclomatic complexity 131, the highest of any function in DataFusion. It has 55 early returns, and `plan.rs` has had 31 commits in the last 12 months. The match over `LogicalPlan` variants is natural, but several arms are very large and inline everything: - `Projection` (about L886-L1217, ~330 lines) mixes unnest-as-table-factor, Snowflake `FLATTEN`, derived-table scoping and Sort/Limit-over-Aggregate absorption. - `Join` (about L1471-L1650) and `SubqueryAlias` (about L1653-L1762). The function threads mutable `query`/`select`/`relation` builders through every arm, so a change for one dialect or plan shape is hard to review in isolation. Unparser bugs regularly come in through this code (e.g. #21746). [`unparse_table_scan_pushdown` (L2540-L2730)](https://github.com/apache/datafusion/blob/0d79252f61bd8777990b2749c216ffd07079c30d/datafusion/sql/src/unparser/plan.rs#L2540-L2730) has the same problem on a smaller scale. It handles three plan shapes in one function, builds the alias rewriter twice (L2549 and L2698), and buries a stacked-projection special case inside an `if` with a 12-line comment. ### Describe the solution you'd like 1. As a purely mechanical first step, keep `select_to_sql_recursively` as a dispatcher and move each arm into a named method with the same signature (`select_projection`, `select_aggregate`, `select_join`, `select_subquery_alias`, `select_window`, `select_unnest`, ...). Behaviour is unchanged, and the existing unparser tests verify it. 2. Split `select_projection` further into unnest / lateral-flatten handling, derived-input scoping, and Sort/Limit absorption helpers. 3. Introduce a small context struct for the `(query, select, relation)` triple to shorten signatures. 4. Split `unparse_table_scan_pushdown` into one helper per plan shape, and build the `TableAliasRewriter` once. ### Describe alternatives you've considered Leave it as is. The function keeps growing as dialect features are added, which raises the cost of every unparser fix. ### Additional context Line references are pinned to commit 0d79252f61bd. -- 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]
