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.

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

Generalize UniqueIndex keys #207

Merged
merged 13 commits into from
Jan 4, 2021
Merged

Generalize UniqueIndex keys #207

merged 13 commits into from
Jan 4, 2021

Conversation

maurolacy
Copy link
Contributor

@maurolacy maurolacy commented Dec 22, 2020

Closes #209.

Use generic index keys in UniqueIndex, so we can prefix() and range() over them.

@maurolacy maurolacy requested a review from ethanfrey December 22, 2020 14:42
@maurolacy maurolacy self-assigned this Dec 22, 2020
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I would make the key less opaque now (no need for index_int hidden away... at least for UniqueIndex).

Otherwise, I would add a testcase for composite index. I don't know, maybe another type... where (first_name, last_name) must be unique, but each alone can exist.

John Doe
John Wayne
Maria Doe

are all fine, but adding a second John Doe will return an error.

packages/storage-plus/src/keys.rs Show resolved Hide resolved
packages/storage-plus/src/indexes.rs Show resolved Hide resolved
packages/storage-plus/src/indexes.rs Outdated Show resolved Hide resolved
packages/storage-plus/src/indexed_map.rs Outdated Show resolved Hide resolved
@ethanfrey
Copy link
Member

Also note these storage-plus changes may be api breaking... If any contracts have to be modified when doing so, please label that api breaking in the title (meaning it must be v0.5.0 not v0.4.1)

@maurolacy
Copy link
Contributor Author

maurolacy commented Dec 22, 2020

Also note these storage-plus changes may be api breaking... If any contracts have to be modified when doing so, please label that api breaking in the title (meaning it must be v0.5.0 not v0.4.1)

Good point. This doesn't happen here AFAIK, but it can happen with changes to MultiIndex. Will take it into account in general.

@maurolacy maurolacy marked this pull request as ready for review December 23, 2020 12:21
@maurolacy
Copy link
Contributor Author

maurolacy commented Dec 23, 2020

prefix() and range() now work as expected. But, they are the "canonical" impl. That is, they require Prefix to be over the value type, i.e. Prefix<T>.

Given that range() is a method of Prefix<T>, there isn't much else that can be done here. That means that, on one side, I had to make the value type used in the UniqueIndex map (called UniqueRef) public. On the other, I cannot change the structure / format of these returned values; because range() is outside my control.

I'm thinking that a good way forward could be to create a MappedPrefix type, which can build prefixes over arbitrary value types, but then accepts a mapping function as a parameter, in order to transform these arbitrary values to our desired types during ranging / iteration. That way, we can specify generic types (i.e. our desired <T>) as outputs, and use indirections internally; whose values are then "magically" mapped to <T>.

Something like you're already doing in pks(), items(), etc., but generic / parametric. And, contained in its own type.

@maurolacy
Copy link
Contributor Author

maurolacy commented Dec 23, 2020

I'm thinking that a good way forward could be to create a MappedPrefix type

Done in the latest commits. Not with a new MappedPrefix type, but by simply generalizing Prefix's deserialization function.

@maurolacy maurolacy force-pushed the unique-index-generic-key branch from c58ef15 to ee9e147 Compare December 27, 2020 10:14
@maurolacy
Copy link
Contributor Author

These commits are mostly a rebase from master.

I would like to merge this, as I think it's stable.

@maurolacy maurolacy merged commit ffd92a0 into master Jan 4, 2021
@maurolacy maurolacy deleted the unique-index-generic-key branch January 4, 2021 09:03
Prefix::new_de_fn(top_name, sub_names, deserialize_kv)
}

pub fn new_de_fn(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use a different name, but I see this extension, and good to keep the default behavior the same.

Definitely need more time to review this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was struggling when naming it.

Maybe new_custom_de?
Another option would be to keep only new, and add a helper method set_de_fn or so. But I think it's more confusing / unclear.

Copy link
Contributor Author

@maurolacy maurolacy Jan 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take your time to review this. It's a more complex variant of the unique index case, but in the end it's the same mechanism: when creating the Prefix, a custom deserialization function is passed, which handles the indirection.

I couldn't make it simpler, because of the different maps / namespaces, and the need to access the store.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant, the related PR for the multi index.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support composite keys on secondary indexes (unique-index)
2 participants