[SPARK-24521][SQL][TEST] Fix ineffective test in CachedTableSuite #21531
Conversation
| var evalCount = 0 | ||
| val myUDF = udf((x: String) => { evalCount += 1; "result" }) | ||
| val df = Seq(("test", 1)).toDF("s", "i").select(myUDF($"s")) | ||
| df.cache() |
There was a problem hiding this comment.
Here is when evalCount is incremented (because the optimizer will execute the UDF locally on driver), the rest of the tests have no effect on the value of evalCount.
evalCount is copied (by value) to the closure and therefore it's not possible to change the value evalCount defined in the driver by incrementing it in the UDF (because each closure has a different copy of evalCount)
|
cc @maryannxue |
icexelloss
changed the title
|
Test build #91672 has finished for PR 21531 at commit
|
|
retest this please |
|
Test build #91713 has finished for PR 21531 at commit
|
|
retest this please |
|
Test build #91719 has finished for PR 21531 at commit
|
|
|
||
| assertCached(df2) | ||
|
|
||
| failAfter(5 seconds) { |
There was a problem hiding this comment.
Can you add a comment here, like "udf has been evaluated during caching, and thus should not be re-evaluated."
|
|
||
| test("persist and then withColumn") { | ||
| val df = Seq(("test", 1)).toDF("s", "i") | ||
| // We should not invalidate the cached DataFrame |
There was a problem hiding this comment.
Remove this comment? Or what is it were you trying to say?
|
Thanks for the review! I addressed the comments |
|
Test build #91800 has finished for PR 21531 at commit
|
|
Jenkins, retest this please |
|
Test build #91819 has finished for PR 21531 at commit
|
|
retest this please |
|
Test build #91848 has finished for PR 21531 at commit
|
|
LGTM Thanks! Merged to master/2.3 |
5 participants
What changes were proposed in this pull request?
test("withColumn doesn't invalidate cached dataframe") in CachedTableSuite doesn't not work because:
The UDF is executed and test count incremented when "df.cache()" is called and the subsequent "df.collect()" has no effect on the test result.
This PR fixed this test and add another test for caching UDF.
How was this patch tested?
Add new tests.