naman-modi commented on code in PR #23789:
URL: https://github.com/apache/datafusion/pull/23789#discussion_r3637964670
##########
datafusion/sql/src/unparser/plan.rs:
##########
@@ -101,6 +101,75 @@ pub fn plan_to_sql(plan: &LogicalPlan) ->
Result<ast::Statement> {
unparser.plan_to_sql(plan)
}
+/// Aggregate-expression scope for one rendered SELECT block.
+///
+/// When an aggregate's input is itself emitted as a derived subquery (a
+/// projection sits between the aggregate and its relation), the input columns
+/// are only reachable by that derived table's output names. Base-table
+/// qualifiers like `t.col` name a relation that is out of scope above the
+/// boundary, so emitting them produces SQL a strict engine rejects.
+///
+/// Every clause that renders an aggregate expression (SELECT / GROUP BY /
+/// HAVING / QUALIFY / ORDER BY) has to apply the same rule. Detect the
+/// boundary once here and reuse it, so the clauses can't drift apart (which is
+/// how earlier fixes left some clauses correct and others not).
+struct UnparserAggScope<'a> {
+ agg: &'a Aggregate,
+ /// `agg.input` renders as a derived projection, so out-of-scope qualifiers
+ /// must be stripped from expressions in this scope.
+ input_is_derived_projection: bool,
+}
+
+impl<'a> UnparserAggScope<'a> {
+ fn new(agg: &'a Aggregate) -> Self {
+ Self {
+ agg,
+ input_is_derived_projection:
Unparser::contains_projection_before_relation(
+ agg.input.as_ref(),
+ ),
+ }
+ }
+
+ /// Prepare a projected column or predicate that still references the
+ /// aggregate by its output columns: unproject it back onto the aggregate
+ /// (and `windows`) expressions, then normalize it for this scope.
+ fn prepare(&self, expr: Expr, windows: Option<&[&Window]>) -> Result<Expr>
{
+ self.normalize(unproject_agg_exprs(expr, self.agg, windows)?)
+ }
+
+ /// Normalize an expression that is already in aggregate form (group / aggr
+ /// exprs, or an unprojected sort expr): strip the qualifiers that fall out
+ /// of scope once the input is a derived projection. No-op otherwise.
+ fn normalize(&self, expr: Expr) -> Result<Expr> {
+ if self.input_is_derived_projection {
+ Unparser::strip_column_qualifiers_for_schema(
+ expr,
+ self.agg.input.schema().as_ref(),
+ )
+ } else {
+ Ok(expr)
+ }
+ }
+
+ /// Unproject a sort expression onto its aggregate, then normalize it so
+ /// ORDER BY resolves against the same scope as every other clause in the
+ /// block. Associated fn taking `Option<&Self>` because a sort can appear
+ /// with no aggregate below it, in which case only unprojection applies.
+ fn prepare_sort_expr(
Review Comment:
Makes sense, I have updated the `prepare_sort_expr` to now be an instance
method of `UnparserAggScope`, and created a
`Unparser::unproject_sort_expr_in_scope` to handle both the arms, hence
unifying the call sites.
##########
datafusion/core/tests/sql/unparser.rs:
##########
@@ -612,6 +664,87 @@ async fn
optimized_duckdb_unparse_qualify_unqualifies_agg_input() -> Result<()>
Ok(())
}
+#[tokio::test]
+async fn optimized_duckdb_unparse_window_over_agg_unqualifies_input() ->
Result<()> {
+ let ctx = issue_23317_context()?;
+ assert!(ctx.remove_optimizer_rule(SingleDistinctToGroupBy::new().name()));
+
+ let plan = ctx
+ .sql(ISSUE_23668_WINDOW_QUERY)
+ .await?
+ .into_optimized_plan()?;
+ let dialect = DuckDBDialect::new();
+ let unparser = Unparser::new(&dialect);
+ let sql = unparser.plan_to_sql(&plan)?.to_string();
+
+ assert_issue_23317_unparsed_sql_plans(&ctx, &sql).await?;
+
+ assert!(
+ sql.contains(r#"OVER (ORDER BY count(DISTINCT "customer_id")"#),
+ "window ORDER BY aggregate should resolve against the derived
projection output: {sql}",
+ );
+ assert!(
+ !sql.contains(r#"count(DISTINCT "cs"."customer_id")"#),
+ "window OVER clause must not reference out-of-scope alias cs: {sql}",
+ );
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn optimized_duckdb_unparse_order_by_unqualifies_agg_input() ->
Result<()> {
+ let ctx = issue_23317_context()?;
+ assert!(ctx.remove_optimizer_rule(SingleDistinctToGroupBy::new().name()));
+
+ let plan = ctx
+ .sql(ISSUE_23668_ORDER_BY_QUERY)
+ .await?
+ .into_optimized_plan()?;
+ let dialect = DuckDBDialect::new();
+ let unparser = Unparser::new(&dialect);
+ let sql = unparser.plan_to_sql(&plan)?.to_string();
+
+ assert_issue_23317_unparsed_sql_plans(&ctx, &sql).await?;
+
+ assert!(
+ sql.contains(r#"ORDER BY round(sum("total_revenue"), 2)"#),
+ "ORDER BY aggregate should resolve against the derived projection
output: {sql}",
+ );
+ assert!(
+ !sql.contains(r#"sum("cs"."total_revenue")"#),
+ "ORDER BY must not reference out-of-scope alias cs: {sql}",
+ );
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn optimized_duckdb_unparse_top_level_sort_over_agg_stays_in_scope() ->
Result<()> {
Review Comment:
You're right, this holds. I have updated the method name & comment to be
more specific - that it only covers the top-level Sort routing.
--
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]
