[SPARK-40743][CONNECT] StructType should contain a list of StructField and each field should have a name #38200

amaliujia · 2022-10-11T06:04:23Z

What changes were proposed in this pull request?

This PR refactors proto's struct datatype with 1) each struct has a struct field list 2) each struct field has a name.

Why are the changes needed?

In the past, connect's struct datatype does not have name for its struct fields.

Does this PR introduce any user-facing change?

No

How was this patch tested?

UT

amaliujia · 2022-10-11T06:04:40Z

R: @cloud-fan
cc @grundprinzip @HyukjinKwon

amaliujia · 2022-10-11T06:05:19Z

BTW struct type is a blocker for us to support the full version of LocalRelation which is a blocker for us to support sparkSession.createDataFrame(in-memory data structure).

amaliujia · 2022-10-11T06:07:51Z

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

+
+      def protoQualifiedAttr: proto.Expression.QualifiedAttribute =
+        proto.Expression.QualifiedAttribute.newBuilder()
+          .setName(identifier.mkString("."))


For LocalRelation's attributes, how to convert identifier: Seq[String] to it?

Do we need to add an extra qualifier: Seq[String]?

actually a.b should not directly map to QualifiedAttribute. It can be GetStructField(QualifiedAttribute...)

I think we can follow the catalyst dsl

if we call protoAttr, then we parse the name (split by dot) and create UnresolvedAttribute

if we call bool, then we directly use the name as attribute name and create AttributeReference

ah let me take a look.

cloud-fan · 2022-10-11T06:30:00Z

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

+  message StructField {
+    DataType type = 1;
+    string name = 2;
+    Nullability nullability = 3;


isn't Nullability just a boolean?

and shall we include metadata?

AmplabJenkins · 2022-10-11T21:45:14Z

Can one of the admins verify this patch?

hvanhovell · 2022-10-12T21:26:06Z

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

+  message StructField {
+    DataType type = 1;
+    string name = 2;
+    bool nullability = 3;


The current data model assumes that the type itself expresses its nullability, so why do we need nullability here?

I am not saying that the current approach of the datatype having nullability is great. I would also be good if we only set nullability in the StructField message.

For a pure type system design, for composable type in which there are sub-types, both parent and children can have nullability. Use StructType as an example:

Struct(nullability=true, structFields=[(name="a", nullability=true)]):
val x = Struct("a"=NULL) // valid
val y: Struct(nullability=true, structFields=[(name="a", nullability=true)] = NULL // valid

Struct(nullability=true, structFields=[(name="a", nullability=false)]):
val x = Struct("a"=NULL) // invalid
val y: Struct(nullability=true, structFields=[(name="a", nullability=true)] = NULL // valid

Struct(nullability=false, structFields=[(name="a", nullability=false)]):
val x = Struct("a"=NULL) // invalid
val y: Struct(nullability=true, structFields=[(name="a", nullability=true)] = NULL // invalid

Same for ARRAY, MAP, etc.

This is very general. I am thinking you are thinking a data model that is a subset of it?

This is different from Catalyst. The catalyst data model is:

most data types are just enums: string, int, float, etc.

some data types have parameters: decimal(3, 1), varchar(10), etc.

there are 3 collection types: struct, array, map. These 3 types have nullability: struct field nullability, array element nullability, map value nullability.

Why is proto data type different?

I believe the connect proto type system is a bit different on the container type. However I just refactor code here and this is existing code. We don't have tests for types neither.

How about let me follow up on this topic later after having more discussions with people. I can add more tests for nullability at that time.

… field should have a name.

cloud-fan · 2022-10-18T02:08:18Z

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

@@ -114,7 +115,26 @@ class SparkConnectProtoSuite extends PlanTest with SparkConnectPlanTest {
      import org.apache.spark.sql.connect.dsl.plans._
      transform(connectTestRelation.as("target_table"))
    }
+


unnecessary change

cloud-fan · 2022-10-18T02:09:13Z

thanks, merging to master!

…d and each field should have a name ### What changes were proposed in this pull request? This PR refactors proto's struct datatype with 1) each struct has a struct field list 2) each struct field has a name. ### Why are the changes needed? In the past, connect's struct datatype does not have name for its struct fields. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT Closes apache#38200 from amaliujia/struct_type_should_contain_name. Authored-by: Rui Wang <rui.wang@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

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

amaliujia commented Oct 11, 2022

View reviewed changes

HyukjinKwon changed the title ~~[SPARK-40743][CONNECT]StructType should contain a list of StructField and each field should have a name~~ [SPARK-40743][CONNECT] StructType should contain a list of StructField and each field should have a name Oct 11, 2022

cloud-fan reviewed Oct 11, 2022

View reviewed changes

amaliujia mentioned this pull request Oct 12, 2022

[SPARK-40587][CONNECT][FOLLOW-UP] Make sure python client support select * #38218

Closed

hvanhovell reviewed Oct 12, 2022

View reviewed changes

amaliujia added 7 commits October 17, 2022 07:27

SPARK-40743] StructType should contain a list of StructField and each…

27a8ff4

… field should have a name.

update

cfa8d19

update

cb2deef

update

d77540d

update

9c13b77

update

55e399a

update

993c2c3

amaliujia force-pushed the struct_type_should_contain_name branch from db4ea09 to 993c2c3 Compare October 17, 2022 14:30

github-actions bot added CORE PYTHON labels Oct 17, 2022

amaliujia added 2 commits October 17, 2022 08:21

update

8bfbd72

update

4c88c46

cloud-fan reviewed Oct 18, 2022

View reviewed changes

cloud-fan approved these changes Oct 18, 2022

View reviewed changes

cloud-fan closed this in 269acf7 Oct 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-40743][CONNECT] StructType should contain a list of StructField and each field should have a name #38200

[SPARK-40743][CONNECT] StructType should contain a list of StructField and each field should have a name #38200

amaliujia commented Oct 11, 2022

amaliujia commented Oct 11, 2022

amaliujia commented Oct 11, 2022 •

edited

amaliujia Oct 11, 2022

cloud-fan Oct 11, 2022

cloud-fan Oct 11, 2022

amaliujia Oct 11, 2022

cloud-fan Oct 11, 2022

cloud-fan Oct 11, 2022

amaliujia Oct 12, 2022

AmplabJenkins commented Oct 11, 2022

hvanhovell Oct 12, 2022

amaliujia Oct 12, 2022 •

edited

cloud-fan Oct 13, 2022

amaliujia Oct 13, 2022 •

edited

cloud-fan Oct 18, 2022

cloud-fan commented Oct 18, 2022

[SPARK-40743][CONNECT] StructType should contain a list of StructField and each field should have a name #38200

[SPARK-40743][CONNECT] StructType should contain a list of StructField and each field should have a name #38200

Conversation

amaliujia commented Oct 11, 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?

amaliujia commented Oct 11, 2022

amaliujia commented Oct 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Oct 11, 2022

Choose a reason for hiding this comment

amaliujia Oct 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaliujia Oct 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Oct 18, 2022

amaliujia commented Oct 11, 2022 •

edited

amaliujia Oct 12, 2022 •

edited

amaliujia Oct 13, 2022 •

edited