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

H-2959, H-3368: Implement a Value data type and make all data types inherit from it #5240

Conversation

TimDiekmann
Copy link
Member

@TimDiekmann TimDiekmann commented Sep 26, 2024

🌟 What is the purpose of this PR?

Every data type should inherit (directly or indirectly) from Value.

🚫 Blocked by

🔍 What does this change?

  • Add a Value data type definitions:
     {
     	"$schema": "https://blockprotocol.org/types/modules/graph/0.3/schema/data-type",
     	"kind": "dataType",
     	"$id": "https://blockprotocol.org/@blockprotocol/types/data-type/value/v/1",
     	"title": "Value",
     	"description": "A value that can be stored in a graph",
     	"anyOf": [
     		{ "type": "null" },
     		{ "type": "boolean" },
     		{ "type": "number" },
     		{ "type": "string" },
     		{ "type": "array" },
     		{ "type": "object" }
     	]
     }
  • Move cached external types from Node migration to the type fetcher. This is the natural flow for reading external types and the type fetcher can read the cached types to avoid going to well-known types. In comparison to the Node API migration this implies that the types may always be available if the type fetcher is running and there is no custom logic required to mimic fetching external types.
  • Change existing primitive types to inherit from Value
  • Change empty-list type to use items: false instead of const: []
  • Remove const for array schemas
  • Enforce a parent in data types (except for Value)

Pre-Merge Checklist 🚀

🚢 Has this modified a publishable library?

This PR:

  • does not modify any publishable blocks or libraries, or modifications do not need publishing

📜 Does this require a change to the docs?

The changes in this PR:

  • are internal and do not require a docs change

🕸️ Does this require a change to the Turbo Graph?

The changes in this PR:

  • do not affect the execution graph

⚠️ Known issues

The production instance needs to be patched to read the new data

📹 Demo

@TimDiekmann TimDiekmann self-assigned this Sep 26, 2024
@github-actions github-actions bot added area/deps Relates to third-party dependencies (area) area/apps > hash* Affects HASH (a `hash-*` app) area/apps > hash-api Affects the HASH API (app) area/libs Relates to first-party libraries/crates/packages (area) type/eng > backend Owned by the @backend team area/tests New or updated tests area/apps area/apps > hash-graph labels Sep 26, 2024
Copy link

codecov bot commented Sep 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 20.23%. Comparing base (4f7aa39) to head (882b224).
Report is 118 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5240      +/-   ##
==========================================
+ Coverage   20.18%   20.23%   +0.04%     
==========================================
  Files         510      508       -2     
  Lines       17300    17260      -40     
  Branches     2546     2537       -9     
==========================================
  Hits         3492     3492              
+ Misses      13770    13730      -40     
  Partials       38       38              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot added the type/eng > frontend Owned by the @frontend team label Sep 27, 2024
@TimDiekmann TimDiekmann changed the title H-2959: Implement a Value data type and make all data types inherit from it H-2959, H-3368: Implement a Value data type and make all data types inherit from it Sep 27, 2024
@TimDiekmann TimDiekmann force-pushed the t/h-2958-extend-type-system-crate-to-support-extended-custom-data branch from 3e7252a to d159b3d Compare September 27, 2024 10:50
@TimDiekmann TimDiekmann force-pushed the t/h-2959-implement-a-value-data-type-and-make-all-data-types-inherit branch from 9938cd9 to c5bfb3b Compare September 27, 2024 14:07
@github-actions github-actions bot added the area/tests > integration New or updated integration tests label Sep 28, 2024
@TimDiekmann TimDiekmann marked this pull request as ready for review September 28, 2024 10:26
Base automatically changed from t/h-2958-extend-type-system-crate-to-support-extended-custom-data to main September 29, 2024 14:31
…ype-and-make-all-data-types-inherit

# Conflicts:
#	apps/hash-graph/libs/graph/src/store/validation.rs
Copy link
Contributor

github-actions bot commented Oct 1, 2024

