[SPARK-40534][CONNECT] Extend the support for Join with different join types #38157

amaliujia · 2022-10-08T03:39:29Z

What changes were proposed in this pull request?

Extend the support for Join with different join types. Before this PR, all joins are hardcoded inner type. So this PR supports other join types.
Add join to connect DSL.
Update a few Join proto fields to better reflect the semantic.

Why are the changes needed?

Extend the support for Join in connect.

Does this PR introduce any user-facing change?

No

How was this patch tested?

UT

amaliujia · 2022-10-08T03:40:11Z

R: @cloud-fan

grundprinzip · 2022-10-09T09:06:54Z

connector/connect/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala

 import org.apache.spark.sql.catalyst.analysis.{UnresolvedAlias, UnresolvedAttribute, UnresolvedFunction, UnresolvedRelation, UnresolvedStar}
+import org.apache.spark.sql.catalyst.expressions
 import org.apache.spark.sql.catalyst.expressions.{Attribute, AttributeReference, Expression}
-import org.apache.spark.sql.catalyst.plans.logical


can you please keep the package import? This makes it easier to read where the specific classes come from in particular when they have similar names.

So it's easier to ready seeing logical.JoinType and proto.JoinType for example.

It is actually still there :). IDE seems auto pack things into import org.apache.spark.sql.catalyst.plans.{logical, Cross, FullOuter, Inner, JoinType, LeftAnti, LeftOuter, LeftSemi, RightOuter}

grundprinzip · 2022-10-09T09:16:51Z

...tor/connect/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectProtoSuite.scala

  }

+  test("Basic joins with different join types") {
+    val connectPlan = {


can you add a test-case for unspecified as well to see that we catch the error?

I discussed with @cloud-fan and we decided to remove both JoinType.CROSS and JoinType.Unspecified.

Proto is our API. We should make the proto itself less ambiguous. For example any proto submitted to the server should not contain a JoinType.Unspecified. It is either set a join type with explicit semantic (inner, left outer, etc.) or not set, which Spark by default treats it as inner join.

The reason for unspecified is not the proto contract but the language behavior for different auto generated targets. To avoid issues with defaults, the recommendation in the typical proto style guides is to always have the first element of an enum be unspecified.

Cc @cloud-fan

If the proto is an API, I'd say the join type is a required field and clients must set the join type in the join plan. For the python client, its dataframe API can omit the join type, and the python client should use INNER as the default join type.

There are two layers of this discussion one on the proto infra level and one on the API level. I'm fine with the API level decision.

My point referred to the recommendations when using protos:

All enums must have an unspecified element due to the different ways languages deal with default construction (cf https://developers.google.com/protocol-buffers/docs/style#enums)

There is no required field, all elements are always optional in proto3.

Ah if this is the style guide of protobuf, let's keep it.

And the server can simply fail if it sees UNSPECIFIED join type?

Yes exactly.

amaliujia · 2022-10-10T07:05:34Z

@cloud-fan comment addressed.

connector/connect/src/main/scala/org/apache/spark/sql/connect/dsl/package.scala

grundprinzip · 2022-10-10T07:27:50Z

connector/connect/src/main/protobuf/spark/connect/relations.proto

This will need an unspecified element - https://developers.google.com/protocol-buffers/docs/style#enums

grundprinzip

Please maintain the styleguide for the enum values.

AmplabJenkins · 2022-10-10T11:14:01Z

Can one of the admins verify this patch?

amaliujia · 2022-10-10T20:01:54Z

I added the JOIN_TYPE_UNSPECIFIED.

BTW I found JOIN_TYPE_UNSPECIFIED is already tested by SparkPlannerConnectSuite.

cloud-fan · 2022-10-11T01:35:30Z

thanks, merging to master!

…ient ### What changes were proposed in this pull request? Following up #38157, add all join types support for python. Please note that in the PR we decided to not support CROSS Join now. ### Why are the changes needed? Finish the join type support in Connect. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? UT Closes #38243 from amaliujia/python_join_types. Authored-by: Rui Wang <rui.wang@databricks.com> Signed-off-by: Ruifeng Zheng <ruifengz@apache.org>

…ient ### What changes were proposed in this pull request? Following up apache#38157, add all join types support for python. Please note that in the PR we decided to not support CROSS Join now. ### Why are the changes needed? Finish the join type support in Connect. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? UT Closes apache#38243 from amaliujia/python_join_types. Authored-by: Rui Wang <rui.wang@databricks.com> Signed-off-by: Ruifeng Zheng <ruifengz@apache.org>

[SPARK-40534] Support join types.

43e42fe

github-actions bot added CONNECT SQL labels Oct 8, 2022

update

3e3d501

grundprinzip reviewed Oct 9, 2022

View reviewed changes

cloud-fan approved these changes Oct 10, 2022

View reviewed changes

update

cbfd5d7

cloud-fan reviewed Oct 10, 2022

View reviewed changes

connector/connect/src/main/scala/org/apache/spark/sql/connect/dsl/package.scala Outdated Show resolved Hide resolved

grundprinzip reviewed Oct 10, 2022

View reviewed changes

grundprinzip requested changes Oct 10, 2022

View reviewed changes

update

80a7ede

amaliujia force-pushed the SPARK-40534 branch from 3a131bc to 80a7ede Compare October 10, 2022 19:59

fix indents

c884729

cloud-fan approved these changes Oct 11, 2022

View reviewed changes

cloud-fan closed this in 67c6408 Oct 11, 2022

amaliujia deleted the SPARK-40534 branch October 11, 2022 02:23

amaliujia mentioned this pull request Oct 13, 2022

[SPARK-40534][CONNECT][FOLLOW-UP] Support all join types in python client #38243

Closed

[SPARK-40534][CONNECT] Extend the support for Join with different join types #38157

[SPARK-40534][CONNECT] Extend the support for Join with different join types #38157

Uh oh!

Conversation

amaliujia commented Oct 8, 2022

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

amaliujia commented Oct 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amaliujia Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amaliujia commented Oct 10, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grundprinzip left a comment

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Oct 10, 2022

Uh oh!

amaliujia commented Oct 10, 2022

Uh oh!

cloud-fan commented Oct 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

amaliujia Oct 10, 2022 •

edited

Loading