[SPARK-48370][CONNECT] Checkpoint and localCheckpoint in Scala Spark Connect client #46683
Conversation
This file contains 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
zhengruifeng
approved these changes
May 21, 2024
hvanhovell
reviewed
May 21, 2024
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/Dataset.scala
Outdated
Show resolved
Hide resolved
hvanhovell
reviewed
May 21, 2024
connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/ClientE2ETestSuite.scala
Outdated
Show resolved
Hide resolved
hvanhovell
reviewed
May 21, 2024
...r/connect/common/src/main/scala/org/apache/spark/sql/connect/client/SparkConnectClient.scala
Show resolved
Hide resolved
HyukjinKwon
force-pushed
the
SPARK-48370
branch
4 times, most recently
from
cd18fe4
to
7778484
Compare
HyukjinKwon
commented
May 22, 2024
| @@ -0,0 +1,176 @@ | |||
| /* | |||
There was a problem hiding this comment.
hvanhovell
reviewed
May 22, 2024
| private var shouldStopCleaner = false | ||
| private[sql] lazy val cleaner = { | ||
| shouldStopCleaner = true | ||
| new ContextCleaner(this) |
There was a problem hiding this comment.
Why not use the JVM one?
There was a problem hiding this comment.
The problem is that the interface forces to fix generated code org.apache.spark.connect.proto.CachedRemoteRelation to inherit AutoCloseable, or I have to create a wrapper (see https://docs.oracle.com/javase/9/docs/api/java/lang/ref/Cleaner.html)
There was a problem hiding this comment.
And seems like Java protobuf gen does not allow it:
Don’t go looking for facilities similar to class inheritance, though – protocol buffers don’t do that.
https://protobuf.dev/getting-started/javatutorial/#defining-your-protocol-format
There was a problem hiding this comment.
Let me make it simpler at least.
HyukjinKwon
force-pushed
the
SPARK-48370
branch
2 times, most recently
from
2cca97e
to
fad0593
Compare
dongjoon-hyun
approved these changes
May 22, 2024
HyukjinKwon
added a commit
that referenced
this pull request
May 22, 2024
…n instead of DataFrame ### What changes were proposed in this pull request? This PR addresses #46683 (comment) comment within Python, by using ID at the plan instead of DataFrame itself. ### Why are the changes needed? Because the DataFrame holds the relation ID, if DataFrame B are derived from DataFrame A, and DataFrame A is garbage-collected, then the cache might not exist anymore. See the example below: ```python df = spark.range(1).localCheckpoint() df2 = df.repartition(10) del df df2.collect() ``` ``` pyspark.errors.exceptions.connect.SparkConnectGrpcException: (org.apache.spark.sql.connect.common.InvalidPlanInput) No DataFrame with id a4efa660-897c-4500-bd4e-bd57cd0263d2 is found in the session cd4764b4-90a9-4249-9140-12a6e4a98cd3 ``` ### Does this PR introduce _any_ user-facing change? No, the main change has not been released out yet. ### How was this patch tested? Manually tested, and added a unittest. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #46694 from HyukjinKwon/SPARK-48258-followup. Authored-by: Hyukjin Kwon <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
|
I believe I addressed all major comments here. I need to make one more followup to fix both Scala/Python. Let me merge this one. and address others in that PR if there are more to fix. Merged to master. |
HyukjinKwon
added a commit
that referenced
this pull request
May 23, 2024
… and eager required fields in CheckpointCommand ### What changes were proposed in this pull request? This PR is a followup of #46683 and #46570 that refactors `local` and `eager` required fields in `CheckpointCommand` ### Why are the changes needed? To make the code easier to maintain. ### Does this PR introduce _any_ user-facing change? No, the main change has not been released yet. ### How was this patch tested? Manually tested. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #46712 from HyukjinKwon/SPARK-48370-SPARK-48258-followup. Authored-by: Hyukjin Kwon <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
HyukjinKwon
added a commit
that referenced
this pull request
May 28, 2024
### What changes were proposed in this pull request? This PR is a followup of #46683 that replaces our custom cleaner to JDK's cleaner. ### Why are the changes needed? Reuse the standard builtin library. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? I manually tested via reenabling `CheckpointSuite.checkpoint gc derived DataFrame` ### Was this patch authored or co-authored using generative AI tooling? No. Closes #46726 from HyukjinKwon/SPARK-48370-followup. Authored-by: Hyukjin Kwon <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
riyaverm-db
pushed a commit
to riyaverm-db/spark
that referenced
this pull request
Jun 7, 2024
### What changes were proposed in this pull request? This PR is a followup of apache#46683 that replaces our custom cleaner to JDK's cleaner. ### Why are the changes needed? Reuse the standard builtin library. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? I manually tested via reenabling `CheckpointSuite.checkpoint gc derived DataFrame` ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#46726 from HyukjinKwon/SPARK-48370-followup. Authored-by: Hyukjin Kwon <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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?
This PR adds
Dataset.checkpointandDataset.localCheckpointinto Scala Spark Connect client. Python API was implemented at #46570Why are the changes needed?
For API parity.
Does this PR introduce any user-facing change?
Yes, it adds
Dataset.checkpointandDataset.localCheckpointinto Scala Spark Connect client.How was this patch tested?
Unittests added.
Was this patch authored or co-authored using generative AI tooling?
No.