Simplify the serialization of `ScalarValue::List` #3547

alamb · 2022-09-20T16:27:12Z

~~Draft as it builds on #3536 and doesn't pass all tests~~

Which issue does this PR close?

Rationale for this change

While working on #3531 I found the ScalarValue::List serialization code quite challenging to understand as it did not match the structure of ScalarValue::List. It can be drastically simplified and maintain the same semantics

I also hope a final PR in this series to remove PrimitiveScalarType but I didn't quite make it today and ran out of time

What changes are included in this PR?

Remove special case null handling for ScalarValue::List (it is already covered)

Are there any user-facing changes?

No

alamb · 2022-09-25T13:23:56Z

datafusion/proto/proto/datafusion.proto

@@ -714,6 +714,9 @@ message Union{
 }

 message ScalarListValue{
+    // encode null explicitly to distinguish a list with a null value
+    // from a list with no values)
+    bool is_null = 3;


I believe much of the complexity of the code to serialize List values was related to trying to figure out NULL values. Encoding it explicitly makes the code much simpler

Why is a null even being encoded at this level, and not using the null_value support at the ScalarValue level?

The short answer is I don't know -- the longer answer is that the null_value support at the scalar level requires a strange mirror type (PrimitiveScalarValue) which doesn't have the same support for types as the normal DataType (specifically it doesn't have a List equivalent) -- I am working in the background to remove that structure in #3612 and I will also try to remove this explict null coding as well

alamb · 2022-09-25T13:25:27Z

datafusion/proto/proto/datafusion.proto

@@ -825,17 +828,6 @@ enum PrimitiveScalarType{
    TIME64 = 27;
 }

-message ScalarType{


I am not sure why but the previous code was encoding ScalarValue::List using a different pattern than the struct. This PR changes the serialization code to use the same structure as ScalarValue::List -- the Field and the a list of scalar values, resulting in a drastic simplification

alamb · 2022-09-25T13:26:38Z

datafusion/proto/src/from_proto.rs

@@ -838,6 +754,23 @@ impl TryFrom<&protobuf::ScalarValue> for ScalarValue {
    }
 }

+/// Ensures that all `values` are of type DataType::List and have the
+/// same type as field
+fn validate_list_values(field: &Field, values: &[ScalarValue]) -> Result<(), Error> {


I am not sure how valuable it is to do this validation if serialized ScalarValue always came from DataFusion, but I wanted to keep the same semantics

It would be a weird edge case but if it did happen it would be hard to debug from whatever error happened downstream so I think it's worthwhile to do here.

alamb · 2022-09-25T13:27:05Z

datafusion/proto/src/lib.rs

-                "The value {:?} should not have been able to serialize. Serialized to :{:?}",
-                test_case, res
-            );
+            let proto: Result<super::protobuf::ScalarValue, super::to_proto::Error> =


Some of the validation is now done during deserialization rather than serialization so I had to change this test slightly

alamb · 2022-09-25T13:29:27Z

datafusion/proto/src/lib.rs

@@ -480,14 +492,11 @@ mod roundtrip_tests {
                    ScalarValue::Float32(Some(2.0)),
                    ScalarValue::Float32(Some(1.0)),
                ]),
-                DataType::List(new_box_field("level1", DataType::Float32, true)),
+                DataType::Float32,


These are not valid ScalarValue::List structures -- specifically, the type of the field on a List is the type of the element (aka it is not DataType::List(type of element), which is what this test was previously doing).

You can see correct usages in the rest of the codebase https://github.com/apache/arrow-datafusion/search?q=new_list

for example

https://github.com/apache/arrow-datafusion/blob/e0a9fa347f7adb1e6d6157b537e44c574e88244a/datafusion/physical-expr/src/aggregate/array_agg.rs#L174-L183

alamb · 2022-09-25T13:38:35Z

datafusion/proto/src/lib.rs

-            )),
-        ];
-
-        let should_fail: Vec<DataType> = vec![


This test is for serializing into a list specific protobuf that was removed in this PR

alamb · 2022-09-25T13:39:38Z

datafusion/proto/src/to_proto.rs

-                                        ))
-                                    }
-                                }
-                                (scalar::ScalarValue::Boolean(_), DataType::Boolean) => {


it is not clear why there was other type specific code to serializing lists - after this PR the existing code for serializing other types are used

codecov-commenter · 2022-09-25T13:53:16Z

Codecov Report

❗ No coverage uploaded for pull request base (master@b4c0601). Click here to learn what that means.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master    #3547   +/-   ##
=========================================
  Coverage          ?   86.08%           
=========================================
  Files             ?      300           
  Lines             ?    56329           
  Branches          ?        0           
=========================================
  Hits              ?    48489           
  Misses            ?     7840           
  Partials          ?        0

Impacted Files	Coverage Δ
datafusion/proto/src/from_proto.rs	`42.29% <100.00%> (ø)`
datafusion/proto/src/lib.rs	`93.93% <100.00%> (ø)`
datafusion/proto/src/to_proto.rs	`58.42% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

alamb · 2022-09-28T20:21:01Z

datafusion/proto/proto/datafusion.proto

@@ -714,6 +714,9 @@ message Union{
 }

 message ScalarListValue{
+    // encode null explicitly to distinguish a list with a null value
+    // from a list with no values)
+    bool is_null = 3;


I hope to remove this when I remove PrimitiveScalarType and use ArrowType instead

alamb · 2022-09-29T11:58:23Z

This PR is now ready for review. It cleans up a lot of this code -- I think I can push one more PR after this that will clean up even more

alamb · 2022-10-07T16:55:39Z

Unless there are any comments on this PR I plan to merge it tomorrow

thinkharderdev

Much simpler. Very nice!

thinkharderdev · 2022-10-07T22:10:48Z

datafusion/proto/src/from_proto.rs

@@ -838,6 +754,23 @@ impl TryFrom<&protobuf::ScalarValue> for ScalarValue {
    }
 }

+/// Ensures that all `values` are of type DataType::List and have the
+/// same type as field
+fn validate_list_values(field: &Field, values: &[ScalarValue]) -> Result<(), Error> {


It would be a weird edge case but if it did happen it would be hard to debug from whatever error happened downstream so I think it's worthwhile to do here.

xudong963

I was under the impression that I had reviewed this ticket a long time ago but forgot to leave an approval.

LGTM, a very nice simplification -- Very sorry for the delay in pushing this ticket to be merged

alamb · 2022-10-08T10:01:29Z

Thanks @thinkharderdev and @xudong963

ursabot · 2022-10-08T10:11:36Z

Benchmark runs are scheduled for baseline = 488b2ce and contender = e54110f. e54110f is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

alamb mentioned this pull request Sep 21, 2022

Add serialization of ScalarValue::Binary and ScalarValue::LargeBinary, ScalarValue::Time64 #3534

Merged

2 tasks

alamb force-pushed the alamb/clean_up_list_serialization branch 3 times, most recently from 1c36f13 to b30172d Compare September 25, 2022 13:22

alamb commented Sep 25, 2022

View reviewed changes

alamb mentioned this pull request Sep 25, 2022

Simplify serialization by removing redundant PrimitiveScalarValue #3612

Merged

Clean up list serialization code

9927216

alamb force-pushed the alamb/clean_up_list_serialization branch from b30172d to 9927216 Compare September 28, 2022 20:19

alamb commented Sep 28, 2022

View reviewed changes

alamb marked this pull request as ready for review September 29, 2022 11:58

alamb requested a review from tustvold September 30, 2022 20:15

thinkharderdev approved these changes Oct 7, 2022

View reviewed changes

xudong963 approved these changes Oct 8, 2022

View reviewed changes

alamb merged commit e54110f into apache:master Oct 8, 2022

alamb deleted the alamb/clean_up_list_serialization branch October 8, 2022 10:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify the serialization of `ScalarValue::List` #3547

Simplify the serialization of `ScalarValue::List` #3547

alamb commented Sep 20, 2022 •

edited

Loading

alamb Sep 25, 2022

tustvold Sep 25, 2022

alamb Sep 26, 2022 •

edited

Loading

alamb Sep 25, 2022 •

edited

Loading

alamb Sep 25, 2022

thinkharderdev Oct 7, 2022

alamb Sep 25, 2022

alamb Sep 25, 2022

alamb Sep 25, 2022

alamb Sep 25, 2022

codecov-commenter commented Sep 25, 2022 •

edited

Loading

alamb Sep 28, 2022

alamb commented Sep 29, 2022 •

edited

Loading

alamb commented Oct 7, 2022

thinkharderdev left a comment

thinkharderdev Oct 7, 2022

xudong963 left a comment

alamb commented Oct 8, 2022

ursabot commented Oct 8, 2022

Simplify the serialization of ScalarValue::List #3547

Simplify the serialization of ScalarValue::List #3547

Conversation

alamb commented Sep 20, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

alamb Sep 25, 2022 • edited Loading

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

codecov-commenter commented Sep 25, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

alamb commented Sep 29, 2022 • edited Loading

alamb commented Oct 7, 2022

thinkharderdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xudong963 left a comment

Choose a reason for hiding this comment

alamb commented Oct 8, 2022

ursabot commented Oct 8, 2022

Simplify the serialization of `ScalarValue::List` #3547

Simplify the serialization of `ScalarValue::List` #3547

alamb commented Sep 20, 2022 •

edited

Loading

alamb Sep 26, 2022 •

edited

Loading

alamb Sep 25, 2022 •

edited

Loading

codecov-commenter commented Sep 25, 2022 •

edited

Loading

alamb commented Sep 29, 2022 •

edited

Loading