[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #3612: Simplify serialization by removing redundant `PrimitiveScalarValue`

[GitBox <gi...@apache.org>]

alamb commented on code in PR #3612:
URL: https://github.com/apache/arrow-datafusion/pull/3612#discussion_r981209205


##########
datafusion/proto/proto/datafusion.proto:
##########
@@ -739,11 +742,19 @@ message IntervalMonthDayNanoValue {
   int64 nanos = 3;
 }
 
+message StructValue {
+  // encode null explicitly to distinguish a struct with no fields (is
+  // that possible?) from a null value
+  bool is_null = 1;
+  repeated ScalarValue field_values = 2;
+  repeated Field fields = 3;
+}
 
 message ScalarValue{
     oneof value {
-        // Null value of any type (type is encoded)
-        PrimitiveScalarType null_value = 19;
+        // was PrimitiveScalarType null_value = 19;
+        // Null value of any type
+        ArrowType null_value = 33;

Review Comment:
   You mean why are nulls typed in general? Basically because of how `ScalarValue::` is implemented (as an `Option<>` around the underlying native type). I think @Jimexist  tried to clean it up at some point and make `ScalarValue::None` and then all the variants like `ScalarValue::Int8` have values like `i8` rather than `Option<i8>`.
   
   I can't remember what the problem was but it didn't work easily. 
   
   In my opinion at least the serialization should follow how they are implemented in `ScalarValue` and if we improve `ScalarValue` then we can also improve the serialization code



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org