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

Add deriveable support for structs as View keys #106

Closed
ecton opened this issue Nov 28, 2021 · 10 comments · Fixed by #239
Closed

Add deriveable support for structs as View keys #106

ecton opened this issue Nov 28, 2021 · 10 comments · Fixed by #239
Labels
collections Issues impacting collections or views enhancement New feature or request help wanted Extra attention is needed
Milestone

Comments

@ecton
Copy link
Member

ecton commented Nov 28, 2021

This request is a refinement of the idea from #65. View Keys need to be able to be compared on a byte-by-byte basis and resolve order, and the Key trait currently must be manually implemented for any non-integer types.

This should be done as a custom derive. The composite format should be lightweight and focus on the problems that multi-field comparisons might unveil -- e.g., a String key is variable length, and a shorter String should sort lower than a longer string, regardless of the bytes that follow in the next field.

The other thing a custom derive can do is offer the ability to support floating points. As far as I can come up with, there's not a reliable way to encode a floating point number such that it can be memcmp'ed. An attribute could be used to allow multiple strategies for utilizing a floating point key -- e.g., optionally multiply by 10^x then convert to a i64/u64.

The macro needs to be able to have the location of bonsaidb_core specified as an optional attribute. By default, it should use ::bonsaidb::core as the path to bonsaidb_core, but a user should be able to override it based on their usage. E.g., #[view(core = bonsaidb_server::core)].

@ecton ecton added the collections Issues impacting collections or views label Nov 28, 2021
@ecton ecton added this to To do in Khonsu Labs Roadmap via automation Nov 28, 2021
@ecton ecton added this to the v0.1.0 milestone Nov 28, 2021
@ecton
Copy link
Member Author

ecton commented Dec 1, 2021

As of #107, there are now private utility functions in core that handle converting a Key type to bytes in a way that it can be concatenated with other keys in sequence in a way that remains sorted (internally called "composite" keys).

This issue is now purely an exercise in coming up with a well-designed proc macro that utilizes these functions to implement Key for an arbitrary structure or Enum.

After we have built-in support for Enums, num_traits should become an optional dependency and the book should be updated with new examples demonstrating the new macro.

@ecton ecton added the help wanted Extra attention is needed label Dec 1, 2021
@ecton ecton changed the title Add deriveable support for tuples/enums/structs as View keys Add deriveable support for tuples/structs as View keys Dec 14, 2021
@ModProg
Copy link
Collaborator

ModProg commented Jan 20, 2022

Maybe other things could also be derived like DefaultSerialization.

Maybe also Collection, but that would only be sensible for once without views etc. but at least for my use that would be really comftable :D.

#[derive(Collection)]
#[collection_name = ["hoppi", "notification_subscription"]]
// or
#[collection_name = "hoppi.notification_subscription"]

If you want I would look into implementing the macro.

I have this a lot:

impl Collection for SubscriptionInfoTable {
    fn collection_name() -> Result<CollectionName, InvalidNameError> {
        CollectionName::new("hoppi", "notification_subscription")
    }

    fn define_views(_schema: &mut Schematic) -> Result<(), bonsaidb::core::Error> {
        Ok(())
    }
}
impl DefaultSerialization for SubscriptionInfoTable {}

@ecton
Copy link
Member Author

ecton commented Jan 20, 2022

Great suggestion, I've split it into its own issue to be tackled independently.

@ecton ecton added the enhancement New feature or request label Jan 26, 2022
@ModProg
Copy link
Collaborator

ModProg commented Feb 9, 2022

If you write down the api and result, I'll implement the proc-macro.

ecton added a commit that referenced this issue Mar 10, 2022
I realized that for #106, these functions would need to be public.
@ecton ecton changed the title Add deriveable support for tuples/structs as View keys Add deriveable support for structs as View keys Mar 10, 2022
@ecton
Copy link
Member Author

ecton commented Mar 10, 2022

After the last commit, encode_composite_field and decode_composite_field have been exported. Thinking about it this morning, the implementation should be pretty straightforward:

  • For structs, encode each field in sequence using encode_composite_field.
  • For enums, encode the enum variant index as an integer value, and any fields in sequence.

The Key trait implementation should look similar to how [Tuples are implemented](https://github.com/khonsulabs/bonsaidb
/blob/082d9af02cd881a87ade5368c542798e54b8f584/crates/bonsaidb-core/src/key.rs#L386-L420).

I'd imagine the derive syntax to look like this:

#[derive(Key)]
struct MyCompositeKey {
  a: String,
  b: u64,
}

#[derive(Key)]
enum MyEnum {
  A(String),
  B{ id: u64 }
}

Let me know if you're still wanting to tackle this @ModProg -- no rush as always!

@ModProg
Copy link
Collaborator

ModProg commented Mar 10, 2022

After the last commit, encode_composite_field and decode_composite_field have been exported. Thinking about it this morning, the implementation should be pretty straightforward:

  • For structs, encode each field in sequence using encode_composite_field.
  • For enums, encode the enum variant index as an integer value, and any fields in sequence.

The Key trait implementation should look similar to how Tuples are implemented.

Let me know if you're still wanting to tackle this @ModProg -- no rush as always!

makes sense. We need to make sure that it is understood that the ordering is relevant right?

For enums with assigned numbers we could use those I think.

@ecton
Copy link
Member Author

ecton commented Mar 10, 2022

makes sense. We need to make sure that it is understood that the ordering is relevant right?

Yes, I figured I would end up writing that documentation.

For enums with assigned numbers we could use those I think.

Agreed, great idea.

@ModProg ModProg mentioned this issue Apr 1, 2022
@ecton ecton closed this as completed in #239 Apr 9, 2022
Khonsu Labs Roadmap automation moved this from To do to Done Apr 9, 2022
@ModProg
Copy link
Collaborator

ModProg commented Apr 10, 2022

I just reread this, I see one todo that should probably be done asap as it would be breaking is the assigned numbers enums. Shouldn't be a big change, either.

@ModProg
Copy link
Collaborator

ModProg commented Apr 10, 2022

@ecton Would you be fine with these restrictions:

  1. if no #repr() is given u64 is used
  2. only literals are supported
  3. what to do if values outside u64's range are found? Error and request repr(...) or automatically extend to i64/u128...?

@ecton
Copy link
Member Author

ecton commented Apr 10, 2022

Great catch. I had a note to document this use case. By defaulting to u64, we end up with enums that are always 8 bytes wide. I think we should do this:

  1. If no #[repr()] is given, i128 is used, but encoded using ordered_varint::Signed::from(number).
  2. If a repr is given, use its type directly.
  3. Any values given outside of i128 or the chosen repr will return an error.

By encoding the default option using the Signed wrapper, we will encode the number in a way that remains small (number of bytes) for small numbers, but will dynamically grow to support the full range of i128 the further away from 0 a number is. This should work for 99.9% of use cases, as I would be shocked if many people are using u128 variant.

I'm not sure how complex that will make your code -- currently Signed doesn't support math operations, so you'll have to do it as part of calling encode_composite_field. If making an artificial limitation of no u128 support makes the code significantly easier, I would be fine with that limitation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collections Issues impacting collections or views enhancement New feature or request help wanted Extra attention is needed
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants