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/11/10 21:55:43 UTC

[GitHub] [spark] amaliujia commented on a diff in pull request #38605: [SPARK-41103][CONNECT][DOC] Document how to add a new proto field of messages

amaliujia commented on code in PR #38605:
URL: https://github.com/apache/spark/pull/38605#discussion_r1019647668


##########
connector/connect/docs/adding-proto-messages.md:
##########
@@ -0,0 +1,87 @@
+# Required, Optional and default values
+
+Connect adopts proto3, which does not support `required` constraint anymore.
+Every field is optional. For non-message proto fields, there is also no `hasXXX`
+functions to easy tell if a filed is set or not-set. For those non-message proto,
+they also have default values. E.g. int has 0 as the default value.
+
+
+### Required field
+
+Even proto3 does not offer `required` constraint, there is still some fields that
+are semantically required. For such case, we share add comment on the field to mark
+it as required. The expectation for client implementation (or any submitted proto plan)
+is that such fields should be always set, and server will always assume such fields
+are set and use whatever values of the fields directly. It is the client side's fault
+to not offer meaningful value for `required` field and in that case, the behavior on
+the server side is not defined.
+
+
+### Optional field and default value
+
+Optional fields could have default values when the field is not set, and we are seeing
+two cases that depends on whether the default value for the proto field is consistent
+with the corresponding Spark parameter default value.
+
+If both default values of the proto field and Spark parameter are the same, we keep the
+proto field as is.
+
+For example:
+```Scala
+// Spark Plan.
+class FooPlan(size: Long = 0)
+```
+
+```protobuf
+// Connect proto.
+message Foo {
+  int64 size; 

Review Comment:
   I intentionally do not add the proto field number.  This is to avoid people mis-understand it as a default value thus causing confusions.



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