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

Add Shim JSON Headers for Databricks 13.3 #9488

Merged
merged 2 commits into from
Oct 20, 2023

Conversation

razajafri
Copy link
Collaborator

@razajafri razajafri commented Oct 19, 2023

This pull request introduces support for Databricks 13.3. This PR is part of a series of PRs that will address the complete support for Databricks 13.3. The primary focus of this PR is the addition of necessary shims for 341db.

Changes Made:

Shims for 341db: This PR includes the implementation of shims to support this version of Databricks, ensuring that our codebase remains compatible.

Testing:

Tests will be addressed in subsequent PRs

contributes towards #9175

@razajafri
Copy link
Collaborator Author

build

@razajafri razajafri self-assigned this Oct 19, 2023
@razajafri razajafri changed the title Add Support for Databricks 13.3 Add Shim Support for Databricks 13.3 Oct 19, 2023
@jlowe
Copy link
Member

jlowe commented Oct 19, 2023

The PR headline should be updated to be a bit more descriptive. As it is, it implies this PR adds support for Databricks 13.3 which isn't quite accurate.

@gerashegalov
Copy link
Collaborator

If this PR merely shimplify-generated it would help if we can see the command line used in the description.

Should spark341db.RapidsShuffleManager be part of the PR?

@razajafri
Copy link
Collaborator Author

If this PR merely shimplify-generated it would help if we can see the command line used in the description.

Should spark341db.RapidsShuffleManager be part of the PR?

Thank you for the comment. This isn't just shimplify-generated code but a commit from a branch that is already building. I have just broken the commits down so they are easier to review.

You can see the branch here.

The RapidsShuffleManager will be a part of the boilerplate that will follow in subsequent PRs.

@razajafri razajafri changed the title Add Shim Support for Databricks 13.3 Add Shim JSON Headers for Databricks 13.3 Oct 19, 2023
@razajafri
Copy link
Collaborator Author

build

@sameerz sameerz added feature request New feature or request task Work required that improves the product but is not user facing labels Oct 19, 2023
@gerashegalov
Copy link
Collaborator

If this PR merely shimplify-generated it would help if we can see the command line used in the description.
Should spark341db.RapidsShuffleManager be part of the PR?

Thank you for the comment. This isn't just shimplify-generated code but a commit from a branch that is already building. I have just broken the commits down so they are easier to review.

You can see the branch here.

The RapidsShuffleManager will be a part of the boilerplate that will follow in subsequent PRs.

In the future It is easier for the reviews if we follow the standard procedure that also takes care of the RapidsShuffleManager

mvn generate-sources -Dshimplify=true \
-Dshimplify.move=true -Dshimplify.overwrite=true \
-Dshimplify.add.shim=324 -Dshimplify.add.base=323

@razajafri
Copy link
Collaborator Author

razajafri commented Oct 19, 2023

If this PR merely shimplify-generated it would help if we can see the command line used in the description.
Should spark341db.RapidsShuffleManager be part of the PR?

Thank you for the comment. This isn't just shimplify-generated code but a commit from a branch that is already building. I have just broken the commits down so they are easier to review.
You can see the branch here.
The RapidsShuffleManager will be a part of the boilerplate that will follow in subsequent PRs.

In the future It is easier for the reviews if we follow the standard procedure that also takes care of the RapidsShuffleManager

mvn generate-sources -Dshimplify=true \
-Dshimplify.move=true -Dshimplify.overwrite=true \
-Dshimplify.add.shim=324 -Dshimplify.add.base=323

I do see your point. In this case, however, breaking down the commits into smaller chunked PRs was an afterthought. I agree it would've helped to review the changes if it was just the shimplify-generated code or something repeatable by the reviewers

@NvTimLiu
Copy link
Collaborator

NvTimLiu commented Oct 20, 2023

@razajafri

You can add 13.3 in the below line to run pre-merge CI with Databricks 13.3 against your PR, values '10.4', '11.3', '12.2', '13.3'
https://github.com/NVIDIA/spark-rapids/blob/branch-23.12/jenkins/Jenkinsfile-blossom.premerge-databricks#L91

@razajafri razajafri merged commit fba1a4b into NVIDIA:branch-23.12 Oct 20, 2023
29 of 30 checks passed
@razajafri razajafri deleted the SP-9175-shim-json-lines branch October 20, 2023 06:00
@razajafri
Copy link
Collaborator Author

@razajafri

You can add 13.3 in the below line to run pre-merge CI with Databricks 13.3 against your PR, values '10.4', '11.3', '12.2', '13.3' https://github.com/NVIDIA/spark-rapids/blob/branch-23.12/jenkins/Jenkinsfile-blossom.premerge-databricks#L91

Thanks, I will add it after the pom changes are in as we aren't building against 13.3 yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request task Work required that improves the product but is not user facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants