Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Index keys consistency #568

Merged
merged 7 commits into from
Dec 7, 2021
Merged

Index keys consistency #568

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f65f0cb
Fix asserts order for clarity
maurolacy Dec 3, 2021
c4cbb65
Remove/change FIXMEs
maurolacy Dec 3, 2021
75c07e4
Fix: `UniqueIndex` `prefix_range_de` prefix spec
maurolacy Dec 3, 2021
d1677bd
Introduce `MultiIndex` generic type to signal pk deserialization, lik…
maurolacy Dec 3, 2021
4c12bca
Remove redundant &str type spec in tests
maurolacy Dec 6, 2021
cd4db6b
Rename phantom data field for clarity
maurolacy Dec 6, 2021
11c3e6f
Improve docs / comments
maurolacy Dec 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 16 additions & 16 deletions packages/storage-plus/src/indexed_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ mod test {

struct DataIndexes<'a> {
// Second arg is for storing pk
pub name: MultiIndex<'a, (String, String), Data>,
pub name: MultiIndex<'a, (String, String), Data, String>,
pub age: UniqueIndex<'a, u32, Data, String>,
pub name_lastname: UniqueIndex<'a, (Vec<u8>, Vec<u8>), Data, String>,
}
Expand All @@ -312,7 +312,7 @@ mod test {
// For composite multi index tests
struct DataCompositeMultiIndex<'a> {
// Third arg needed for storing pk
pub name_age: MultiIndex<'a, (Vec<u8>, u32, Vec<u8>), Data>,
pub name_age: MultiIndex<'a, (Vec<u8>, u32, Vec<u8>), Data, String>,
}

// Future Note: this can likely be macro-derived
Expand Down Expand Up @@ -722,31 +722,31 @@ mod test {
last_name: "".to_string(),
age: 42,
};
let pk1: &[u8] = b"5627";
let pk1: &str = "5627";
maurolacy marked this conversation as resolved.
Show resolved Hide resolved
map.save(&mut store, pk1, &data1).unwrap();

let data2 = Data {
name: "Juan".to_string(),
last_name: "Perez".to_string(),
age: 13,
};
let pk2: &[u8] = b"5628";
let pk2: &str = "5628";
map.save(&mut store, pk2, &data2).unwrap();

let data3 = Data {
name: "Maria".to_string(),
last_name: "Young".to_string(),
age: 24,
};
let pk3: &[u8] = b"5629";
let pk3: &str = "5629";
map.save(&mut store, pk3, &data3).unwrap();

let data4 = Data {
name: "Maria Luisa".to_string(),
last_name: "Bemberg".to_string(),
age: 43,
};
let pk4: &[u8] = b"5630";
let pk4: &str = "5630";
map.save(&mut store, pk4, &data4).unwrap();

