You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/09/25 14:03:48 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #3612: Simplify serialization by removing redundant `PrimitiveScalarValue`
alamb opened a new pull request, #3612:
URL: https://github.com/apache/arrow-datafusion/pull/3612
Draft as it builds on https://github.com/apache/arrow-datafusion/pull/3547
# Which issue does this PR close?
Re https://github.com/apache/arrow-datafusion/issues/3531
# Rationale for this change
While working on https://github.com/apache/arrow-datafusion/issues/3531 I noticed that `PrimitiveScalarType` duplciated the functionality of `DataType`.
# What changes are included in this PR?
1. Remove `PrimitiveScalarValue` and use the normal Arrow DataType
# Are there any user-facing changes?
No
--
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
[GitHub] [arrow-datafusion] ursabot commented on pull request #3612: Simplify serialization by removing redundant `PrimitiveScalarValue`
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #3612:
URL: https://github.com/apache/arrow-datafusion/pull/3612#issuecomment-1290951272
Benchmark runs are scheduled for baseline = 48f73c6af3b0cc747c38b4a9c7a610f4630e8736 and contender = 54d2870a56d8d8f914a617a7e2d52e387ef5dba2. 54d2870a56d8d8f914a617a7e2d52e387ef5dba2 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/1bb6ff45a8e24745a2eab3c5565c6478...16cf17703c534b61b1655c3cc88a11fa/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/fd4acce4a6164acaa45e1630b7be03dd...18762beb4be04143b499ce4a4fb4ffc7/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/ef7b21917cc24e40ac2ff47d535be20b...3211b76e8371424a99095439a6e16deb/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/0642a51cee9240f09e01c2ab40ad0efb...f57466b63e3e42f1a5dc75e182aa2022/)
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
--
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
[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #3612: Simplify serialization by removing redundant `PrimitiveScalarValue`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #3612:
URL: https://github.com/apache/arrow-datafusion/pull/3612#discussion_r1002816676
##########
datafusion/proto/proto/datafusion.proto:
##########
@@ -762,8 +762,9 @@ message StructValue {
message ScalarValue{
oneof value {
- // Null value of any type (type is encoded)
- PrimitiveScalarType null_value = 19;
+ // was PrimitiveScalarType null_value = 19;
Review Comment:
Technically you should `reserved 19`
--
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #3612: Simplify serialization by removing redundant `PrimitiveScalarValue`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #3612:
URL: https://github.com/apache/arrow-datafusion/pull/3612#discussion_r1002693944
##########
datafusion/proto/src/from_proto.rs:
##########
@@ -1359,184 +1246,6 @@ fn vec_to_array<T, const N: usize>(v: Vec<T>) -> [T; N] {
})
}
-//Does not typecheck lists
-fn typechecked_scalar_value_conversion(
Review Comment:
this is all redundant with code that (now) exists in ScalarValue
--
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
[GitHub] [arrow-datafusion] alamb merged pull request #3612: Simplify serialization by removing redundant `PrimitiveScalarValue`
Posted by GitBox <gi...@apache.org>.
alamb merged PR #3612:
URL: https://github.com/apache/arrow-datafusion/pull/3612
--
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #3612: Simplify serialization by removing redundant `PrimitiveScalarValue`
Posted by 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
[GitHub] [arrow-datafusion] alamb commented on pull request #3612: Simplify serialization by removing redundant `PrimitiveScalarValue`
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #3612:
URL: https://github.com/apache/arrow-datafusion/pull/3612#issuecomment-1289511313
> This looks fine for me, not sure what stability guarantees we provide on this API. Theoretically we should go through a period of supporting both, but I don't know if this is worth the effort
Current answer I believe is "we provide no guarantees" (even though in theory we could provide a backwards compatible APIs). For example https://github.com/apache/arrow-datafusion/pull/3547 was likely not backward compatible but yet did not seem to cause changes.
cc @andygrove and @yahoNanJing I think Ballista is the other major user of this API
--
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #3612: Simplify serialization by removing redundant `PrimitiveScalarValue`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #3612:
URL: https://github.com/apache/arrow-datafusion/pull/3612#discussion_r1003691072
##########
datafusion/proto/proto/datafusion.proto:
##########
@@ -762,8 +762,9 @@ message StructValue {
message ScalarValue{
oneof value {
- // Null value of any type (type is encoded)
- PrimitiveScalarType null_value = 19;
+ // was PrimitiveScalarType null_value = 19;
Review Comment:
in 595c97067
--
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
[GitHub] [arrow-datafusion] alamb commented on pull request #3612: Simplify serialization by removing redundant `PrimitiveScalarValue`
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #3612:
URL: https://github.com/apache/arrow-datafusion/pull/3612#issuecomment-1289521059
I plan to merge this tomorrow unless someone has additional comments or would like more time to review
--
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #3612: Simplify serialization by removing redundant `PrimitiveScalarValue`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #3612:
URL: https://github.com/apache/arrow-datafusion/pull/3612#discussion_r1002693845
##########
datafusion/proto/proto/datafusion.proto:
##########
@@ -803,48 +804,9 @@ message Decimal128{
int64 s = 3;
}
-// Contains all valid datafusion scalar type except for
-// List
-enum PrimitiveScalarType{
Review Comment:
All of these types are already handled in `ArrowType`
--
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
[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #3612: Simplify serialization by removing redundant `PrimitiveScalarValue`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #3612:
URL: https://github.com/apache/arrow-datafusion/pull/3612#discussion_r1002816676
##########
datafusion/proto/proto/datafusion.proto:
##########
@@ -762,8 +762,9 @@ message StructValue {
message ScalarValue{
oneof value {
- // Null value of any type (type is encoded)
- PrimitiveScalarType null_value = 19;
+ // was PrimitiveScalarType null_value = 19;
Review Comment:
Technically you should `reserve 19`
--
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
[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #3612: Simplify serialization by removing redundant `PrimitiveScalarValue`
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #3612:
URL: https://github.com/apache/arrow-datafusion/pull/3612#discussion_r979434005
##########
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:
This might be a dumb question but why are nulls typed? This encoding of scalarvalue seems to conflate encoding the schema with encoding the values, which seems unfortunate.
Perhaps we could take a look at what substrait does and copy that?
--
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