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

Fall back to the CPU for non-zero scale on Ceil or Floor functions [databricks] #4971

Merged
merged 9 commits into from
Mar 22, 2022
Merged

Fall back to the CPU for non-zero scale on Ceil or Floor functions [databricks] #4971

merged 9 commits into from
Mar 22, 2022

Conversation

nartal1
Copy link
Collaborator

@nartal1 nartal1 commented Mar 16, 2022

This fixes #4913.
Apache Spark has added scale parameter for Ceil/Floor functions in Spark-3.3 #4910. When ceil/floor functions are called with scale parameter, the physical plan changes compared to earlier. It adds RoundCeil/RoundFloor and the result type is decimal type.

In this PR, we have added support to RoundCeil and RoundFloor if the scale is 0. i.e we call GpuCeil if ceil is called with scale 0. For scale values other than 0, we fallback to CPU.

@nartal1 nartal1 added bug Something isn't working audit_3.3.0 Audit related tasks for 3.3.0 labels Mar 16, 2022
@nartal1 nartal1 added this to the Feb 28 - Mar 18 milestone Mar 16, 2022
@nartal1 nartal1 self-assigned this Mar 16, 2022
@nartal1
update copyright
a79bd52 
Signed-off-by: Niranjan Artal <nartal@nvidia.com>
@nartal1
Copy link
Collaborator Author

nartal1 commented Mar 16, 2022

build

@razajafri
Copy link
Collaborator

@gerashegalov @jlowe what do you guys think about adding utility classes, like RapidsFloorCeilUtils in this PR and RapidsErrorUtils previously, as we move forward as opposed to just adding methods in the SparkShims trait?
My vote is for having individual modular classes like the ones I mentioned above in favor of just adding methods to SparkShimsImpl.

@jlowe
Copy link
Member

jlowe commented Mar 17, 2022

what do you guys think about adding utility classes, like RapidsFloorCeilUtils in this PR and RapidsErrorUtils previously, as we move forward as opposed to just adding methods in the SparkShims trait?

I think it depends on what is being shimmed. If there's only one shim that needs the different behavior than the rest, then it can be nice to drop a new class in one of the large footprint directories, like 301until330-all, and cover a lot of shims all at once. If most shims need slightly different behavior then maybe the SparkShims approach makes more sense. There are base classes for SparkShim implementations that make this very similar to the separate class in terms of reuse, so it's more of a judgement call than a clear recommendation, IMO, unless we want to eventually eliminate SparkShims completely over time and only use purpose-built shim classes.

@nartal1
remove test
5afb004 
Signed-off-by: Niranjan Artal <nartal@nvidia.com>
razajafri
razajafri previously approved these changes Mar 18, 2022
View reviewed changes
@nartal1
Copy link
Collaborator Author

nartal1 commented Mar 18, 2022

build

revans2
revans2 reviewed Mar 18, 2022
View reviewed changes
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.

Just some nits and a question. Looks good.

integration_tests/src/main/python/arithmetic_ops_test.py Show resolved Hide resolved
.../src/main/301until330-all/scala/org/apache/spark/sql/rapids/shims/RapidsFloorCeilUtils.scala Outdated Show resolved Hide resolved
.../src/main/301until330-all/scala/org/apache/spark/sql/rapids/shims/RapidsFloorCeilUtils.scala Outdated Show resolved Hide resolved
@nartal1
addressed review comments
c843e75 
Signed-off-by: Niranjan Artal <nartal@nvidia.com>
@nartal1
Copy link
Collaborator Author

nartal1 commented Mar 21, 2022

build

@nartal1
update test
b55486b 
Signed-off-by: Niranjan Artal <nartal@nvidia.com>
revans2
revans2 previously approved these changes Mar 21, 2022
View reviewed changes
@revans2
Copy link
Collaborator

revans2 commented Mar 21, 2022

build

@nartal1 nartal1 changed the title Fall back to the CPU for non-zero scale on Ceil or Floor functions. Fall back to the CPU for non-zero scale on Ceil or Floor functions [databricks] Mar 21, 2022
@nartal1
Copy link
Collaborator Author

nartal1 commented Mar 21, 2022

Upmerged and updated code as 301 shim was removed recently.

@nartal1
Copy link
Collaborator Author

nartal1 commented Mar 21, 2022

build

@nartal1
Copy link
Collaborator Author

nartal1 commented Mar 22, 2022 •
edited
Loading

CI failed due to databricks test failure. Fix is in #4998. Thanks @razajafri. Will upmerge once that PR is merged.

@razajafri razajafri mentioned this pull request Mar 22, 2022
Merged
@nartal1
Copy link
Collaborator Author

nartal1 commented Mar 22, 2022

build

@nartal1 nartal1 merged commit 3b053cf into NVIDIA:branch-22.04 Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@revans2 revans2 revans2 approved these changes

@razajafri razajafri razajafri left review comments

Assignees

@nartal1 nartal1

Labels
audit_3.3.0 Audit related tasks for 3.3.0 bug Something isn't working
Projects
None yet
Milestone
Mar 21 - Apr 1
Development

Successfully merging this pull request may close these issues.

[BUG] Fall back to the CPU if we see a scale on Ceil or Floor
5 participants
@nartal1 @razajafri @jlowe @revans2 @sameerz