let marias: Vec<_> = map
Expand All @@ -760,8 +760,8 @@ mod test {
assert_eq!(2, count);

// Remaining part (age) of the index keys, plus pks (bytes) (sorted by age descending)
assert_eq!((42, pk1.to_vec()), marias[0].0);
assert_eq!((24, pk3.to_vec()), marias[1].0);
assert_eq!(pk1, marias[0].0);
assert_eq!(pk3, marias[1].0);

// Data
assert_eq!(data1, marias[0].1);
Expand Down Expand Up @@ -894,18 +894,18 @@ mod test {
assert_eq!(5, count);

// The pks, sorted by age ascending
assert_eq!(pks[4], String::from_slice(&ages[4].0).unwrap());
assert_eq!(pks[3], String::from_slice(&ages[0].0).unwrap());
assert_eq!(pks[1], String::from_slice(&ages[1].0).unwrap());
assert_eq!(pks[2], String::from_slice(&ages[2].0).unwrap());
assert_eq!(pks[0], String::from_slice(&ages[3].0).unwrap());
assert_eq!(pks[3], String::from_slice(&ages[0].0).unwrap()); // 12
assert_eq!(pks[1], String::from_slice(&ages[1].0).unwrap()); // 23
assert_eq!(pks[2], String::from_slice(&ages[2].0).unwrap()); // 32
assert_eq!(pks[0], String::from_slice(&ages[3].0).unwrap()); // 42
assert_eq!(pks[4], String::from_slice(&ages[4].0).unwrap()); // 90

// The associated data
assert_eq!(datas[4], ages[4].1);
assert_eq!(datas[3], ages[0].1);
assert_eq!(datas[1], ages[1].1);
assert_eq!(datas[2], ages[2].1);
assert_eq!(datas[0], ages[3].1);
assert_eq!(datas[4], ages[4].1);
}

#[test]
Expand All @@ -927,18 +927,18 @@ mod test {
assert_eq!(5, count);

// The pks, sorted by age ascending
assert_eq!(pks[4], ages[4].0);
assert_eq!(pks[3], ages[0].0);
assert_eq!(pks[1], ages[1].0);
assert_eq!(pks[2], ages[2].0);
assert_eq!(pks[0], ages[3].0);
assert_eq!(pks[4], ages[4].0);

// The associated data
assert_eq!(datas[4], ages[4].1);
assert_eq!(datas[3], ages[0].1);
assert_eq!(datas[1], ages[1].1);
assert_eq!(datas[2], ages[2].1);
assert_eq!(datas[0], ages[3].1);
assert_eq!(datas[4], ages[4].1);
}

#[test]
Expand Down
8 changes: 4 additions & 4 deletions packages/storage-plus/src/indexed_snapshot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ mod test {

struct DataIndexes<'a> {
// Second arg is for storing pk
pub name: MultiIndex<'a, (Vec<u8>, String), Data>,
pub name: MultiIndex<'a, (Vec<u8>, String), Data, String>,
// Last generic type arg is pk deserialization type
pub age: UniqueIndex<'a, u32, Data, String>,
// Last generic type arg is pk deserialization type
Expand All @@ -330,7 +330,7 @@ mod test {
// For composite multi index tests
struct DataCompositeMultiIndex<'a> {
// Third arg needed for storing pk
pub name_age: MultiIndex<'a, (Vec<u8>, u32, Vec<u8>), Data>,
pub name_age: MultiIndex<'a, (Vec<u8>, u32, Vec<u8>), Data, String>,
}

// Future Note: this can likely be macro-derived
Expand Down Expand Up @@ -752,8 +752,8 @@ mod test {
assert_eq!(2, count);

// Pks (sorted by age descending)
assert_eq!((42, pk1.as_bytes().to_owned()), marias[0].0);
assert_eq!((24, pk3.as_bytes().to_owned()), marias[1].0);
assert_eq!(pk1.to_string(), marias[0].0);
assert_eq!(pk3.to_string(), marias[1].0);

// Data
assert_eq!(data1, marias[0].1);
Expand Down
57 changes: 32 additions & 25 deletions packages/storage-plus/src/indexes/multi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use crate::iter_helpers::deserialize_kv;
use crate::map::Map;
use crate::prefix::{namespaced_prefix_range, PrefixBound};
use crate::{Bound, Index, Prefix, Prefixer, PrimaryKey};
use std::marker::PhantomData;

/// MultiIndex stores (namespace, index_name, idx_value, pk) -> b"pk_len".
/// Allows many values per index, and references pk.
Expand All @@ -24,14 +25,16 @@ use crate::{Bound, Index, Prefix, Prefixer, PrimaryKey};
/// The MultiIndex definition must include a field for the pk. That is, the MultiIndex K value
/// is always a n-tuple (n >= 2) and its last element must be the pk.
/// The index function must therefore put the pk as last element, when generating the index.
pub struct MultiIndex<'a, K, T> {
/// The optional PK type defines the type of Primary Key deserialization.
pub struct MultiIndex<'a, K, T, PK = ()> {
index: fn(&T, Vec<u8>) -> K,
idx_namespace: &'a [u8],
idx_map: Map<'a, K, u32>,
pk_namespace: &'a [u8],
_phantom: PhantomData<PK>,
maurolacy marked this conversation as resolved.
Show resolved Hide resolved
}

impl<'a, K, T> MultiIndex<'a, K, T>
impl<'a, K, T, PK> MultiIndex<'a, K, T, PK>
where
T: Serialize + DeserializeOwned + Clone,
{
Expand All @@ -54,7 +57,7 @@ where
/// pub age: u32,
/// }
///
/// MultiIndex::new(
/// let index: MultiIndex<_, _, String> = MultiIndex::new(
/// |d: &Data, k: Vec<u8>| (d.age, k),
/// "age",
/// "age__owner",
Expand All @@ -70,6 +73,7 @@ where
idx_namespace: idx_namespace.as_bytes(),
idx_map: Map::new(idx_namespace),
pk_namespace: pk_namespace.as_bytes(),
_phantom: PhantomData,
}
}
}
Expand All @@ -95,7 +99,6 @@ fn deserialize_multi_v<T: DeserializeOwned>(
.ok_or_else(|| StdError::generic_err("pk not found"))?;
let v = from_slice::<T>(&v)?;

// FIXME: Return `key` here instead of `pk` (be consistent with `deserialize_multi_kv` and `Map` behaviour)
Ok((pk.to_vec(), v))
}

