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

StorageProvider development #91

Closed
wants to merge 131 commits into from
Closed

StorageProvider development #91

wants to merge 131 commits into from

Conversation

sandreae
Copy link
Member

@sandreae sandreae commented Apr 24, 2022

Overview

  • define SQL table layouts for Operationss, DocumentViews andDocuments
  • define and implement storage traits for Operations and DocumentViews.
  • once the traits are defined, they will be moved over to p2panda-rs
  • this PR also contains changes to EntryStore defined here: Entry store additions p2panda#310 which implement replication methods. I wanted to split them out into there own PR but really haven't found a good way to do it.... 😭

References

Todo:

  • Error handling in OperationStore and DocumentStore needs improvement
  • Custom definition of get_next_n_entries_after_seq() in EntryStore as the default implementation will be very inefficient as it would end up with repeated db requests for each entry
  • DocumentStore is the least developed, a few areas which need work spring to mind:
    • return a DocumentView from get_document_view_by_id()
    • implement find_all_related_views()
    • review the SQL table layouts, i see potential in having typed value fields, this way we can write queries which reference the actual field values (could be cool for recursive relation queries). This could be achieved either through having multiple typed value columns in the operation_field_v1 table or by splitting the fields into their own typed tables (as @cafca sketched out)
    • implement any Document methods we need.
  • Review the tests we have, and write some more 👍

Notes

  • Do we not want to use FOREIGN KEY(x) for relations in the SQL tables, as we don't know the order items will be added to the db? for now we avoid using it as we aren't clear of the implications.
  • We said we wanted to use SchemaHashId for a shortened unique id to identify schemas with potentially ever group SchemaIds. However, if an operation follows a system schema, then it already has a short identifier, and so it won't be hashed... Shall we call it sometihng else in the db? Like schema_id_short? Which will be a {name}_{schema_id_hash} in the case of an application schema, and {name}_{version} in the case of a system schema?

Next

  • SchemaStore
  • AuthorStore

📋 Checklist

  • Add tests that cover your changes
  • Add this PR to the Unreleased section in CHANGELOG.md
  • Link this PR to any issues it closes
  • New files contain a SPDX license header

@sandreae sandreae changed the title Support db storage of Operations and DocumentViews Implement OperationStore and DocumentStore May 5, 2022
@adzialocha
Copy link
Member

adzialocha commented May 5, 2022

I've changed the p2panda_rs crate to point against our temporary aquadoggo-wip branch: https://github.com/p2panda/p2panda/tree/aquadoggo-wip (it contains everything which is not reviewed / merged yet but is needed for this PR).

Also I had a quick renaming (based on our conversation on our chat), hope that was alright! I can see that this might not be the final structure and things will still move but I could identify a few building blocks, hopefully this helps a little:

  • Traits which will eventually move over to p2panda_rs so we don't need to worry too much about them
  • Structs which actually represent SQL rows I moved to models and renamed to EntryRow, LogRow, etc.
  • Structs which serve as some sort of "intermediate" type which is neither in the database nor an p2panda type but still related to storage implementation I kept in the regarding stores module and named OperationStorage, DocumentViewStorage etc.
  • Struct which is the base SqlProvider where all stores are derived from. Renamed sql_store to provider

/// the db as this method only creates relations between document view fields and their current
/// values (last updated operation value).
///
/// QUESTION: Is this too implementation specific? It assumes quite a lot about the db
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be implemented for p2panda_rs? I agree, this implementation is too specific for something else than aquadoggo SQL, but I'm sure we can a) either find something else for a more "generic" p2panda_rs version b) or just leave it unimplemented?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need an insert_document_view() method in p2panda_rs, but exposing the details of how we're doing that is too specific to our db layout, yeh. I would say that we should develop DocumentView to contain the data we need though, as it's useful and not too much overhead, then the signature for this method could be something like:

    async fn insert_document_view(
        &self,
        document_view: &DocumentView,
        schema_id: &SchemaId,
    ) -> Result<bool, DocumentViewStorageError>;

I'll open an issue for changes in DocumentView.

Comment on lines +83 to +86
// - alternatively we could do some dynamic "reverse" graph traversal
// starting from the document view id. This would require
// implementing some new traversal logic (maybe it is already underway
// somewhere? I remember @cafca working on this a while ago).
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I was so sure that @cafca was working on this and even merged it, but now I can't find it anywhere 🕵️

@sandreae
Copy link
Member Author

sandreae commented May 7, 2022

Thanks for the branch organising 👍 and all the naming makes a lot of sense!

@sandreae sandreae changed the title Implement OperationStore and DocumentStore StorageProvider development May 9, 2022
@sandreae
Copy link
Member Author

sandreae commented May 9, 2022

I'm retiring this PR and splitting it into a series of smaller ones, goodbye my friend. ⛵

First up:

@sandreae sandreae closed this May 9, 2022
@sandreae sandreae deleted the db-operation branch May 23, 2022 01:32
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.

None yet

2 participants