Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix canonicalization regression with Spark 3.2 #3403

Merged
merged 1 commit into from
Sep 9, 2021

Conversation

andygrove
Copy link
Contributor

Signed-off-by: Andy Grove andygrove@nvidia.com

Partially address #3400

This PR extends the exprId normalization logic to also apply to aggregate expressions where mode=Partial and fixes two test failures (HashAggregatesSuite and ExpandExecSuite) when running against Spark 3.2.

See #3400 for more detail on why the behavior is different with Spark 3.2

…gression when running with Spark 3.2

Signed-off-by: Andy Grove <andygrove@nvidia.com>
Copy link
Collaborator

@revans2 revans2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct to me.

@andygrove
Copy link
Contributor Author

build

case a: AttributeReference => a.withExprId(ExprId(0))
}
case Partial | Complete => aggregateFunction
case Complete => aggregateFunction
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if we normalize all? (i.e. no match).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did wonder about that as well but I really don't know. Prior to this PR, our code matches Spark, so there seems to be a reason not to apply this in all cases.

@sameerz sameerz added this to the Aug 30 - Sept 10 milestone Sep 9, 2021
@andygrove andygrove self-assigned this Sep 9, 2021
@andygrove andygrove merged commit de2c1a3 into NVIDIA:branch-21.10 Sep 9, 2021
@andygrove andygrove deleted the fix-canonical-spark32 branch September 9, 2021 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants