Skip to content

[SPARK-25949][SQL] Add test for PullOutPythonUDFInJoinCondition - #22955

Closed
xuanyuanking wants to merge 3 commits into
apache:masterfrom
xuanyuanking:SPARK-25949
Closed

[SPARK-25949][SQL] Add test for PullOutPythonUDFInJoinCondition#22955
xuanyuanking wants to merge 3 commits into
apache:masterfrom
xuanyuanking:SPARK-25949

Conversation

@xuanyuanking

@xuanyuankingxuanyuanking commented Nov 6, 2018

Copy link
Copy Markdown
Member

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.optimizer like other optimizer rules.

How was this patch tested?

new added UT

@SparkQA

SparkQA commented Nov 6, 2018

Copy link
Copy Markdown

Test build #98515 has finished for PR 22955 at commit 344b516.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class PullOutPythonUDFInJoinConditionSuite extends PlanTest

@xuanyuanking

xuanyuanking commented Nov 7, 2018

Copy link
Copy Markdown
MemberAuthor

cc @cloud-fan@mgaido91

@mgaido91

mgaido91 commented Nov 7, 2018

Copy link
Copy Markdown
Contributor

shall we also remove the end-to-end tests which are now not needed anymore?

@xuanyuanking

xuanyuanking commented Nov 8, 2018

Copy link
Copy Markdown
MemberAuthor

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.

mgaido91
mgaido91 reviewed Nov 8, 2018
View reviewed changes
Comment thread...est/scala/org/apache/spark/sql/catalyst/optimizer/PullOutPythonUDFInJoinConditionSuite.scala Outdated
Comment thread...est/scala/org/apache/spark/sql/catalyst/optimizer/PullOutPythonUDFInJoinConditionSuite.scala Outdated
Comment thread...est/scala/org/apache/spark/sql/catalyst/optimizer/PullOutPythonUDFInJoinConditionSuite.scala Outdated
Comment thread...est/scala/org/apache/spark/sql/catalyst/optimizer/PullOutPythonUDFInJoinConditionSuite.scala Outdated
}
}

test("python udf with other common condition") {

@mgaido91mgaido91Nov 8, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we add more cases like this for Or instead of And? And with several UDF/other conditions? Thanks.

@xuanyuankingxuanyuankingNov 9, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, add more cases in 38b1555.

@SparkQA

SparkQA commented Nov 9, 2018

Copy link
Copy Markdown

Test build #98644 has finished for PR 22955 at commit 38b1555.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@mgaido91

mgaido91 commented Nov 10, 2018

Copy link
Copy Markdown
Contributor

retest this please

mgaido91
mgaido91 reviewed Nov 10, 2018
View reviewed changes
Comment thread...est/scala/org/apache/spark/sql/catalyst/optimizer/PullOutPythonUDFInJoinConditionSuite.scala Outdated
condition = None).where(pythonUDF).analyze
val unsupportedJoinTypes = Seq(LeftOuter, RightOuter, FullOuter, LeftAnti)

private def comparePlansWithConf(query: LogicalPlan, expected: LogicalPlan): Unit = {

@mgaido91mgaido91Nov 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better naming? what does it mean WithConf?

@xuanyuankingxuanyuankingNov 11, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about comparePlanWithCrossJoinEnable? Just afraid it's too long at first, any advise :) Thanks.

Comment thread...est/scala/org/apache/spark/sql/catalyst/optimizer/PullOutPythonUDFInJoinConditionSuite.scala
}
}

test("inner join condition with python udf only") {

@mgaido91mgaido91Nov 10, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)...

@xuanyuankingxuanyuankingNov 11, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@SparkQA

SparkQA commented Nov 10, 2018

Copy link
Copy Markdown

Test build #98676 has finished for PR 22955 at commit 38b1555.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA

SparkQA commented Nov 11, 2018

Copy link
Copy Markdown

Test build #98698 has finished for PR 22955 at commit 8d04b4c.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@cloud-fan

cloud-fan commented Nov 12, 2018

Copy link
Copy Markdown
Contributor

thanks, merging to master!

@xuanyuanking

xuanyuanking commented Nov 12, 2018

Copy link
Copy Markdown
MemberAuthor

Thanks @mgaido91@cloud-fan

@xuanyuanking
xuanyuanking deleted the SPARK-25949 branch November 12, 2018 12:15
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@xuanyuankingJackey Lee
[SPARK-25949][SQL] Add test for PullOutPythonUDFInJoinCondition
ed0d1e1
## 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
Closesapache#22955 from xuanyuanking/SPARK-25949.
Authored-by: Yuanjian Li <xyliyuanjian@gmail.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@mgaido91mgaido91mgaido91 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@xuanyuanking@SparkQA@mgaido91@cloud-fan