[SPARK-41830][CONNECT][PYTHON][TESTS][FOLLOWUP] Enable parity test test_sample
#39765
Conversation
| @@ -884,7 +888,10 @@ def test_sample(self): | |||
|
|
|||
| self.assertRaises(TypeError, lambda: self.spark.range(1).sample(seed="abc")) | |||
|
|
|||
| self.assertRaises(IllegalArgumentException, lambda: self.spark.range(1).sample(-1.0)) | |||
There was a problem hiding this comment.
@itholic should we and a new subclass for IllegalArgumentException?
There was a problem hiding this comment.
@zhengruifeng Maybe are you asking if we want to add a new subclass for IllegalArgumentException such as PySparkIllegalArgumentException ?
If so, I don't think so because the IllegalArgumentException already has its own proper error class and handled in JVM side.
There was a problem hiding this comment.
Ah, I got it. We should catch IllegalArgumentException properly before SparkConnectException occurred in Spark Connect as mentioned in https://github.com/apache/spark/pull/39765/files#r1089673621.
Let me handle this in a separate ticket.
|
Could you rebase this PR, @zhengruifeng ? There is a conflict after merging your another PR in this area. |
|
@dongjoon-hyun Sure, thanks for the reviews of these test PRs! |
zhengruifeng
force-pushed
the
connect_enable_41830
branch
from
21966b8
to
c2500c5
Compare
| @@ -888,7 +889,10 @@ def test_sample(self): | |||
|
|
|||
| self.assertRaises(TypeError, lambda: self.spark.range(1).sample(seed="abc")) | |||
|
|
|||
| self.assertRaises(IllegalArgumentException, lambda: self.spark.range(1).sample(-1.0)) | |||
| self.assertRaises( | |||
| (IllegalArgumentException, SparkConnectException), | |||
There was a problem hiding this comment.
No a big deal, I think maybe we can catch SparkConnectGrpcException for more precious exception testing?
There was a problem hiding this comment.
Can we refine Spark Connect error to throw IllegalArgumentException similarly?
There was a problem hiding this comment.
Can we refine Spark Connect error to throw
IllegalArgumentExceptionsimilarly?
Ah, this way sounds better. Let me address it soon!
There was a problem hiding this comment.
Just submitted #39783.
This should be simplified back to self.assertRaises(IllegalArgumentException, lambda: self.spark.range(1).sample(-1.0)) after that change.
…est_sample` ### What changes were proposed in this pull request? Enable parity test `test_sample` ### Why are the changes needed? For test coverage ### Does this PR introduce _any_ user-facing change? no, test-only ### How was this patch tested? enabled test Closes #39765 from zhengruifeng/connect_enable_41830. Authored-by: Ruifeng Zheng <ruifengz@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 2fa1d6b) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
What changes were proposed in this pull request?
Enable parity test
test_sampleWhy are the changes needed?
For test coverage
Does this PR introduce any user-facing change?
no, test-only
How was this patch tested?
enabled test