-
Notifications
You must be signed in to change notification settings - Fork 84
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
H-3543: Add interface to read closed multi-entity type schemas #5578
H-3543: Add interface to read closed multi-entity type schemas #5578
Conversation
const [firstEntityTypeId, ...restEntityTypesIds] = entityTypesIds.sort(); | ||
let currentClosedMultiEntityTypeMap = | ||
response.closedMultiEntityTypes[firstEntityTypeId]; | ||
|
||
for (const id of restEntityTypesIds) { | ||
if (!currentClosedMultiEntityTypeMap?.inner) { | ||
return; | ||
} | ||
currentClosedMultiEntityTypeMap = currentClosedMultiEntityTypeMap.inner[id]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to understand what is going on here, I would have thought a closed multi-entity type would be keyed under some combination of all its typeIds but here we have items keyed by a single typeId and are returning whatever the item in the map with the last entityTypeId in the list is (unless one is missing, in which case we return nothing). Why is it like this, and what happens if there are multiple multi-type entities which have the same entityTypeId amongst their types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it's a nested map which is nested to however many levels there are numbers of types for the entity... is this what we want long-term, or is it just temporary? I thought we'd just sort and concatenate the typeIds or something. Is there a benefit to doing it this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if we would keep it longer term. This is the reason why I have "hidden" the implementation behind the response
object. I'm currently writing a few comments to explain this more. Also, I try to rewrite it to use reduce
instead.
The reason why I used this is because it's easier to build and does not need to reallocate strings when concatenating strings. It can be iteratively be built in the graph as it's layered.
return; | ||
} | ||
|
||
const [firstEntityTypeId, ...restEntityTypesIds] = entityTypesIds.sort(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it really matters here, but to note that sort
mutates the original array, we can use this newish alternative to avoid mutating the input array
const [firstEntityTypeId, ...restEntityTypesIds] = entityTypesIds.sort(); | |
const [firstEntityTypeId, ...restEntityTypesIds] = entityTypesIds.toSorted(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the reasons why I like Rust, if there is a side effect it's impossible to have an existing reference to entityTypeIds
, so it's possible to just sort the array and profit from it later again if we call sort()
again (I assume O(n)
for sorted arrays).
I changed it to toSorted
but had to add !
to firstEntityTypeId
as it does not retain the exact type of the array.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5578 +/- ##
==========================================
- Coverage 20.15% 20.09% -0.06%
==========================================
Files 509 514 +5
Lines 17319 17368 +49
Branches 2538 2545 +7
==========================================
Hits 3490 3490
- Misses 13791 13840 +49
Partials 38 38
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Benchmark results
|
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | 25 entities | Flame Graph | |
entity_by_id | 5 entities | Flame Graph | |
entity_by_id | 1 entities | Flame Graph | |
entity_by_id | 10 entities | Flame Graph | |
entity_by_id | 50 entities | Flame Graph |
representative_read_entity
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/book/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/block/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/organization/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/song/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/page/v/2
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/person/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/uk-address/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/building/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/playlist/v/1
|
Flame Graph |
scaling_read_entity_complete_zero_depth
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | 25 entities | Flame Graph | |
entity_by_id | 5 entities | Flame Graph | |
entity_by_id | 1 entities | Flame Graph | |
entity_by_id | 10 entities | Flame Graph | |
entity_by_id | 50 entities | Flame Graph |
scaling_read_entity_linkless
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | 1 entities | Flame Graph | |
entity_by_id | 10000 entities | Flame Graph | |
entity_by_id | 100 entities | Flame Graph | |
entity_by_id | 1000 entities | Flame Graph | |
entity_by_id | 10 entities | Flame Graph |
representative_read_multiple_entities
Function | Value | Mean | Flame graphs |
---|---|---|---|
link_by_source_by_property | depths: DT=255, PT=255, ET=255, E=255 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=0, ET=2, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=2, PT=2, ET=2, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=0, ET=0, E=0 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=2, ET=2, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=0, ET=0, E=2 | Flame Graph | |
entity_by_property | depths: DT=255, PT=255, ET=255, E=255 | Flame Graph | |
entity_by_property | depths: DT=0, PT=0, ET=2, E=2 | Flame Graph | |
entity_by_property | depths: DT=2, PT=2, ET=2, E=2 | Flame Graph | |
entity_by_property | depths: DT=0, PT=0, ET=0, E=0 | Flame Graph | |
entity_by_property | depths: DT=0, PT=2, ET=2, E=2 | Flame Graph | |
entity_by_property | depths: DT=0, PT=0, ET=0, E=2 | Flame Graph |
representative_read_entity_type
Function | Value | Mean | Flame graphs |
---|---|---|---|
get_entity_type_by_id | Account ID: d4e16033-c281-4cde-aa35-9085bf2e7579
|
Flame Graph |
🌟 What is the purpose of this PR?
We are able to query closed multi-entity types from the graph but it would be more helpful if it could be passed when querying entities.
🔍 What does this change?
includeClosedMultiEntityTypes = false
to the parameters when requesting entities or an entity subgraphgetClosedMultiEntityTypesFromResponse
in thehash-graph-sdk
which takes the response object and the entity type ids. That should give the same type as a query to the graph with the same entity type ids.Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
The graph implementation might not be the fastest, but until we benchmark this code path there is no point in optimizing this.
🛡 What tests cover this?
Tests were added to compare the results of the returned query against the schema returned from the graph when querying the types independently.