Expand All @@ -120,10 +123,11 @@ fn deserialize_multi_kv<K: KeyDeserialize, T: DeserializeOwned>(
.ok_or_else(|| StdError::generic_err("pk not found"))?;
let v = from_slice::<T>(&v)?;

Ok((K::from_vec(key)?, v))
// We return deserialized `pk` here for consistency
Ok((K::from_slice(pk)?, v))
}

impl<'a, K, T> Index<T> for MultiIndex<'a, K, T>
impl<'a, K, T, PK> Index<T> for MultiIndex<'a, K, T, PK>
where
T: Serialize + DeserializeOwned + Clone,
K: PrimaryKey<'a>,
Expand All @@ -140,7 +144,7 @@ where
}
}

impl<'a, K, T> MultiIndex<'a, K, T>
impl<'a, K, T, PK> MultiIndex<'a, K, T, PK>
where
T: Serialize + DeserializeOwned + Clone,
K: PrimaryKey<'a>,
Expand Down Expand Up @@ -198,7 +202,7 @@ where
}

// short-cut for simple keys, rather than .prefix(()).range(...)
impl<'a, K, T> MultiIndex<'a, K, T>
impl<'a, K, T, PK> MultiIndex<'a, K, T, PK>
where
T: Serialize + DeserializeOwned + Clone,
K: PrimaryKey<'a>,
Expand Down Expand Up @@ -228,8 +232,8 @@ where
self.no_prefix().keys(store, min, max, order)
}

/// While `range_de` over a `prefix_de` fixes the prefix to one element and iterates over the
/// remaining, `prefix_range_de` accepts bounds for the lowest and highest elements of the
/// While `range` over a `prefix` fixes the prefix to one element and iterates over the
/// remaining, `prefix_range` accepts bounds for the lowest and highest elements of the
/// `Prefix` itself, and iterates over those (inclusively or exclusively, depending on
/// `PrefixBound`).
/// There are some issues that distinguish these two, and blindly casting to `Vec<u8>` doesn't
Expand All @@ -252,33 +256,35 @@ where
}

#[cfg(feature = "iterator")]
impl<'a, K, T> MultiIndex<'a, K, T>
impl<'a, K, T, PK> MultiIndex<'a, K, T, PK>
where
PK: PrimaryKey<'a> + KeyDeserialize,
T: Serialize + DeserializeOwned + Clone,
K: PrimaryKey<'a>,
{
pub fn prefix_de(&self, p: K::Prefix) -> Prefix<K::Suffix, T> {
pub fn prefix_de(&self, p: K::Prefix) -> Prefix<PK, T> {
Prefix::with_deserialization_function(
self.idx_namespace,
&p.prefix(),
self.pk_namespace,
deserialize_multi_kv::<K::Suffix, T>,
deserialize_multi_kv::<PK, T>,
)
}

pub fn sub_prefix_de(&self, p: K::SubPrefix) -> Prefix<K::SuperSuffix, T> {
pub fn sub_prefix_de(&self, p: K::SubPrefix) -> Prefix<PK, T> {
Prefix::with_deserialization_function(
self.idx_namespace,
&p.prefix(),
self.pk_namespace,
deserialize_multi_kv::<K::SuperSuffix, T>,
deserialize_multi_kv::<PK, T>,
)
}
}

#[cfg(feature = "iterator")]
impl<'a, K, T> MultiIndex<'a, K, T>
impl<'a, K, T, PK> MultiIndex<'a, K, T, PK>
where
PK: PrimaryKey<'a> + KeyDeserialize,
T: Serialize + DeserializeOwned + Clone,
K: PrimaryKey<'a> + KeyDeserialize,
{
Expand All @@ -294,15 +300,16 @@ where
min: Option<PrefixBound<'a, K::Prefix>>,
max: Option<PrefixBound<'a, K::Prefix>>,
order: cosmwasm_std::Order,
) -> Box<dyn Iterator<Item = StdResult<(K::Output, T)>> + 'c>
) -> Box<dyn Iterator<Item = StdResult<(PK::Output, T)>> + 'c>
where
T: 'c,
'a: 'c,
K: 'c,
K::Output: 'static,
PK: 'c,
PK::Output: 'static,
{
let mapped = namespaced_prefix_range(store, self.idx_namespace, min, max, order)
.map(deserialize_kv::<K, T>);
.map(deserialize_kv::<PK, T>);
Box::new(mapped)
}

Expand All @@ -312,10 +319,10 @@ where
min: Option<Bound>,
max: Option<Bound>,
order: cosmwasm_std::Order,
) -> Box<dyn Iterator<Item = StdResult<(K::Output, T)>> + 'c>
) -> Box<dyn Iterator<Item = StdResult<(PK::Output, T)>> + 'c>
where
T: 'c,
K::Output: 'static,
PK::Output: 'static,
{
self.no_prefix_de().range_de(store, min, max, order)
}
Expand All @@ -326,20 +333,20 @@ where
min: Option<Bound>,
max: Option<Bound>,
order: cosmwasm_std::Order,
) -> Box<dyn Iterator<Item = StdResult<K::Output>> + 'c>
) -> Box<dyn Iterator<Item = StdResult<PK::Output>> + 'c>
where
T: 'c,
K::Output: 'static,
PK::Output: 'static,
{
self.no_prefix_de().keys_de(store, min, max, order)
}

fn no_prefix_de(&self) -> Prefix<K, T> {
fn no_prefix_de(&self) -> Prefix<PK, T> {
Prefix::with_deserialization_function(
self.idx_namespace,
&[],
self.pk_namespace,
deserialize_multi_kv::<K, T>,
deserialize_multi_kv::<PK, T>,
)
}
}
10 changes: 4 additions & 6 deletions packages/storage-plus/src/indexes/unique.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,17 +92,15 @@ where
fn deserialize_unique_v<T: DeserializeOwned>(kv: Record) -> StdResult<Record<T>> {
let (_, v) = kv;
let t = from_slice::<UniqueRef<T>>(&v)?;
// FIXME: Return `k` here instead of `t.pk` (be consistent with `Map` behaviour)
Ok((t.pk.to_vec(), t.value))
Ok((t.pk.0, t.value))
}

fn deserialize_unique_kv<K: KeyDeserialize, T: DeserializeOwned>(
kv: Record,
) -> StdResult<(K::Output, T)> {
let (_, v) = kv;
let t = from_slice::<UniqueRef<T>>(&v)?;
// FIXME: Return `k` deserialization here instead of `t.pk` (be consistent with `deserialize_multi_kv` and `Map` behaviour)
Ok((K::from_vec(t.pk.to_vec())?, t.value))
Ok((K::from_vec(t.pk.0)?, t.value))
}

impl<'a, K, T, PK> UniqueIndex<'a, K, T, PK>
Expand Down Expand Up @@ -190,8 +188,8 @@ where
pub fn prefix_range_de<'c>(
&self,
store: &'c dyn Storage,
min: Option<PrefixBound<'a, PK::Prefix>>,
max: Option<PrefixBound<'a, PK::Prefix>>,
min: Option<PrefixBound<'a, K::Prefix>>,
max: Option<PrefixBound<'a, K::Prefix>>,
order: cosmwasm_std::Order,
) -> Box<dyn Iterator<Item = StdResult<(PK::Output, T)>> + 'c>
where
Expand Down