Data Builder Record Abstraction #5
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 tasks
Signed-off-by: Andrew <andrjc4@vt.edu>
AndrewCiambrone
force-pushed
the
ajc-data-builder-record-abstraction-rfc
branch
from
612ef6c to
b5e1c5c
Compare
|
will take a look, thanks for the RFC |
Golodhros
reviewed
Oct 8, 2020
Co-authored-by: Marcos Iglesias <190833+Golodhros@users.noreply.github.com>
Signed-off-by: Andrew <andrjc4@vt.edu>
AndrewCiambrone
force-pushed
the
ajc-data-builder-record-abstraction-rfc
branch
2 times, most recently
from
8e76c0c to
30461fe
Compare
Co-authored-by: Marcos Iglesias <190833+Golodhros@users.noreply.github.com> Signed-off-by: Andrew <andrjc4@vt.edu>
AndrewCiambrone
force-pushed
the
ajc-data-builder-record-abstraction-rfc
branch
from
30461fe to
61eab0c
Compare
Signed-off-by: Andrew <andrjc4@vt.edu>
feng-tao
reviewed
Oct 10, 2020
|
also the DCO fails as well |
feng-tao
reviewed
Oct 10, 2020
|
|
||
| ## Reference-level Explanation (aka Technical Details) | ||
|
|
||
| Each of the models in the databuilder are built off of the `GraphSerializable` class. The `GraphSerializable` class is an abstract class that is responsible for validating and returning the nodes and relationships of its subclasses. The interface on the `GraphSerializable` is `next_node` with a return type of `Union[GraphNode, None]` and `next_relation` with a return type of `Union[GraphRelationship, None]`. |
|
|
||
| ## Drawbacks | ||
|
|
||
| This adds a level of complexity because it adds an additional layer to the databuilder. Instead of having the neo4j serialization built into the models it becomes a separate process. This also would add a small performance degradation because the models are producing named tuples and then they are being convered into another format. |
There was a problem hiding this comment.
could you elaborate a bit or maybe you could create the pr and point me to the place of this?
There was a problem hiding this comment.
yeah what I was trying to get at here is that there is a slight performance degradation because you are now converting from a named tuple to a dictionary.
|
also could you create the pr with your branch? thanks a lot |
5 tasks
I am not sure if its a bug or a typo but some of the timestamps were not unquoted. I currently have it so it is unquoted let me know if that is unwanted. |
feng-tao
added
Status: Active
Status: Final Comment Period (FCP)
|
@AndrewCiambrone I just merged the change. Could you update this rfc and move it under rfcs folder and name it 002-xxx.md? thanks. |
…record-abstraction-rfc
…record-abstraction-rfc
Signed-off-by: Andrew <andrjc4@vt.edu>
Signed-off-by: Andrew <andrjc4@vt.edu>
feng-tao
approved these changes
Nov 5, 2020
feng-tao
added
Status: Landed
allisonsuarez
pushed a commit
that referenced
this pull request
Feb 23, 2021
* RFC for introducing an abstraction layer to the databuilder records Signed-off-by: Andrew <andrjc4@vt.edu> * Update templates/000-databuilder-record-abstraction.md Co-authored-by: Marcos Iglesias <190833+Golodhros@users.noreply.github.com> * RFC for introducing an abstraction layer to the databuilder records Signed-off-by: Andrew <andrjc4@vt.edu> * remove merge tags Signed-off-by: Andrew <andrjc4@vt.edu> * move the rfc to the rfc folder Signed-off-by: Andrew <andrjc4@vt.edu> * delete old rfc in template folder Signed-off-by: Andrew <andrjc4@vt.edu> Co-authored-by: Marcos Iglesias <190833+Golodhros@users.noreply.github.com> Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
allisonsuarez
pushed a commit
that referenced
this pull request
May 5, 2021
* RFC for introducing an abstraction layer to the databuilder records Signed-off-by: Andrew <andrjc4@vt.edu> * Update templates/000-databuilder-record-abstraction.md Co-authored-by: Marcos Iglesias <190833+Golodhros@users.noreply.github.com> * RFC for introducing an abstraction layer to the databuilder records Signed-off-by: Andrew <andrjc4@vt.edu> * remove merge tags Signed-off-by: Andrew <andrjc4@vt.edu> * move the rfc to the rfc folder Signed-off-by: Andrew <andrjc4@vt.edu> * delete old rfc in template folder Signed-off-by: Andrew <andrjc4@vt.edu> Co-authored-by: Marcos Iglesias <190833+Golodhros@users.noreply.github.com> Signed-off-by: Allison Suarez Miranda <asuarezmiranda@lyft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.