ARROW-11265: [Rust] Made bool not ArrowNativeType #9212

jorgecarleitao · 2021-01-16T06:13:24Z

This PR removes the risk of boolean values to be converted to bytes via ToByteSlice by explicitly making ArrowNativeType be only used in types whose in-memory representation in Rust equates to the in-memory representation in Arrow. bool in Rust is a byte and in Arrow it is a bit.

Overall, the direction of this PR is to have the traits represent one aspect of the type. In this case, ArrowNativeType is currently

a type that has the same in memory representation (ToByteSlice is implemented for it)
a json serializable type
something that can be casted to/from usize.

This poses a problem because:

bools are serializable, not castable to usize, have different memory representation
fixed size (iX, uX) are serializable, castable to usize, have the same memory representation
fixed floating (f32, f64) are serializable, not castable to usize, have the same memory representation

however, they all implement ArrowNativeType.

This PR focus on splitting the json-serializable part of it.

github-actions · 2021-01-16T06:14:06Z

https://issues.apache.org/jira/browse/ARROW-11265

codecov-io · 2021-01-16T06:35:31Z

Codecov Report

Merging #9212 (1cd33d0) into master (1393188) will increase coverage by 0.00%.
The diff coverage is 80.00%.

@@           Coverage Diff           @@
##           master    #9212   +/-   ##
=======================================
  Coverage   81.61%   81.61%           
=======================================
  Files         215      215           
  Lines       51867    51866    -1     
=======================================
  Hits        42329    42329           
+ Misses       9538     9537    -1