Benchmark results

@rust/graph-benches – Integrations

representative_read_multiple_entities

Function Value Mean Flame graphs
entity_by_property depths: DT=0, PT=2, ET=2, E=2 $$56.2 \mathrm{ms} \pm 512 \mathrm{μs}\left({\color{gray}1.46 \mathrm{\%}}\right) $$ Flame Graph
entity_by_property depths: DT=0, PT=0, ET=2, E=2 $$51.7 \mathrm{ms} \pm 360 \mathrm{μs}\left({\color{gray}0.628 \mathrm{\%}}\right) $$ Flame Graph
entity_by_property depths: DT=0, PT=0, ET=0, E=2 $$44.9 \mathrm{ms} \pm 242 \mathrm{μs}\left({\color{gray}-0.336 \mathrm{\%}}\right) $$ Flame Graph
entity_by_property depths: DT=255, PT=255, ET=255, E=255 $$68.0 \mathrm{ms} \pm 271 \mathrm{μs}\left({\color{gray}-0.890 \mathrm{\%}}\right) $$ Flame Graph
entity_by_property depths: DT=2, PT=2, ET=2, E=2 $$60.0 \mathrm{ms} \pm 412 \mathrm{μs}\left({\color{gray}-0.367 \mathrm{\%}}\right) $$ Flame Graph
entity_by_property depths: DT=0, PT=0, ET=0, E=0 $$41.1 \mathrm{ms} \pm 218 \mathrm{μs}\left({\color{gray}-0.034 \mathrm{\%}}\right) $$ Flame Graph
link_by_source_by_property depths: DT=0, PT=2, ET=2, E=2 $$94.4 \mathrm{ms} \pm 623 \mathrm{μs}\left({\color{gray}-0.896 \mathrm{\%}}\right) $$ Flame Graph
link_by_source_by_property depths: DT=0, PT=0, ET=2, E=2 $$89.5 \mathrm{ms} \pm 527 \mathrm{μs}\left({\color{gray}-2.175 \mathrm{\%}}\right) $$ Flame Graph
link_by_source_by_property depths: DT=0, PT=0, ET=0, E=2 $$83.4 \mathrm{ms} \pm 684 \mathrm{μs}\left({\color{gray}1.74 \mathrm{\%}}\right) $$ Flame Graph
link_by_source_by_property depths: DT=255, PT=255, ET=255, E=255 $$107 \mathrm{ms} \pm 605 \mathrm{μs}\left({\color{gray}-0.889 \mathrm{\%}}\right) $$ Flame Graph
link_by_source_by_property depths: DT=2, PT=2, ET=2, E=2 $$98.8 \mathrm{ms} \pm 442 \mathrm{μs}\left({\color{gray}-1.663 \mathrm{\%}}\right) $$ Flame Graph
link_by_source_by_property depths: DT=0, PT=0, ET=0, E=0 $$43.9 \mathrm{ms} \pm 287 \mathrm{μs}\left({\color{gray}-0.172 \mathrm{\%}}\right) $$ Flame Graph

representative_read_entity

