Upgraded rand 0.4 -> 0.5 #167
Conversation
There was a problem hiding this comment.
Thanks for the patch. parquet-rs uses rand 0.4.3 and the issue could have been a changed behaviour in rust nightly - parquet-rs 0.3.0 was building okay with rust 1.28. It sounds strange that your project having dependency on rand 0.5 would affect parquet-rs. Anyway, I left some minor questions and suggestions. Thanks again!
src/encodings/rle.rs
Outdated
| @@ -497,7 +497,8 @@ impl RleDecoder { | |||
| #[cfg(test)] | |||
| mod tests { | |||
| use super::*; | |||
| use rand::{self, thread_rng, Rng, SeedableRng}; | |||
| use rand::{self, thread_rng, Rng, SeedableRng }; | |||
src/encodings/rle.rs
Outdated
| let seed = rng.gen_iter::<usize>().take(seed_len).collect::<Vec<usize>>(); | ||
| let mut gen = rand::StdRng::from_seed(&seed[..]); | ||
| let seed_vec : Vec<u8> = Standard.sample_iter(&mut rng).take(seed_len).collect(); | ||
| let seed_slice = &seed_vec[0..32]; |
There was a problem hiding this comment.
You might want to use seed_len, or just pass slice directly into copy_from_slice.
src/util/bit_util.rs
Outdated
| @@ -22,6 +22,9 @@ use errors::{ParquetError, Result}; | |||
| use util::bit_packing::unpack32; | |||
| use util::memory::ByteBufferPtr; | |||
|
|
|||
| #[cfg(test)] | |||
| use rand::distributions::{ Distribution, Standard }; | |||
There was a problem hiding this comment.
Why can't you move that under mod tests?
src/lib.rs
Outdated
| @@ -142,7 +142,6 @@ extern crate lz4; | |||
| extern crate num_bigint; | |||
| extern crate zstd; | |||
|
|
|||
| #[cfg(test)] | |||
| @@ -767,7 +768,7 @@ mod tests { | |||
|
|
|||
| #[test] | |||
| fn test_random() { | |||
| let seed_len = 10; | |||
There was a problem hiding this comment.
Why do you need to change from 10 to 32?
There was a problem hiding this comment.
Good question, I went with 32 because I thought I needed 32 u8's to seed the PRNG. Again, I'm not very familiar with the library, but here's the error I see with an array of size 10:
Compiling parquet v0.3.0 (/Users/xlange/code/rust/parquet-rs)
error[E0308]: mismatched types
--> src/encodings/rle.rs:783:45
|
783 | let mut gen = rand::StdRng::from_seed(seed);
| ^^^^ expected an array with a fixed size of 32 elements, found one with 10 elements
|
= note: expected type `[u8; 32]`
found type `[u8; 10]`
|
Hey, thanks for the thorough review. All great feedback, I was definitely a little sloppy while hacking on this. I agree, it doesn't make sense that just having a mix of rand 0.4 and rand 0.5 in a project would cause an issue, but the type errors made me think it was a mix up. In any case, maybe this is just helpful in general to the project. |
xrl
force-pushed
the
rand-update
branch
2 times, most recently
from
e36ee73
to
f33fc63
Compare
I was starting a new project with dependencies that dragged in rand 0.5. This was breaking parquet-rs. I don't know the rand library very well but it looks like there has been a change for how the Standard/Distribution work and it creeped in to parquet-rs.
Should fix: #166
And should answer my question at: rust-random/rand#626