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

Generate classes identical up to the shim package name [databricks] #11665

Merged

Conversation

gerashegalov
Copy link
Collaborator

@gerashegalov gerashegalov commented Oct 26, 2024

This PR adds the feature to shimplify to be able to define classes that are identical up to the package name requiring a spark.version.classifer interpolation in the package name

This allows to delete the per-shim RapidsShuffleManager

@gerashegalov gerashegalov changed the title Generate classes identical up to the shim package name Generate classes identical up to the shim package name [databricks] Oct 27, 2024
Signed-off-by: Gera Shegalov <[email protected]>
@gerashegalov gerashegalov self-assigned this Oct 27, 2024
@gerashegalov gerashegalov added build Related to CI / CD or cleanly building tech debt labels Oct 27, 2024
@gerashegalov
Copy link
Collaborator Author

build

Signed-off-by: Gera Shegalov <[email protected]>
@gerashegalov
Copy link
Collaborator Author

build

Signed-off-by: Gera Shegalov <[email protected]>
@gerashegalov
Copy link
Collaborator Author

build

spark-rapids-shim-json-lines ***/
package com.nvidia.spark.rapids.spark320
package com.nvidia.spark.rapids.$_spark.version.classifier_
Copy link
Member

Choose a reason for hiding this comment

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

Are we regressing any functionality in the IDE's ability to understand what's going on with this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the only change for IDE is that the developer needs a warning that the original file should be edited instead of the generated one. I'll add this warning

Copy link
Member

Choose a reason for hiding this comment

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

But when editing the original file, I assume the IDE is still totally confused by this package statement and thus doesn't understand what package the file is originally in. That would be an issue if there were many classes in the package in question, since the IDE would not realize all of those classes don't need to be imported. But in this specific case, that's not a problem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is undoubtedly room for confusion. But I think we can mitigate if the need arises. $spark.version.classifier is a legal package name. We could import such files directly and extend interpolation support to anywhere in the code.

@gerashegalov
Copy link
Collaborator Author

build

@gerashegalov gerashegalov merged commit 986eb5d into NVIDIA:branch-24.12 Oct 28, 2024
47 checks passed
@gerashegalov gerashegalov deleted the generateRapidsShuffleManager branch October 28, 2024 22:36
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 tech debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants