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

Feature/make properties typed #1266

Merged
merged 10 commits into from
Sep 20, 2023
Merged

Conversation

ljeub-pometry
Copy link
Collaborator

What changes were proposed in this pull request?

  • Properties are forced to have consistent type across vertices/edges
  • Eliminated a bunch of String clones during property ingestion
  • Internal ingestion apis now work only with the ids which should make it possible to speed up loading of columnar data (such as csv/pandas) by only resolving property names once (not implemented yet).
  • Graph constant properties now error when attempting to change the value (same as vertex and edge properties)

Why are the changes needed?

  • Fix bugs and make it possible to treat properties as columnar

Does this PR introduce any user-facing change? If yes is this documented?

  • API is unchanged though a few more things produce errors now

How was this patch tested?

  • Existing tests still pass after minor fixes to stop testing broken behaviour

Issues

If this resolves any issues, please link to them here, the format is a KEYWORD followed by @_
KEYWORDS available are close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved.
Please delete this text before creating your PR

Are there any further changes required?

  • More tests
  • Improvements to property retrieval that are now possible
  • Speed up csv and pandas loader by using property ids directly during ingestion

@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Patch coverage: 64.76% and project coverage change: -0.03% ⚠️

Comparison is base (3af8ab4) 58.02% compared to head (0417fca) 58.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1266      +/-   ##
==========================================
- Coverage   58.02%   58.00%   -0.03%     
==========================================
  Files         173      173              
  Lines       18562    18776     +214     
==========================================
+ Hits        10771    10891     +120     
- Misses       7791     7885      +94     
Files Changed Coverage Δ
python/src/graphql.rs 0.00% <ø> (ø)
python/src/lib.rs 1.58% <ø> (ø)
raphtory-graphql/src/data.rs 0.00% <0.00%> (ø)
raphtory-graphql/src/lib.rs 0.00% <ø> (ø)
raphtory-graphql/src/model/filters/edge_filter.rs 0.00% <0.00%> (ø)
...htory-graphql/src/model/filters/property_filter.rs 0.00% <0.00%> (ø)
raphtory-graphql/src/model/graph/edge.rs 0.00% <0.00%> (ø)
raphtory-graphql/src/model/graph/graph.rs 0.00% <0.00%> (ø)
raphtory-graphql/src/model/graph/mod.rs 0.00% <0.00%> (ø)
raphtory-graphql/src/model/graph/node.rs 0.00% <0.00%> (ø)
... and 59 more

... and 1 file with indirect coverage changes

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

@ljeub-pometry ljeub-pometry force-pushed the feature/MakePropertiesTyped branch from d490a17 to ff2094a Compare September 18, 2023 11:21
ljeub-pometry and others added 5 commits September 20, 2023 13:13
* eliminate a lot of arc clones

* replace String by ArcStr (wrapped Arc<str>) for cheap clone and to make it possible to support string deduplication

* test string deduplication

* implement string deduplication for property values

* clean up warnings

* expose meta data in core ops and minor cleanup
@ljeub-pometry ljeub-pometry force-pushed the feature/MakePropertiesTyped branch from d9aa5c1 to 349da6e Compare September 20, 2023 11:28
Copy link
Collaborator

@miratepuffin miratepuffin left a comment

Choose a reason for hiding this comment

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

Couple of points/questions, but feel free to merge when happy.

}

pub type Key = String; //Fixme: This should really be the internal usize index but that means more reworking of the low-level api
pub type Key = ArcStr; //Fixme: This should really be the internal usize index but that means more reworking of the low-level api
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wanna make a ticket for this so we don't forget


fn edge_meta(&self) -> &Meta;

fn graph_meta(&self) -> &GraphProps;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should GraphProps be renamed to GraphMeta?

@miratepuffin miratepuffin merged commit 8096021 into master Sep 20, 2023
@miratepuffin miratepuffin deleted the feature/MakePropertiesTyped branch September 20, 2023 17:07
fabianmurariu pushed a commit that referenced this pull request May 21, 2024
* take property insertion apart and put it back together again

* fix tests that were testing broken behaviour

* remove `"_id"` from properties and change stray ints to floats in python tests

* fix warnings

* String deduplication (#1269)

* eliminate a lot of arc clones

* replace String by ArcStr (wrapped Arc<str>) for cheap clone and to make it possible to support string deduplication

* test string deduplication

* implement string deduplication for property values

* clean up warnings

* expose meta data in core ops and minor cleanup

* fix rebase issues and clean up warnings

* dubious warning fix

* attribute does not work, warning is still there

* simplify edge addition and deletion

* No more spin-locking for adding edges (instead get locks in consistent order)
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