Re: [PR] [SPARK-52444][SQL][CONNECT] Add support for Variant/Char/Varchar Literal [spark]

["dengziming (via GitHub)" <gi...@apache.org>]

dengziming commented on code in PR #51215:
URL: https://github.com/apache/spark/pull/51215#discussion_r2173116527


##########
sql/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -240,6 +243,21 @@ message Expression {
         Strings strings = 6;
       }
     }
+
+    message Variant {

Review Comment:
   I added a reference to Spark's VariantVal, and I searched the docs directory without finding expressions-related docs, so I changed no documents in the docs directory.



##########
sql/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -240,6 +243,21 @@ message Expression {
         Strings strings = 6;
       }
     }
+
+    message Variant {
+      bytes value = 1;
+      bytes metadata = 2;
+    }
+
+    message Char {
+      string value = 1;
+      optional int32 length = 2;

Review Comment:
   I think when using Char or Varchar, the length field is better to provide, or a String is preferred. We will use the length field if it's provided and do some validation, or use len(value) as a default when length is omitted. I added these descriptions to the documents.



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