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

[SPARK-43851][SQL] Support LCA in grouping expressions #41804

Closed
wants to merge 3 commits into from

Conversation

Hisoka-X
Copy link
Member

What changes were proposed in this pull request?

This PR bring support lateral column alias reference in grouping expressions.

Why are the changes needed?

add new feature for LCA

Does this PR introduce any user-facing change?

No

How was this patch tested?

exist test

@github-actions github-actions bot added the SQL label Jun 30, 2023
@Hisoka-X
Copy link
Member Author

cc @wangyum @anchovYu @cloud-fan

Copy link
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

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

LGTM except of a minor comment.

Comment on lines +108 to +109
g.transformWithPruning(_.containsAnyPattern(UNRESOLVED_ATTRIBUTE,
LATERAL_COLUMN_ALIAS_REFERENCE)) {
Copy link
Member

@MaxGekk MaxGekk Jun 30, 2023

Choose a reason for hiding this comment

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

The patterns can guarantee that we have only UnresolvedAttribute and LateralColumnAliasReference, so you can write more generic code:

          case u: NamedExpression =>
            selectList.find(ne => conf.resolver(ne.name, u.name)).getOrElse(u)

Copy link
Member Author

@Hisoka-X Hisoka-X Jun 30, 2023

Choose a reason for hiding this comment

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

I'm not sure if this way will match cases that shouldn't be matched, because _.containsAnyPattern(UNRESOLVED_ATTRIBUTE, LATERAL_COLUMN_ALIAS_REFERENCE also return true when one plan children node contains UnresolvedAttribute or LateralColumnAliasReference. Then may match other NamedExpression

@github-actions github-actions bot added the DOCS label Jun 30, 2023
@MaxGekk
Copy link
Member

MaxGekk commented Jul 1, 2023

+1, LGTM. Merging to master.
Thank you, @Hisoka-X and @wangyum for review.

@MaxGekk MaxGekk closed this in 9353d67 Jul 1, 2023
@Hisoka-X
Copy link
Member Author

Hisoka-X commented Jul 1, 2023

Thanks @MaxGekk and @wangyum

@Hisoka-X Hisoka-X deleted the SPARK-43851_LCA_in_group branch July 1, 2023 07:57
selectList.find(ne => conf.resolver(ne.name, u.name)).getOrElse(u)
g.transformWithPruning(_.containsAnyPattern(UNRESOLVED_ATTRIBUTE,
LATERAL_COLUMN_ALIAS_REFERENCE)) {
case u @ (_: UnresolvedAttribute | _: LateralColumnAliasReference) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we update the lca resolution rule to not resolve columns in grouping expressions to LateralColumnAliasReference? The produced plan is very weird. also cc @anchovYu

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @cloud-fan , I'll try to implement it in followup PR, please help to see if it meets the requirements when it ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants