ARROW-11345: [Rust] Made most ops not rely on value(i)#9291
ARROW-11345: [Rust] Made most ops not rely on value(i)#9291jorgecarleitao wants to merge 1 commit intoapache:masterfrom jorgecarleitao:simplify_ops
value(i)#9291Conversation
|
cc @tyrelr . |
rust/arrow/src/array/ord.rs
Outdated
There was a problem hiding this comment.
this will be slower, but at this point we have no guarantee that i and j are within bounds.
Codecov Report
@@ Coverage Diff @@
## master #9291 +/- ##
=======================================
Coverage 81.88% 81.89%
=======================================
Files 215 215
Lines 52988 52972 -16
=======================================
- Hits 43391 43379 -12
+ Misses 9597 9593 -4
Continue to review full report at Codecov.
|
nevi-me
changed the title
value(i)value(i)
nevi-me
left a comment
nevi-me
left a comment
There was a problem hiding this comment.
@jorgecarleitao this needs a rebase
| } | ||
|
|
||
| Ok(Arc::new(b.finish()) as ArrayRef) | ||
| // todo: can be optimized by computing this for the whole values buffer and |
There was a problem hiding this comment.
we can open a JIRA to track this. I think 'unary' can also work (or was this before the cast improvement?)
jorgecarleitao
force-pushed
the
master
branch
from
d4608a9 to
356c300
Compare
|
@jorgecarleitao is this PR something that you plan to clean up and merge? Or should we close this PR as you have switched to working on arrow 2? |
4 participants
Problem
Currently, a lot of our code relies on
PrimitiveArray::value(usize), however, that method isunsafe(but not marked as such!): anyusizelarger than the length of the array allows to read arbitrary memory regions due to the lack of bound checks.This PR:
This PR changes some of our kernels to not rely on it for this operation, replacing by a safe alternative. This PR is expected to affect the performance of the touched kernels, either improving as we already have alternatives to efficiently create an array out of an iterator, or decreasing (when the index bound is indeed unknown).
todo: benchmark