jchen5 commented on code in PR #45125: URL: https://github.com/apache/spark/pull/45125#discussion_r1498343024
########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala: ########## @@ -328,6 +328,30 @@ abstract class Optimizer(catalogManager: CatalogManager) // Do not optimize DPP subquery, as it was created from optimized plan and we should not // optimize it again, to save optimization time and avoid breaking broadcast/subquery reuse. case d: DynamicPruningSubquery => d + case s@ScalarSubquery(a@Aggregate(group, _, child), _, _, _, _, mayHaveCountBug) + if mayHaveCountBug.nonEmpty && mayHaveCountBug.get => + // This is a subquery with an aggregate that may suffer from a COUNT bug. + // Detailed COUNT bug detection is done at a later stage (e.g. in + // RewriteCorrelatedScalarSubquery). + // Make sure that the output plan always has the same aggregate node + // (i.e., it is not being constant folded). + // Note that this does not limit optimization opportunities for the subquery: after + // decorrelation is done, the subquery's body becomes part of the main plan and all + // optimization rules are applied again. + val groupRefs = group.flatMap(x => x.references) + val projectOverSubqueryBody = Project(groupRefs ++ a.references.toSeq, child) Review Comment: maybe projectOverAggregateChild would be clearer ########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala: ########## @@ -328,6 +328,30 @@ abstract class Optimizer(catalogManager: CatalogManager) // Do not optimize DPP subquery, as it was created from optimized plan and we should not // optimize it again, to save optimization time and avoid breaking broadcast/subquery reuse. case d: DynamicPruningSubquery => d + case s@ScalarSubquery(a@Aggregate(group, _, child), _, _, _, _, mayHaveCountBug) + if mayHaveCountBug.nonEmpty && mayHaveCountBug.get => + // This is a subquery with an aggregate that may suffer from a COUNT bug. + // Detailed COUNT bug detection is done at a later stage (e.g. in + // RewriteCorrelatedScalarSubquery). + // Make sure that the output plan always has the same aggregate node + // (i.e., it is not being constant folded). + // Note that this does not limit optimization opportunities for the subquery: after + // decorrelation is done, the subquery's body becomes part of the main plan and all + // optimization rules are applied again. Review Comment: There could potentially be changes in optimization due to different order in which rules get applied, right? I think that would be quite edge case, but can we confirm whether there are plan changes in benchmarks at least? -- 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: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org