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/10/24 19:54:03 UTC
[GitHub] [arrow-datafusion] retikulum opened a new pull request, #3943: Implement serialization for ScalarValue::FixedSizeBinary
retikulum opened a new pull request, #3943:
URL: https://github.com/apache/arrow-datafusion/pull/3943
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #3928.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Implement serialization for `ScalarValue::FixedSizeBinary` for communicating with protobuf.
I added `ScalarFixedSizeBinary` as message in .proto file. I am not sure about if it is the correct way.
My other concern is one of the test is `ScalarValue::FixedSizeBinary(0, None)` but what is the length of None?
--
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 #3943: Implement serialization for ScalarValue::FixedSizeBinary
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #3943:
URL: https://github.com/apache/arrow-datafusion/pull/3943#issuecomment-1291873847
> Thanks @alamb for review and sorry for lots of commits. These conflicts are great for improving my git/rust skills :)
Thank you for putting up with it. It takes some getting used to but then it is quite cool
--
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] retikulum commented on a diff in pull request #3943: Implement serialization for ScalarValue::FixedSizeBinary
Posted by GitBox <gi...@apache.org>.
retikulum commented on code in PR #3943:
URL: https://github.com/apache/arrow-datafusion/pull/3943#discussion_r1004767086
##########
datafusion/proto/proto/datafusion.proto:
##########
@@ -760,6 +760,11 @@ message StructValue {
repeated Field fields = 3;
}
+message ScalarFixedSizeBinary{
Review Comment:
Thanks for feedback. While I was looking for similar definitons, I came acrros to this and implement it in this way. https://github.com/apache/arrow-datafusion/blob/48f73c6af3b0cc747c38b4a9c7a610f4630e8736/datafusion/proto/proto/datafusion.proto#L711-L714
However, I am open to fix in a way to comply with preferred code style. If this is the only thing that needs to be changed, I will create a commit.
--
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] isidentical commented on a diff in pull request #3943: Implement serialization for ScalarValue::FixedSizeBinary
Posted by GitBox <gi...@apache.org>.
isidentical commented on code in PR #3943:
URL: https://github.com/apache/arrow-datafusion/pull/3943#discussion_r1004642023
##########
datafusion/proto/proto/datafusion.proto:
##########
@@ -760,6 +760,11 @@ message StructValue {
repeated Field fields = 3;
}
+message ScalarFixedSizeBinary{
Review Comment:
NIT: should we follow the actual definition and put the length as the first field (it shouldn't matter much, except aesthetically but I think it keeps the declaration in here and the struct in scalar value consistent).
--
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 #3943: Implement serialization for ScalarValue::FixedSizeBinary
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #3943:
URL: https://github.com/apache/arrow-datafusion/pull/3943#issuecomment-1292447432
Benchmark runs are scheduled for baseline = 97ff3458db1b1ab57071f395083c88b91b81aa13 and contender = 4e298353c592cccae3cbed20c64051809c4923fd. 4e298353c592cccae3cbed20c64051809c4923fd 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/98fbe47d3120454d92360912a71204c6...50798c6ef2b442d59a90dc694b82f205/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/3e4d23c18db9414bbb1193de27571ad8...37e968a86a6a4a0b81e215e24a6dae5c/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/6a59214307694369837de660d56b7535...957132ff37b142159d8f47bd146276ba/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/ca16a68954ec42a9b2ae793fb26b6e5b...582344bf8ec64fbe929fa0a20147e196/)
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] alamb commented on a diff in pull request #3943: Implement serialization for ScalarValue::FixedSizeBinary
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #3943:
URL: https://github.com/apache/arrow-datafusion/pull/3943#discussion_r1004918870
##########
datafusion/proto/proto/datafusion.proto:
##########
@@ -834,6 +840,7 @@ enum PrimitiveScalarType{
BINARY = 25;
LARGE_BINARY = 26;
+ FIXED_SIZE_BINARY = 29;
Review Comment:
I removed the duplication with PrimitiveScalarType and `ScalarValue` in https://github.com/apache/arrow-datafusion/commit/54d2870a56d8d8f914a617a7e2d52e387ef5dba2 so when you are resolving the merge conflict you should be able to just remove this code
##########
datafusion/proto/src/lib.rs:
##########
@@ -556,6 +556,11 @@ mod roundtrip_tests {
Field::new("a", DataType::Boolean, false),
]),
),
+ ScalarValue::FixedSizeBinary(
+ b"bar".to_vec().len() as i32,
+ Some(b"bar".to_vec()),
+ ),
+ ScalarValue::FixedSizeBinary(0, None),
Review Comment:
I suggest adding a None case for non zero length as well
```suggestion
ScalarValue::FixedSizeBinary(0, None),
ScalarValue::FixedSizeBinary(5, None),
```
##########
datafusion/proto/src/to_proto.rs:
##########
@@ -1064,10 +1064,13 @@ impl TryFrom<&ScalarValue> for protobuf::ScalarValue {
Value::LargeBinaryValue(s.to_owned())
})
}
- scalar::ScalarValue::FixedSizeBinary(_, _) => {
- return Err(Error::General(
- "FixedSizeBinary is not yet implemented".to_owned(),
- ));
+ scalar::ScalarValue::FixedSizeBinary(length, val) => {
Review Comment:
❤️
--
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 #3943: Implement serialization for ScalarValue::FixedSizeBinary
Posted by GitBox <gi...@apache.org>.
alamb merged PR #3943:
URL: https://github.com/apache/arrow-datafusion/pull/3943
--
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] retikulum commented on pull request #3943: Implement serialization for ScalarValue::FixedSizeBinary
Posted by GitBox <gi...@apache.org>.
retikulum commented on PR #3943:
URL: https://github.com/apache/arrow-datafusion/pull/3943#issuecomment-1291130631
Thanks @alamb for review and sorry for lots of commits. These conflicts are great for improving my git/rust skills :)
--
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