[SPARK-20392][SQL][followup] should not add extra AnalysisBarrier #20094
Conversation
| if aggregate.resolved => | ||
| case Filter(cond, AnalysisBarrier(agg: Aggregate)) => | ||
| apply(Filter(cond, agg)).mapChildren(AnalysisBarrier) | ||
| case f @ Filter(cond, agg @ Aggregate(grouping, originalAggExprs, child)) if agg.resolved => |
There was a problem hiding this comment.
just make the names shorter
| val resolved = resolveExpression(expr, plan) | ||
| if (resolved.resolved) { | ||
| resolved | ||
| private def resolveExprsAndAddMissingAttrs( |
There was a problem hiding this comment.
I refactored the code to resolve expressions and add missing attributes in one shot, so that we have a central place to deal with analysis barrier and to decide which operator is supported and which is not.
|
Test build #85439 has finished for PR 20094 at commit
|
| @@ -665,14 +664,18 @@ class Analyzer( | |||
| * Generate a new logical plan for the right child with different expression IDs | |||
| * for all conflicting attributes. | |||
| */ | |||
| private def dedupRight (left: LogicalPlan, originalRight: LogicalPlan): LogicalPlan = { | |||
| // Remove analysis barrier if any. | |||
| val right = EliminateBarriers(originalRight) | |||
There was a problem hiding this comment.
If right plan is wrapped (e.g., we join two datasets) in an analysis barrier, the later right.collect doesn't work.
There was a problem hiding this comment.
oh, I see, you have recursively dedupRight on it.
| @@ -723,7 +726,7 @@ class Analyzer( | |||
| s.withNewPlan(dedupOuterReferencesInSubquery(s.plan, attributeRewrites)) | |||
| } | |||
| } | |||
| AnalysisBarrier(newRight) | |||
| newRight | |||
There was a problem hiding this comment.
newRight is introduced before to be wrapped in AnalysisBarrier. We can get rid of this redundant variable now.
| case d: Distinct => | ||
| (exprs.map(resolveExpression(_, d)), d) | ||
|
|
||
| case u: UnaryNode => |
There was a problem hiding this comment.
Shouldn't we stop at SubqueryAlias as before?
There was a problem hiding this comment.
ah good catch! I missed that because the logic was in resolveExpressionRecursively instead of addMissingAttr.
It indicates that it's more clear to merge these 2 methods :)
|
LGTM with two minor comments. |
|
Test build #85452 has finished for PR 20094 at commit
|
|
Test build #85450 has finished for PR 20094 at commit
|
|
Test build #85453 has finished for PR 20094 at commit
|
|
Thanks! Merged to master. |
What changes were proposed in this pull request?
I found this problem while auditing the analyzer code. It's dangerous to introduce extra
AnalysisBarrerduring analysis, as the plan inside it will bypass all analysis afterward, which may not be expected. We should only preserveAnalysisBarrerbut not introduce new ones.How was this patch tested?
existing tests