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

range_de for IndexMap #498

Merged
merged 6 commits into from
Oct 20, 2021
Merged

range_de for IndexMap #498

merged 6 commits into from
Oct 20, 2021

Conversation

uint
Copy link
Contributor

@uint uint commented Oct 18, 2021

Part of #461

@uint
Copy link
Contributor Author

uint commented Oct 19, 2021

@maurolacy Any idea what would be a good way to test prefixed_range_de here?

@maurolacy
Copy link
Contributor

maurolacy commented Oct 19, 2021

A multi index that has a triple key would be good. That way you can confirm deserialization of the remaining of the key works. Take a look at the name_age multi index, in tests. Any other multi index will work too, because they always have (at least) a tuple as key. Perhaps simpler to check one of those first.

And, perhaps better to start with UniqueIndex. Again, a unique index that has a tuple as key would be good. Like name_lastname.

So, I would say:

  • Test a UniqueIndex with a tuple as key. You should get the 2nd element (when using prefix_de / prefix_range_de) deserialized.
  • Test a MultiIndex with a tuple as key. You should get the deserialization of the pk (when using prefix_de / prefix_range_de).
  • Test a MultiIndex with a triple as key. You should get a tuple which is the deserialization of the 2nd element, along with the pk (3rd) (when using sub_prefix / sub_prefix_range).

Notice that for this triples test you'll need a couple of sub_prefix_de / sub_prefix_range_de impls.

Finally, for completeness:

  • Test a MultiIndex with a triple as key, but using prefix_de / prefix_range_de. You should get the deserialization of the pk (3rd element).

Hope that makes sense. Just ping me if you need further clarification.

@uint uint force-pushed the 461-indexedmap-range_de branch from f226273 to 7391742 Compare October 20, 2021 06:14
@uint
Copy link
Contributor Author

uint commented Oct 20, 2021

I'm going to pull the index stuff into a separate PR since Ethan asked for small PRs.

@uint uint requested review from ethanfrey and maurolacy October 20, 2021 06:16
@uint uint marked this pull request as ready for review October 20, 2021 06:16
@maurolacy
Copy link
Contributor

maurolacy commented Oct 20, 2021

I realize now my comments above are not totally correct. For unique indexes, when ranging / prefixing / sub-prefixing over the indexes of a UniqueIndex, you will get a range of the values, and their primary keys. So, it would be good to test key deserialization over tuples, etc. but that will involve a deserialization of the pk, not the unique index key.

For multi-indexes is different: There you'll get as key a tuple (or triple) of the (remaining) elements of the index key, and the corresponding pk. In this case, this format matches the index key; because the pk is being added to it.

We could change that: by example, by returning only the (deserialized) pk. But, keeping with the current convention is better in terms of compatibility / consistency. Having the remaining elements of the index key may also be useful, to do extra filtering / ranging, by example.

Finally: We could modify / extend UniqueIndex to also return both things: the (remaining) elements of the index key, and the primary key. This would be good for compatibility with MultiIndex, but it will break UniqueIndex's current contract. I wouldn't do this, at least in this iteration.

@uint
Copy link
Contributor Author

uint commented Oct 20, 2021

I realize now my comments above are not totally correct. For unique indexes, when ranging / prefixing / sub-prefixing over the indexes of a UniqueIndex, you will get a range of the values, and their primary keys. So, it would be good to test key deserialization over tuples, etc. but that will involve a deserialization of the pk, not the unique index key.

That's what had me confused. I assumed you meant to suggest we implement range_de and friends for indexes first, but then I figured that would be better in a separate PR, so I pulled it out there (#500). I also already added the tests for the deserialization of the PK in this PR - does that look good?

For multi-indexes is different: There you'll get as key a tuple (or triple) of the remaining elements of the index key, and the corresponding pk. In this case, this format matches the index key; because the pk is being added to it.

Ahh, so that makes it a bit tricky. Gotcha.

maurolacy
maurolacy previously approved these changes Oct 20, 2021
Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

Base Map impl.

Good to merge. It would be good to improve / clarify tests first.

packages/storage-plus/src/indexed_map.rs Show resolved Hide resolved
@uint uint requested a review from maurolacy October 20, 2021 15:46
Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

Nice.

@uint uint merged commit 43eaa80 into main Oct 20, 2021
@uint uint deleted the 461-indexedmap-range_de branch October 20, 2021 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants