fix(array): inconsistent hash behavior of Datum and Array::Item
#3458
Conversation
Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Codecov Report
@@ Coverage Diff @@
## main #3458 +/- ##
=======================================
Coverage 74.30% 74.31%
=======================================
Files 769 769
Lines 106775 106773 -2
=======================================
+ Hits 79338 79346 +8
+ Misses 27437 27427 -10
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
| Some(datum) => datum.hash(&mut hasher), | ||
| None => bail!("index {} out of row bound", idx), | ||
| } | ||
| hash_datum(&self.0[*idx], &mut hasher); |
There was a problem hiding this comment.
What if idx is greater than the length of the row? 🤔
There was a problem hiding this comment.
Indexing with [..] will panic if out of bound.
There was a problem hiding this comment.
Should we panic here? Or should we return an error?
There was a problem hiding this comment.
I think panicking here is okay since the indices are inferred or specified by some other components most of the time. If this fails, there must be a bug in our system and we're unable to recover it.
BugenZhao
added
the
mergify/can-merge
I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.
What's changed and what's your intention?
As title. This is a fix for common types except for array and struct. Check #3457 for more details.
Checklist
./risedev check(or alias,./risedev c)Refer to a related PR or issue link (optional)
Hashimplementation onDatumis not consistent withArray::hash_at#3457