Impacted Files	Coverage Δ
rust/arrow/src/util/integration_util.rs	`66.87% <0.00%> (+0.14%)`	⬆️
rust/arrow/src/compute/kernels/sort.rs	`93.56% <100.00%> (ø)`
rust/arrow/src/datatypes.rs	`78.47% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eaa7b7a...1cd33d0. Read the comment docs.

nevi-me · 2021-01-16T13:29:28Z

rust/arrow/src/util/integration_util.rs

@@ -449,12 +449,10 @@ impl ArrowJsonBatch {
                    for i in 0..col.len() {
                        if col.is_null(i) {
                            validity.push(1);


This is unrelated, but unless I'm reading this incorrectly (likely am), I would have thought that we'd validity.push(0) here for null values.

nevi-me

Thanks for the detailed explanation @jorgecarleitao

alamb · 2021-01-17T11:12:31Z

I apologize for the delay in merging Rust PRs -- the 3.0 release is being finalized now and are planning to minimize entropy by postponing merging changes not critical for the release until the process was complete. I hope the process is complete in the next few days. There is more discussion in the mailing list

jhorstmann · 2021-01-17T22:50:57Z

I did not see this, but had the same idea at around the same time :) This matches now also the distinction between PrimitiveArray and BooleanArray.

Another motivation for this change is that this could make Buffer::typed_data safe and allow us to remove the num::Num trait bound from that method.

jhorstmann · 2021-01-17T22:55:25Z

rust/arrow/src/datatypes.rs


+/// Trait expressing a Rust type that has the same in-memory representation
+/// as Arrow. This includes `i16`, `f32`, but excludes `bool` (which in arrow is represented in bits).
+/// In little endian machines, types that implement [`ArrowNativeType`] can be memcopied to arrow buffers


Does the in-memory format require little-endian? I'd expect conversion to happen when reading data into memory, which should make memcpy work on bot LE and BE machines. Assuming absence of other endianness bugs of course.

It does not require it but is little-endian by default.

I recall someone wanting to add big-endian support to the C++ impl.

paddyhoran

LGTM, this has always been a source of corner cases (bools being bitpacked) so nice to clean it up.

alamb

Thank you @jorgecarleitao

alamb · 2021-01-18T11:17:11Z

rust/arrow/src/datatypes.rs

+/// In little endian machines, types that implement [`ArrowNativeType`] can be memcopied to arrow buffers
+/// as is.
+pub trait ArrowNativeType:
+    fmt::Debug + Send + Sync + Copy + PartialOrd + FromStr + Default + JsonSerializable


This is a nice approach (breaking out the JsonSerializable part) 👍

alamb · 2021-01-20T11:59:14Z

I merged this branch locally into master and re-ran the tests. Things looked good.

This PR removes the risk of boolean values to be converted to bytes via `ToByteSlice` by explicitly making `ArrowNativeType` be only used in types whose in-memory representation in Rust equates to the in-memory representation in Arrow. `bool` in Rust is a byte and in Arrow it is a bit. Overall, the direction of this PR is to have the traits represent one aspect of the type. In this case, `ArrowNativeType` is currently * a type that has the same in memory representation (ToByteSlice is implemented for it) * a json serializable type * something that can be casted to/from `usize`. This poses a problem because: 1. bools are serializable, not castable to usize, have different memory representation 2. fixed size (iX, uX) are serializable, castable to usize, have the same memory representation 3. fixed floating (f32, f64) are serializable, not castable to usize, have the same memory representation however, they all implement `ArrowNativeType`. This PR focus on splitting the json-serializable part of it. Closes #9212 from jorgecarleitao/fix_trait Authored-by: Jorge C. Leitao <jorgecarleitao@gmail.com> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>

This PR removes the risk of boolean values to be converted to bytes via `ToByteSlice` by explicitly making `ArrowNativeType` be only used in types whose in-memory representation in Rust equates to the in-memory representation in Arrow. `bool` in Rust is a byte and in Arrow it is a bit. Overall, the direction of this PR is to have the traits represent one aspect of the type. In this case, `ArrowNativeType` is currently * a type that has the same in memory representation (ToByteSlice is implemented for it) * a json serializable type * something that can be casted to/from `usize`. This poses a problem because: 1. bools are serializable, not castable to usize, have different memory representation 2. fixed size (iX, uX) are serializable, castable to usize, have the same memory representation 3. fixed floating (f32, f64) are serializable, not castable to usize, have the same memory representation however, they all implement `ArrowNativeType`. This PR focus on splitting the json-serializable part of it. Closes apache#9212 from jorgecarleitao/fix_trait Authored-by: Jorge C. Leitao <jorgecarleitao@gmail.com> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>

Made bool not ArrowNativeType.

1cd33d0

github-actions bot added the Component: Rust label Jan 16, 2021

nevi-me self-requested a review January 16, 2021 13:17

nevi-me reviewed Jan 16, 2021

View reviewed changes

nevi-me approved these changes Jan 16, 2021

View reviewed changes

jhorstmann reviewed Jan 17, 2021

View reviewed changes

paddyhoran approved these changes Jan 18, 2021

View reviewed changes

alamb reviewed Jan 18, 2021

View reviewed changes

alamb closed this in 01c5aec Jan 20, 2021

asfimport mentioned this pull request Jan 20, 2021

[Rust] Made bool not convertable to bytes #27166

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-11265: [Rust] Made bool not ArrowNativeType #9212

ARROW-11265: [Rust] Made bool not ArrowNativeType #9212

jorgecarleitao commented Jan 16, 2021 •

edited

Loading

github-actions bot commented Jan 16, 2021

codecov-io commented Jan 16, 2021

nevi-me Jan 16, 2021

nevi-me left a comment

alamb commented Jan 17, 2021

jhorstmann commented Jan 17, 2021 •

edited

Loading

jhorstmann Jan 17, 2021

paddyhoran Jan 18, 2021

paddyhoran left a comment

alamb left a comment

alamb Jan 18, 2021

alamb commented Jan 20, 2021

ARROW-11265: [Rust] Made bool not ArrowNativeType #9212

ARROW-11265: [Rust] Made bool not ArrowNativeType #9212

Conversation

jorgecarleitao commented Jan 16, 2021 • edited Loading

github-actions bot commented Jan 16, 2021

codecov-io commented Jan 16, 2021

Codecov Report

nevi-me Jan 16, 2021

Choose a reason for hiding this comment

nevi-me left a comment

Choose a reason for hiding this comment

alamb commented Jan 17, 2021

jhorstmann commented Jan 17, 2021 • edited Loading

jhorstmann Jan 17, 2021

Choose a reason for hiding this comment

paddyhoran Jan 18, 2021

Choose a reason for hiding this comment

paddyhoran left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Jan 18, 2021

Choose a reason for hiding this comment

alamb commented Jan 20, 2021

jorgecarleitao commented Jan 16, 2021 •

edited

Loading

jhorstmann commented Jan 17, 2021 •

edited

Loading