[SPARK-54707] [SQL] Refactor PIVOT resolution main logic to the PivotTransformer#53474
[SPARK-54707] [SQL] Refactor PIVOT resolution main logic to the PivotTransformer#53474mihailoale-db wants to merge 1 commit intoapache:masterfrom
PIVOT resolution main logic to the PivotTransformer#53474Conversation
|
@vladimirg-db @cloud-fan PTAL. |
mihailoale-db
force-pushed
the
pivotrefactor
branch
from
047d2e4 to
49897a1
Compare
mihailoale-db
force-pushed
the
pivotrefactor
branch
2 times, most recently
from
113eded to
b229446
Compare
mihailoale-db
force-pushed
the
pivotrefactor
branch
from
b229446 to
3979d25
Compare
dtenedor
left a comment
dtenedor
left a comment
There was a problem hiding this comment.
Generally LGTM! Just one question about an unused parameter.
| aggregates: Seq[Expression], | ||
| childOutput: Seq[Attribute], | ||
| newAlias: (Expression, Option[String]) => Alias, | ||
| fixedPointImplementation: Boolean): LogicalPlan = { |
There was a problem hiding this comment.
This parameter doesn't appear to be used anywhere? Is this PR accidentally missing a piece of code that should be using it? If not, should we remove it, at least for now until we can think of a way to use it later and then we can put it back?
There was a problem hiding this comment.
It's something that's going to be used once we implement PIVOT in the single-pass resolver. I guess that we can remove it for now and add it later if possible
| aggregates.zip(pivotAggregatesAttributes).map { | ||
| case (aggregate, pivotAtt) => | ||
| newAlias( | ||
| ExtractValue(pivotAtt, Literal(i), conf.resolver), |
There was a problem hiding this comment.
This used to be:
ExtractValue(pivotAtt, Literal(i), resolver)
The original code used a resolver from the Analyzer's scope, while the new code uses conf.resolver from SQLConfHelper. These are usually the same, but if the Analyzer ever overrides resolver, this could cause behavioral differences. This is worth verifying they're guaranteed equivalent. Is it intended?
There was a problem hiding this comment.
There's no enforcement as I can see. For example we use conf.resolver in the Analyzer.scala besides the Analyzer.resolver whereas in the ColumnResolutionHelper we use only the conf.resolver. It looks like a matter of preference so I would keep it as it is to avoid adding another parameter to the already extensive PivotTransformers list
mihailoale-db
force-pushed
the
pivotrefactor
branch
from
3979d25 to
a2c2892
Compare
|
This change seems like a pure refactor at this point. LGTM, merging to master. |
3 participants
What changes were proposed in this pull request?
In this issue I propose to refactor
PIVOTresolution main logic to thePivotTransformerin order to reuse it later to implementPIVOTin the single-pass resolver.Why are the changes needed?
To ease the development of the single-pass resolver.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing tests.
Was this patch authored or co-authored using generative AI tooling?
No.