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

Commonize more shim module files [databricks] #3577

Merged
merged 54 commits into from
Sep 22, 2021

Conversation

tgravescs
Copy link
Collaborator

This commonizes the majority of the rest of the shim module files excluding shuffle and the main Spark3XXShim file.

I tried to keep this easy to review and mostly moved files, a few functions in SparkBaseShims had to move to the respective Spark3XX file to make it common.

Signed-off-by: Thomas Graves <tgraves@nvidia.com>
Signed-off-by: Thomas Graves <tgraves@nvidia.com>
Signed-off-by: Thomas Graves <tgraves@nvidia.com>
Signed-off-by: Thomas Graves <tgraves@nvidia.com>
Signed-off-by: Thomas Graves <tgraves@nvidia.com>
Signed-off-by: Thomas Graves <tgraves@nvidia.com>
Signed-off-by: Thomas Graves <tgraves@nvidia.com>
Signed-off-by: Thomas Graves <tgraves@nvidia.com>
Signed-off-by: Thomas Graves <tgraves@nvidia.com>
@tgravescs tgravescs requested a review from revans2 as a code owner September 21, 2021 16:15
@tgravescs
Copy link
Collaborator Author

build

revans2
revans2 previously approved these changes Sep 21, 2021
pom.xml Outdated
@@ -141,6 +142,7 @@
<source>${project.basedir}/src/main/301until310-nondb/scala</source>
<source>${project.basedir}/src/main/301until320-nondb/scala</source>
<source>${project.basedir}/src/main/301until320-all/scala</source>
<source>${project.basedir}/src/main/301until310-all/scala</source>
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: There are getting to be enough directories now, that it would be good to have a consistent ordering to them, so it is simpler to tell if something is missing for example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah, I missed a few I tried moved some of them but didn't get them all. I need to update my IntelliJ as well as Gera mentioned it has a sort lines in new version, but we should decide on order

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sorted them

@tgravescs
Copy link
Collaborator Author

fyi - seeing some test failures locally so investigating them.

revans2
revans2 previously approved these changes Sep 21, 2021
@revans2
Copy link
Collaborator

revans2 commented Sep 21, 2021

I am happy to help resolve the conflicts. Those are related to a patch I did, that really just wants us to fall back to the CPU when ANSI is enabled for some timestamp operations. The fallback is for Spark 3.1.1+

@tgravescs
Copy link
Collaborator Author

wait to build until #3580 fixed

@@ -14,62 +14,47 @@
* limitations under the License.
*/

package com.nvidia.spark.rapids.shims.spark313
package com.nvidia.spark.rapids.shims.v2
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this whole file is a very odd diff, it did not get moved from 313 ithis got copied from GpuWindowExpression.scala

@tgravescs
Copy link
Collaborator Author

build

Copy link
Collaborator

@gerashegalov gerashegalov left a comment

Choose a reason for hiding this comment

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

LGTM

@tgravescs tgravescs merged commit bea4473 into NVIDIA:branch-21.10 Sep 22, 2021
@tgravescs tgravescs deleted the commonpandsWithJoin branch September 22, 2021 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Related to CI / CD or cleanly building
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants