WASM group by 32bit/64bit conversion bugfix #17793
Conversation
crates/polars-row/src/row.rs
Outdated
| @@ -87,8 +93,13 @@ impl RowsEncoded { | |||
|
|
|||
| unsafe { | |||
| let (_, values, _) = mmap::slice(&self.values).into_inner(); | |||
| let offsets = bytemuck::cast_slice::<usize, i64>(self.offsets.as_slice()); | |||
| let (_, offsets, _) = mmap::slice(offsets).into_inner(); | |||
| let offsets = match bytemuck::try_cast_slice::<usize, i64>(self.offsets.as_slice()) { | |||
There was a problem hiding this comment.
I think we should then just keep a Vec<u64> instead of a Vec<usize>.
There was a problem hiding this comment.
Hi @ritchie46 , I updated the code, I am not all too happy about it because I introduced lots of casting from u64 to usize and vice versa. Imo there is more significant refactoring needed to reduce the casting - by abstracting the RowsEncoded such that the other structs are no longer able to directly write on the offset vector. If you agree with that, I can do it, let me know.
Also, on more point, the WASM build also fails on utf8_to module. I misled the compiler, but it isn't a real fix. Please take a look at that part as well.
daBlesr
force-pushed
the
bugfix/wasm-group-by
branch
from
68375d2
to
d38dd53
Compare
|
Hi @ritchie46 , would you mind taking another look at the PR? Much appreciated 😄 . |
|
Hi, @ritchie46, is there any reason why this PR has not yet been merged? Is it sufficient to update it to its current state? |
|
Ci was never green. It was never in a state for me to consider it merging. |
# Conflicts: # Cargo.lock # crates/polars-row/src/encode.rs # crates/polars-row/src/fixed.rs # crates/polars-row/src/row.rs # crates/polars-row/src/variable.rs
On WASB,
group_bybreaks becauseusizeis by default 32 bit. For encoding rows toi64type, elements need only be casted, but on WASM it also needs creation of new vector.Fixes: #17192