Function Value Mean Flame graphs
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/uk-address/v/1 $$16.9 \mathrm{ms} \pm 223 \mathrm{μs}\left({\color{gray}1.36 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/building/v/1 $$16.1 \mathrm{ms} \pm 185 \mathrm{μs}\left({\color{lightgreen}-5.642 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/song/v/1 $$15.5 \mathrm{ms} \pm 194 \mathrm{μs}\left({\color{red}14.2 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/page/v/2 $$16.2 \mathrm{ms} \pm 205 \mathrm{μs}\left({\color{gray}0.625 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/person/v/1 $$15.6 \mathrm{ms} \pm 184 \mathrm{μs}\left({\color{lightgreen}-16.540 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/block/v/1 $$16.0 \mathrm{ms} \pm 167 \mathrm{μs}\left({\color{lightgreen}-10.108 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/book/v/1 $$15.7 \mathrm{ms} \pm 148 \mathrm{μs}\left({\color{lightgreen}-30.322 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/organization/v/1 $$17.2 \mathrm{ms} \pm 205 \mathrm{μs}\left({\color{red}11.7 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/playlist/v/1 $$18.6 \mathrm{ms} \pm 213 \mathrm{μs}\left({\color{red}17.8 \mathrm{\%}}\right) $$ Flame Graph

representative_read_entity_type

Function Value Mean Flame graphs
get_entity_type_by_id Account ID: d4e16033-c281-4cde-aa35-9085bf2e7579 $$1.41 \mathrm{ms} \pm 5.26 \mathrm{μs}\left({\color{gray}0.024 \mathrm{\%}}\right) $$ Flame Graph

scaling_read_entity_complete_zero_depth

Function Value Mean Flame graphs
entity_by_id 10 entities $$2.07 \mathrm{ms} \pm 22.2 \mathrm{μs}\left({\color{gray}2.62 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 25 entities $$2.51 \mathrm{ms} \pm 8.85 \mathrm{μs}\left({\color{gray}-0.082 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 5 entities $$1.90 \mathrm{ms} \pm 6.92 \mathrm{μs}\left({\color{gray}1.13 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 50 entities $$4.09 \mathrm{ms} \pm 27.4 \mathrm{μs}\left({\color{gray}0.354 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 1 entities $$1.87 \mathrm{ms} \pm 9.15 \mathrm{μs}\left({\color{gray}-0.178 \mathrm{\%}}\right) $$ Flame Graph

scaling_read_entity_complete_one_depth

Function Value Mean Flame graphs
entity_by_id 10 entities $$51.3 \mathrm{ms} \pm 312 \mathrm{μs}\left({\color{gray}4.05 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 25 entities $$70.9 \mathrm{ms} \pm 463 \mathrm{μs}\left({\color{gray}-4.690 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 5 entities $$24.6 \mathrm{ms} \pm 100 \mathrm{μs}\left({\color{gray}1.07 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 50 entities $$273 \mathrm{ms} \pm 1.82 \mathrm{ms}\left({\color{gray}0.638 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 1 entities $$19.7 \mathrm{ms} \pm 117 \mathrm{μs}\left({\color{gray}0.052 \mathrm{\%}}\right) $$ Flame Graph

scaling_read_entity_linkless

Function Value Mean Flame graphs
entity_by_id 10000 entities $$9.50 \mathrm{ms} \pm 163 \mathrm{μs}\left({\color{red}6.13 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 10 entities $$1.85 \mathrm{ms} \pm 6.87 \mathrm{μs}\left({\color{gray}0.906 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 100 entities $$2.02 \mathrm{ms} \pm 10.6 \mathrm{μs}\left({\color{gray}1.66 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 1000 entities $$2.73 \mathrm{ms} \pm 22.0 \mathrm{μs}\left({\color{lightgreen}-6.830 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 1 entities $$1.84 \mathrm{ms} \pm 5.09 \mathrm{μs}\left({\color{gray}-0.214 \mathrm{\%}}\right) $$ Flame Graph

@TimDiekmann TimDiekmann added this pull request to the merge queue Oct 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 1, 2024
@TimDiekmann TimDiekmann added this pull request to the merge queue Oct 1, 2024
Merged via the queue into main with commit 2db0e09 Oct 1, 2024
156 of 157 checks passed
@TimDiekmann TimDiekmann deleted the t/h-2959-implement-a-value-data-type-and-make-all-data-types-inherit branch October 1, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/apps > hash* Affects HASH (a `hash-*` app) area/apps > hash-api Affects the HASH API (app) area/apps > hash-graph area/apps area/deps Relates to third-party dependencies (area) area/libs Relates to first-party libraries/crates/packages (area) area/tests > integration New or updated integration tests area/tests New or updated tests type/eng > backend Owned by the @backend team type/eng > frontend Owned by the @frontend team
Development

Successfully merging this pull request may close these issues.

3 participants