You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/10/12 21:26:21 UTC
[GitHub] [spark] hvanhovell commented on a diff in pull request #38200: [SPARK-40743][CONNECT] StructType should contain a list of StructField and each field should have a name
hvanhovell commented on code in PR #38200:
URL: https://github.com/apache/spark/pull/38200#discussion_r993909088
##########
connector/connect/src/main/protobuf/spark/connect/types.proto:
##########
@@ -55,134 +57,135 @@ message DataType {
uint32 user_defined_type_reference = 31;
}
- enum Nullability {
- NULLABILITY_UNSPECIFIED = 0;
- NULLABILITY_NULLABLE = 1;
- NULLABILITY_REQUIRED = 2;
- }
-
message Boolean {
uint32 type_variation_reference = 1;
- Nullability nullability = 2;
+ bool nullability = 2;
}
message I8 {
uint32 type_variation_reference = 1;
- Nullability nullability = 2;
+ bool nullability = 2;
}
message I16 {
uint32 type_variation_reference = 1;
- Nullability nullability = 2;
+ bool nullability = 2;
}
message I32 {
uint32 type_variation_reference = 1;
- Nullability nullability = 2;
+ bool nullability = 2;
}
message I64 {
uint32 type_variation_reference = 1;
- Nullability nullability = 2;
+ bool nullability = 2;
}
message FP32 {
uint32 type_variation_reference = 1;
- Nullability nullability = 2;
+ bool nullability = 2;
}
message FP64 {
uint32 type_variation_reference = 1;
- Nullability nullability = 2;
+ bool nullability = 2;
}
message String {
uint32 type_variation_reference = 1;
- Nullability nullability = 2;
+ bool nullability = 2;
}
message Binary {
uint32 type_variation_reference = 1;
- Nullability nullability = 2;
+ bool nullability = 2;
}
message Timestamp {
uint32 type_variation_reference = 1;
- Nullability nullability = 2;
+ bool nullability = 2;
}
message Date {
uint32 type_variation_reference = 1;
- Nullability nullability = 2;
+ bool nullability = 2;
}
message Time {
uint32 type_variation_reference = 1;
- Nullability nullability = 2;
+ bool nullability = 2;
}
message TimestampTZ {
uint32 type_variation_reference = 1;
- Nullability nullability = 2;
+ bool nullability = 2;
}
message IntervalYear {
uint32 type_variation_reference = 1;
- Nullability nullability = 2;
+ bool nullability = 2;
}
message IntervalDay {
uint32 type_variation_reference = 1;
- Nullability nullability = 2;
+ bool nullability = 2;
}
message UUID {
uint32 type_variation_reference = 1;
- Nullability nullability = 2;
+ bool nullability = 2;
}
// Start compound types.
message FixedChar {
int32 length = 1;
uint32 type_variation_reference = 2;
- Nullability nullability = 3;
+ bool nullability = 3;
}
message VarChar {
int32 length = 1;
uint32 type_variation_reference = 2;
- Nullability nullability = 3;
+ bool nullability = 3;
}
message FixedBinary {
int32 length = 1;
uint32 type_variation_reference = 2;
- Nullability nullability = 3;
+ bool nullability = 3;
}
message Decimal {
int32 scale = 1;
int32 precision = 2;
uint32 type_variation_reference = 3;
- Nullability nullability = 4;
+ bool nullability = 4;
+ }
+
+ message StructField {
+ DataType type = 1;
+ string name = 2;
+ bool nullability = 3;
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org