kosiew commented on code in PR #23789:
URL: https://github.com/apache/datafusion/pull/23789#discussion_r3636758208
##########
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:
I like `UnparserAggScope` as a small abstraction because it naturally ties
an aggregate to its derived-input scope decision.
One small thing that stood out is `prepare_sort_expr(scope: Option<&Self>,
...)`. The `None` case doesn't really have an aggregate scope, so it feels like
the abstraction is carrying a responsibility outside its model.
Would it make sense to keep aggregate normalization as an instance method
and move the optional sort orchestration into an `Unparser` helper, or simply
split the aggregate and non-aggregate paths? I think that would keep the
responsibilities a bit more focused. Just a small design suggestion.
--
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]
