Skip to content

Support dynamic inline properties for CREATE and MERGE patterns #594

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Aug 15, 2019
Merged

Support dynamic inline properties for CREATE and MERGE patterns #594

merged 7 commits into from
Aug 15, 2019

Conversation

jeffreylovitz
Copy link
Contributor

No description provided.

@jeffreylovitz jeffreylovitz requested a review from swilly22 August 13, 2019 21:28
@jeffreylovitz jeffreylovitz self-assigned this Aug 13, 2019
@jeffreylovitz jeffreylovitz force-pushed the dynamic-property-creation branch from 9c49635 to 880ed76 Compare August 13, 2019 22:02
swilly22
swilly22 suggested changes Aug 14, 2019
View reviewed changes
src/execution_plan/ops/op_merge.c

uint node_count = array_len(op->nodes_to_merge);
for(uint i = 0; i < node_count; i ++) {
PropertyMap_Free(op->nodes_to_merge[i].properties);
Copy link
Contributor

Choose a reason for hiding this comment

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

Plugged leak?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes; this should have been there all along.

src/graph/entities/graph_entity.h
@@ -28,6 +28,7 @@ typedef GrB_Index NodeID;
typedef GrB_Index EdgeID;

typedef enum GraphEntityType {
GETYPE_UNKNOWN,
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this dangerous, is that compatible with old RDBs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GraphEntityType never gets serialized; prior to now it had only been used in op_update.c. I deleted the (nearly identical) enum EntityType to declutter a bit.

src/execution_plan/ops/op_merge.c
static void _AddEdgeProperties(OpMerge *op, Schema *schema, Edge *e, PropertyMap *props) {
if(props == NULL) return;

static void _AddProperties(OpMerge *op, Record r, GraphEntity *ge,
Copy link
Contributor

Choose a reason for hiding this comment

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

NIce!

src/execution_plan/ops/op_create.h
@@ -15,6 +15,13 @@

/* Creates new entities according to the CREATE clause. */

// Container struct for properties to be added to a new graph entity
typedef struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's wrong with PropertyMap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This stores SIValues that have been converted from the AR_ExpNodes of PropertyMap. We could use the PropertyMap struct by calling AR_EXP_NewConstOperandNode on each value, but I think that would be more wasteful than necessary.

src/ast/ast_build_op_contexts.c
@@ -317,13 +313,13 @@ AST_CreateContext AST_PrepareCreateOp(GraphContext *gc, RecordMap *record_map, A
QueryGraph_AddPath(gc, ast, qg, path);

uint path_elem_count = cypher_ast_pattern_path_nelements(path);
for(uint j = 0; j < path_elem_count; j ++) {
for(uint k = 0; k < path_elem_count; k ++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How have we missed this?!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, I know. I can't believe that this type of bug doesn't trigger a compiler warning?

src/execution_plan/ops/op_create.c Outdated
PendingProperties *_ConvertPropertyMap(Record r, const PropertyMap *map) {
PendingProperties *converted = rm_malloc(sizeof(PendingProperties));
converted->property_count = map->property_count;
converted->keys = map->keys;
Copy link
Contributor

Choose a reason for hiding this comment

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

How about instead of holding keys as const char ** we use an array of attribute ids?
should save us some time when committing the properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

src/execution_plan/ops/op_create.c Outdated
/* Convert query-level properties. */
PropertyMap *map = op->edges_to_create[i].properties;
PendingProperties *converted_properties = NULL;
if(map) {
Copy link
Contributor

Choose a reason for hiding this comment

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

one liner

src/execution_plan/ops/op_create.c Outdated
/* Convert query-level properties. */
PropertyMap *map = op->nodes_to_create[i].properties;
PendingProperties *converted_properties = NULL;
if(map) {
Copy link
Contributor

Choose a reason for hiding this comment

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

one liner

src/execution_plan/ops/op_create.c Outdated
for(uint i = 0; i < prop_count; i ++) {
PendingProperties *props = op->node_properties[i];
if(props == NULL) continue;
for(uint j = 0; j < props->property_count; j ++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Introduce PendingProperties_Free function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

swilly22
swilly22 suggested changes Aug 15, 2019
View reviewed changes
src/execution_plan/ops/op_create.c Outdated
PendingProperties *converted = rm_malloc(sizeof(PendingProperties));
converted->property_count = map->property_count;
converted->keys = map->keys;
converted->attr_keys = rm_malloc(sizeof(Attribute_ID) * map->property_count);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this was my miss understanding, I wanted to reduce the number of calls to GraphContext_FindOrAddAttribute suppose we've got one PropertyMap object,
MATCH (n) CREATE (m {n.v + 1})
and there are 1M n nodes in our graph, as a result GraphContext_FindOrAddAttribute will be called 1M times,
I think we should extend PropertyMap to include Attribute_ID, this way we'll only need to call GraphContext_FindOrAddAttribute once per attribute.
reducing the example above from 1M to just once.

@jeffreylovitz jeffreylovitz force-pushed the dynamic-property-creation branch from 94aba71 to 6bed431 Compare August 15, 2019 17:55
@swilly22 swilly22 merged commit 085a359 into master Aug 15, 2019
@swilly22 swilly22 deleted the dynamic-property-creation branch August 15, 2019 18:00
pnxguide pushed a commit to CMU-SPEED/RedisGraph that referenced this pull request Mar 22, 2023
@jeffreylovitz @swilly22
Support dynamic inline properties for CREATE and MERGE patterns (Redi…
1b0c5d8 
…sGraph#594)

* Remove extraneous entity type enum

* Support non-constant inlined properties in CREATE and MERGE clauses

* Fix memory leaks

* Add additional tests

* PR fixes

* Early translation of string keys into Attribute IDs

* Fix mis-sized allocation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@swilly22 swilly22 swilly22 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@jeffreylovitz jeffreylovitz

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jeffreylovitz @swilly22