-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-51334][CONNECT] Add java/scala version in analyze spark_version response #50102
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
Conversation
I think we could add some tests here to verify the change in the response? |
@the-sakthi do you know where i can write them? im not seeing a relevant test suite that already includes tests for analyze plan handler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM, please add a test via the Scala E2E tests.
|
||
message SparkVersion { | ||
string version = 1; | ||
// The java version the Spark Connect Service is built with. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct, @garlandz-db ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree this is the JRE version not the built version. Same is true for the Scala version likely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, this PR provides the runtime Java version instead of The java version the Spark Connect Service is built with
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to #50102 (review), this PR seems to cause a semantic confusion to SparkConnect
eco-system because SparkVersion
is completely irrelevant from the underlying runtime Java
version which can be Java 17 or Java 21.
message SparkVersion {
@dongjoon-hyun IMHO it is ok to return these values, for language agnostic clients they can simply inquire the server about how it runs. I don't think this creates any inconsistency. |
@dongjoon-hyun please take another pass thanks! |
To @grundprinzip and @garlandz-db , could you propose new messages instead of touching the existing message
|
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
What changes were proposed in this pull request?
Why are the changes needed?
Does this PR introduce any user-facing change?
How was this patch tested?
existing tests
Was this patch authored or co-authored using generative AI tooling?
No