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.

Sign up for GitHub

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

Jump to bottom

Add support for WindowGroupLimitExec #906

Merged
merged 6 commits into from
Apr 8, 2024
Merged

Add support for WindowGroupLimitExec #906

merged 6 commits into from
Apr 8, 2024

Conversation

parthosa
Copy link
Collaborator

@parthosa parthosa commented Apr 4, 2024 •
edited
Loading

Fixes #883.

Changes:

  • Add parser for WindowGroupLimitExec
  • Update operator score files and override json file.
  • Add unit test for the WindowGroupLimitExec operator (only for spark 3.5.0+).

@parthosa
Add support for WindowGroupLimitExec
732b7c9 
Signed-off-by: Partho Sarthi <psarthi@nvidia.com>
@parthosa parthosa added bug Something isn't working core_tools Scope the core module (scala) labels Apr 4, 2024
@parthosa parthosa self-assigned this Apr 4, 2024
@parthosa
Update comment
04a0481 
Signed-off-by: Partho Sarthi <psarthi@nvidia.com>
@parthosa
Make unit test columns generic
5fbf4d4 
Signed-off-by: Partho Sarthi <psarthi@nvidia.com>
cindyyuanjiang
cindyyuanjiang previously approved these changes Apr 5, 2024
View reviewed changes
Copy link
Collaborator

@cindyyuanjiang cindyyuanjiang left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @parthosa!

amahussein
amahussein requested changes Apr 5, 2024
View reviewed changes
Copy link
Collaborator

@amahussein amahussein left a comment

Choose a reason for hiding this comment

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

Thanks @parthosa
LGTM in general with minor changes to address,

core/src/main/scala/com/nvidia/spark/rapids/tool/planparser/SQLPlanParser.scala Outdated Show resolved Hide resolved
core/src/main/scala/com/nvidia/spark/rapids/tool/planparser/WindowGroupLimitParser.scala Outdated
Comment on lines 34 to 35
val expression = SQLPlanParser.parseWindowGroupLimitExpression(exprString)
val notSupportedExprs = checker.getNotSupportedExprs(expression.toSeq)
Copy link
Collaborator

Choose a reason for hiding this comment

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

somehwere here the code should check for the list of accepted expressions.
This is a common case that we may need to do for other Execs. For example, @nartal1 and I were just discussing the same thing for #715
So, it will be good if the implementation we come up with here can easily be used to fix #715

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added an validateRankingExpr() for this current use case. The refactoring could be taken up in a separate PR.

core/src/main/scala/org/apache/spark/sql/rapids/tool/ToolUtils.scala Show resolved Hide resolved
core/src/main/scala/com/nvidia/spark/rapids/tool/planparser/WindowGroupLimitParser.scala Outdated Show resolved Hide resolved
@parthosa
Address review comments
1658332 
Signed-off-by: Partho Sarthi <psarthi@nvidia.com>
amahussein
amahussein reviewed Apr 5, 2024
View reviewed changes
Copy link
Collaborator

@amahussein amahussein left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I am ok With the changes.

@nartal1 remember we discussed that it would be useful to explain the reason an exec won't be supported when there is a conflict with the expressions? #715
If you are ok with this PR as it is, then please approve it.

@amahussein amahussein self-requested a review April 5, 2024 21:13
@nartal1
Copy link
Collaborator

nartal1 commented Apr 5, 2024

I am ok With the changes.

@nartal1 remember we discussed that it would be useful to explain the reason an exec won't be supported when there is a conflict with the expressions? #715 If you are ok with this PR as it is, then please approve it.

Yes @amahussein ! I am going to include those changes in #715. The exprs not supported here will be included in 715.

nartal1
nartal1 approved these changes Apr 5, 2024
View reviewed changes
Copy link
Collaborator

@nartal1 nartal1 left a comment

Choose a reason for hiding this comment

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

Thanks @parthosa ! I will include the refactoring in #715.

cindyyuanjiang
cindyyuanjiang approved these changes Apr 5, 2024
View reviewed changes
Copy link
Collaborator

@cindyyuanjiang cindyyuanjiang left a comment

Choose a reason for hiding this comment

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

Thanks @parthosa! LGTM

@amahussein amahussein merged commit 62ae855 into NVIDIA:dev Apr 8, 2024
15 checks passed
@parthosa parthosa deleted the spark-rapids-tools-865-883 branch April 8, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cindyyuanjiang cindyyuanjiang cindyyuanjiang approved these changes

@amahussein amahussein amahussein approved these changes

@nartal1 nartal1 nartal1 approved these changes

Assignees

@parthosa parthosa

Labels
bug Something isn't working core_tools Scope the core module (scala)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Support for WindowGroupLimitExec
4 participants
@parthosa @nartal1 @cindyyuanjiang @amahussein