[SPARK-42585][CONNECT][FOLLOWUP] Store cached local relations as proto#41107
Closed
MaxGekk wants to merge 7 commits intoapache:masterfrom
Closed
[SPARK-42585][CONNECT][FOLLOWUP] Store cached local relations as proto#41107MaxGekk wants to merge 7 commits intoapache:masterfrom
MaxGekk wants to merge 7 commits intoapache:masterfrom
Conversation
MaxGekk
changed the title
MaxGekk
changed the title
HyukjinKwon
approved these changes
May 9, 2023
HyukjinKwon
approved these changes
May 9, 2023
amaliujia
reviewed
May 10, 2023
| .setSchema(schema) | ||
| .setData(data) | ||
| .build() | ||
| artifactManager.cacheArtifact(localRelation.toByteArray) |
Contributor
There was a problem hiding this comment.
is the byte format of the proto LocalRelation compatible with existing code? FYI @vicennial
Contributor
There was a problem hiding this comment.
Oh I see so from the server side it also re-construct LocalRelation proto from the bytes. Probably @vicennial can confirm if all the places are updated.
Contributor
There was a problem hiding this comment.
Thanks for the heads up! It looks good to me
vicennial
approved these changes
May 10, 2023
Member
Author
|
The GA Run / Build modules: pyspark-sql, pyspark-mllib, pyspark-resource frozen in the run https://github.com/MaxGekk/spark/runs/13365446543 but it passed in https://github.com/MaxGekk/spark/runs/13370236086 |
Member
Author
|
Merging to master. Thank you, @HyukjinKwon @vicennial @amaliujia for review. |
LuciferYang
pushed a commit
to LuciferYang/spark
that referenced
this pull request
May 10, 2023
### What changes were proposed in this pull request? In the PR, I propose to store the cached local relations in the proto format, the same as `LocalRelation`. Also I reverted `transformLocalRelation()` to the state before the commit apache@0d7618a. ### Why are the changes needed? To explicitly specify the format of cached local relations in the proto API. ### Does this PR introduce _any_ user-facing change? Yes but the feature of cached local relations haven't been released yet. ### How was this patch tested? By running the existing tests: ``` $ build/sbt "test:testOnly *.ArtifactManagerSuite" $ build/sbt "test:testOnly *.ClientE2ETestSuite" $ build/sbt "test:testOnly *.ArtifactStatusesHandlerSuite" ``` Closes apache#41107 from MaxGekk/cached-blob-in-proto. Authored-by: Max Gekk <max.gekk@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
In the PR, I propose to store the cached local relations in the proto format, the same as
LocalRelation. Also I revertedtransformLocalRelation()to the state before the commit 0d7618a.Why are the changes needed?
To explicitly specify the format of cached local relations in the proto API.
Does this PR introduce any user-facing change?
Yes but the feature of cached local relations haven't been released yet.
How was this patch tested?
By running the existing tests: