[SPARK-25949][SQL] Add test for PullOutPythonUDFInJoinCondition #22955
Conversation
|
Test build #98515 has finished for PR 22955 at commit
|
|
shall we also remove the end-to-end tests which are now not needed anymore? |
|
Thanks for the reply, unnecessary end-to-end tests removed in 2b6977d, others maybe should be kept? Cause mock python udf in scala side can't 100% same with python side. |
...est/scala/org/apache/spark/sql/catalyst/optimizer/PullOutPythonUDFInJoinConditionSuite.scala
Outdated
Show resolved
Hide resolved
...est/scala/org/apache/spark/sql/catalyst/optimizer/PullOutPythonUDFInJoinConditionSuite.scala
Outdated
Show resolved
Hide resolved
...est/scala/org/apache/spark/sql/catalyst/optimizer/PullOutPythonUDFInJoinConditionSuite.scala
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| test("python udf with other common condition") { |
There was a problem hiding this comment.
shall we add more cases like this for Or instead of And? And with several UDF/other conditions? Thanks.
There was a problem hiding this comment.
Thanks, add more cases in 38b1555.
|
Test build #98644 has finished for PR 22955 at commit
|
|
retest this please |
|
|
||
| private def comparePlansWithConf(query: LogicalPlan, expected: LogicalPlan): Unit = { |
There was a problem hiding this comment.
better naming? what does it mean WithConf?
There was a problem hiding this comment.
how about comparePlanWithCrossJoinEnable? Just afraid it's too long at first, any advise :) Thanks.
| } | ||
| } | ||
|
|
||
| test("inner join condition with python udf only") { |
There was a problem hiding this comment.
sorry, probably I was not clear enough in my previous comment. This UT and the following differ only for the join type. We can dedup them by doing something like:
Seq(Inner, LeftSemi).foreach { joinType =>
test(...) { ...}
}
PS nit: maybe we can also define a new val supportedJoinTypes = Seq(Inner, LeftSemi)...
There was a problem hiding this comment.
I'm sorry for lacking of comments to your previous comment they differ only by the join type..., they differ not only the type, but also the expected plan.
|
Test build #98676 has finished for PR 22955 at commit
|
|
Test build #98698 has finished for PR 22955 at commit
|
|
thanks, merging to master! |
|
Thanks @mgaido91 @cloud-fan |
## What changes were proposed in this pull request? As comment in apache#22326 (comment), we test the new added optimizer rule by end-to-end test in python side, need to add suites under `org.apache.spark.sql.catalyst.optimizer` like other optimizer rules. ## How was this patch tested? new added UT Closes apache#22955 from xuanyuanking/SPARK-25949. Authored-by: Yuanjian Li <xyliyuanjian@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
What changes were proposed in this pull request?
As comment in #22326 (comment), we test the new added optimizer rule by end-to-end test in python side, need to add suites under
org.apache.spark.sql.catalyst.optimizerlike other optimizer rules.How was this patch tested?
new added UT