Separate ExecutionPlan op construction logic into different files by jeffreylovitz · Pull Request #1352 · RedisGraph/RedisGraph · GitHub
Skip to content

Separate ExecutionPlan op construction logic into different files#1352

Merged
swilly22 merged 6 commits into
masterfrom
execution-plan-logical-units
Oct 1, 2020
Merged

Separate ExecutionPlan op construction logic into different files#1352
swilly22 merged 6 commits into
masterfrom
execution-plan-logical-units

Conversation

@jeffreylovitz

@jeffreylovitz jeffreylovitz commented Sep 24, 2020

Copy link
Copy Markdown
Contributor

This is a separation of the op-building code to outside of execution_plan.c, no logic has been changed.

Let me know what you think and whether you'd like anything to be structured differently!

@jeffreylovitz jeffreylovitz self-assigned this Sep 24, 2020
swilly22
swilly22 suggested changes Sep 27, 2020
View reviewed changes

@swilly22 swilly22 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is so much cleaner, its been overdue. just a few comments.

Comment thread src/execution_plan/execution_plan_build/execution_plan_clone.h Outdated

@swilly22 swilly22 Sep 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it would be OK leaving execution_plan_clone under /src/execution_plan/

Comment thread src/execution_plan/execution_plan_build/execution_plan_modify.c Outdated
Comment on lines +106 to +107


@swilly22 swilly22 Sep 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove extra line

Comment thread src/execution_plan/execution_plan_build/execution_plan_modify.c
_OpBase_AddChild(tail, old_root);
}

inline void ExecutionPlan_UpdateRoot(ExecutionPlan *plan, OpBase *new_root) {

@swilly22 swilly22 Sep 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this a replacement to _ExecutionPlan_UpdateRoot ?

@jeffreylovitz jeffreylovitz Sep 28, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes - same logic, just migrated into this file.

Comment thread src/execution_plan/execution_plan_build/execution_plan_clone.c Outdated
#include "../RG.h"
#include "../query_ctx.h"
#include "../util/rax_extensions.h"
#include "../../RG.h"

@swilly22 swilly22 Sep 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it would be OK leaving execution_plan_clone under /src/execution_plan/

Comment thread src/execution_plan/execution_plan_build/convert_clauses.c
Comment on lines +1 to +8
#include "../execution_plan.h"
#include "../../RG.h"
#include "../ops/ops.h"
#include "../../query_ctx.h"
#include "../../util/rax_extensions.h"
#include "../../ast/ast_build_ar_exp.h"
#include "../ops/shared/scan_functions.h"
#include "../../ast/ast_build_op_contexts.h"

@swilly22 swilly22 Sep 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please make sure every include is indeed needed

Comment thread src/execution_plan/execution_plan_build/convert_clauses.c Outdated
Comment on lines +13 to +17
/* Clause conversion logic in other files. */
void buildMatchOpTree(ExecutionPlan *plan, AST *ast, const cypher_astnode_t *clause);
void buildReturnOps(ExecutionPlan *plan, const cypher_astnode_t *clause);
void buildWithOps(ExecutionPlan *plan, const cypher_astnode_t *clause);
void buildMergeOp(ExecutionPlan *plan, AST *ast, const cypher_astnode_t *clause, GraphContext *gc);

@swilly22 swilly22 Sep 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I prefer moving these to a header file, including ExecutionPlanSegment_ConvertClause

Comment thread src/execution_plan/execution_plan_build/build_projection_ops.c
Comment on lines +133 to +134


@swilly22 swilly22 Sep 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove extra line

swilly22
swilly22 previously approved these changes Sep 30, 2020
View reviewed changes

@swilly22 swilly22 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please see comments.

Comment thread src/execution_plan/execution_plan.c Outdated
#include "./optimizations/optimizer.h"
#include "../ast/ast_build_filter_tree.h"
#include "./optimizations/optimizations.h"
#include "execution_plan_build/convert_clauses.h"

@swilly22 swilly22 Sep 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe rename convert_clauses.h to execution_plan_construct.h ?

Comment thread src/execution_plan/execution_plan_build/execution_plan_modify.h Outdated
Comment on lines +40 to +43

@swilly22 swilly22 Sep 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider using GraphBLAS comment "notation"

//-----------------------------
// DESC
//-----------------------------

@swilly22 swilly22 merged commit 4898fde into master Oct 1, 2020
@swilly22 swilly22 deleted the execution-plan-logical-units branch October 1, 2020 13:21
alonre24 pushed a commit that referenced this pull request Oct 27, 2020
@jeffreylovitz @swilly22 @alonre24
Separate ExecutionPlan op construction logic into different files (#1352
9497c83 
)

* Separate ExecutionPlan op construction logic into different files

* Address PR comments

* Refactor header files

* Address PR comments

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
(cherry picked from commit 4898fde)
swilly22 added a commit that referenced this pull request Oct 27, 2020
@alonre24 @jeffreylovitz @swilly22
V2.2.7 (#1404)
4439a93 
* Allow upper case letters in proc names (#1372)

* added to_lower for procedure name in Proc_get to support both upper and lower cases

* new test for lookup_case_insensitive

* Created strutil and moved the implementations of str_tolower and str_toupper to it. Remove the static implementations of these functions and replaced every call with a new call for str_tolower.
minor changes in getProc and procRegister due to PR

* fix indentations

* fix indentations

* minor changes

* minor changes

* indentations fix

* indentations fix

* indentations fix

* indentations fix

* indentations fix

* PR fixes

* PR fixes

* PR fixes

* PR fixes

* PR fixes

* PR fixes

* PR fixes

* PR fixes

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
(cherry picked from commit d0ec075)

* Always set GraphCtx in QueryCtx while decoding graph keys (#1381)

(cherry picked from commit 38f6f56)

* Avoid double free of heap-allocated values in CASE...WHEN statements (#1386)

* Avoid double free of heap-allocated values in CASE...WHEN statements

* Update test_function_calls.py

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
(cherry picked from commit e25746f)

* Separate ExecutionPlan op construction logic into different files (#1352)

* Separate ExecutionPlan op construction logic into different files

* Address PR comments

* Refactor header files

* Address PR comments

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
(cherry picked from commit 4898fde)

* With filter scoping (#1361)

* Don't migrate WITH filters into Merge and Apply scopes

* Don't place filters in previous scope for filtered aliases

* Fix memory error

* remove vector

* consolidate operation locating

* execution plan construction refactoring WIP

* move arithmetic expression construction from AST to AR_EXP

* discover skip, limit and black-listed ops modifiers

* PR fixes

* Fixes continued

* Fix ResultSet updates in UNION queries

* Clarify comments, minor cleanup

* Fix unit tests

* Fix caching logic for SKIP and LIMIT

* Address PR comments

* tidy up execution plan construction and join op

* reset limit on aggregation

* define 16 as the default batch size for traversal operations

Co-authored-by: swilly22 <roi@redislabs.com>
Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
(cherry picked from commit f2de97a)

* Allow property accesses on non-identifier entity references (#1389)

* Allow property accesses on non-identifier entity references

* access entity attribute only via the property function

* address PR comments

Co-authored-by: swilly22 <roi@redislabs.com>
Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
(cherry picked from commit 70372aa)

* redundent filter construction and placement (#1397)

* redundent filter construction and placement

* Add validating flow tests

Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com>
(cherry picked from commit c65aecf)

* Update patch version to 7

Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com>
Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
Co-authored-by: DvirDukhan <dvir@redislabs.com>
swilly22 added a commit that referenced this pull request Nov 8, 2020
@alonre24 @swilly22
@jeffreylovitz @gkorland @anakaiti @simonprickett
V2.2.8 (#1418)
5aec24e 
* added init time NodeScanCtx update on label scan (#1358)

* added init time NodeScanCtx update on label scan

* rephrase comment

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>

* Bfs procedure call (#1306)

* Import LAGraph files

* Apply autoformatter

* Add LAGraph include file

* WIP Add BFS procedure

* tmp Utility

* Revert LAGraph inclusions

* Replace LAGraph reference with GraphBLAS references

* WIP

* WIP

* Add BFSTree procedure

* Add unit tests

* Update documentation

* Resolve compiler warnings

* Address PR comments

* Make singular BFS procedure with variable outputs

* Remove start_node yield

* Address PR comments

* Remove yield-edges BFS input arg

* Address PR comments

* Switch to LAGraph_bfs_pushpull

* Remove unused code paths from LAGraph BFS algorithm

* procedure invocation receives yield array

* consume BFS result using iterator

* test BFS returning no results

* clean up, address PR comments

Co-authored-by: swilly22 <roi@redislabs.com>
Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>

* Separate ExecutionPlan op construction logic into different files (#1352)

* Separate ExecutionPlan op construction logic into different files

* Address PR comments

* Refactor header files

* Address PR comments

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>

* Support array properties in bulk loader (#1365)

* Support array properties in bulk loader

* Address PR comments

* Add no_multi_key & hash_policy (#1363)

* Add no_multi_key & hash_policy

* Update ramp.yml

At the moment we can't handle user hash policy due to virtual keys

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>

* Add LGTM (#1367)

* read only query (#1364)

* read only query

* added sleep, try to avoid ci failure

* added more write scenarios. added replica shutdown

* added skip flag to avoid memcheck failure

* fixed PR comments

* Update module.c

* try to skip on any debugger. added more write scenarios

* checkSlaveSynced moved to use time.sleep. Another try to skip debuger

* added RO_QUERY to commands

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>

* Fix typo (#1368)

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>

* Add GRAPH.BULK documentation and link to bulk loader documentation (#1371)

* Add GRAPH.BULK documentation and link to bulk loader documentation

* Address PR comments

* Allow upper case letters in proc names (#1372)

* added to_lower for procedure name in Proc_get to support both upper and lower cases

* new test for lookup_case_insensitive

* Created strutil and moved the implementations of str_tolower and str_toupper to it. Remove the static implementations of these functions and replaced every call with a new call for str_tolower.
minor changes in getProc and procRegister due to PR

* fix indentations

* fix indentations

* minor changes

* minor changes

* indentations fix

* indentations fix

* indentations fix

* indentations fix

* indentations fix

* PR fixes

* PR fixes

* PR fixes

* PR fixes

* PR fixes

* PR fixes

* PR fixes

* PR fixes

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>

* Adding a new procedure that returns all the procedures in the system (#1377)

* Adding a new procedure that returns all the procedures in the system. This procedure may yield up to two fields for each procedure: its name and/or its mode (READ/WRITE).

* PR fixes

* Saving raxIterator reference instead of allocating it on the heap in proc_proceudres_private_data. More PR code fixes

* Remove the getter "Procedure_ReadOnly" that gets only the procedures name, leaving only the getter "Procedure_IsReadOnly" that gets the whole procedure object.

* Always set GraphCtx in QueryCtx while decoding graph keys (#1381)

* LGTM alert fixes (#1383)

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>

* Avoid double free of heap-allocated values in CASE...WHEN statements (#1386)

* Avoid double free of heap-allocated values in CASE...WHEN statements

* Update test_function_calls.py

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>

* With filter scoping (#1361)

* Don't migrate WITH filters into Merge and Apply scopes

* Don't place filters in previous scope for filtered aliases

* Fix memory error

* remove vector

* consolidate operation locating

* execution plan construction refactoring WIP

* move arithmetic expression construction from AST to AR_EXP

* discover skip, limit and black-listed ops modifiers

* PR fixes

* Fixes continued

* Fix ResultSet updates in UNION queries

* Clarify comments, minor cleanup

* Fix unit tests

* Fix caching logic for SKIP and LIMIT

* Address PR comments

* tidy up execution plan construction and join op

* reset limit on aggregation

* define 16 as the default batch size for traversal operations

Co-authored-by: swilly22 <roi@redislabs.com>
Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>

* Updates to Astyle config file (#1384)

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>

* replace gitter with Discord (#1395)

* reaplace gitter with Discord

* replace gitter to discord

* Allow property accesses on non-identifier entity references (#1389)

* Allow property accesses on non-identifier entity references

* access entity attribute only via the property function

* address PR comments

Co-authored-by: swilly22 <roi@redislabs.com>
Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>

* Minor suggested update. (#1398)

* replace automation service (#1399)

* redundent filter construction and placement (#1397)

* redundent filter construction and placement

* Add validating flow tests

Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com>

* Parse full argv array on main thread (#1298)

* Parse full argv array on main thread

* remove read_flags from cmd_query

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
Co-authored-by: swilly22 <roi@redislabs.com>

* query validation should not check if procedure outputs been defined (#1406)

* query validation should not check if procedure outputs been defined

* address PR comments

* Add trusted-by logos (#1412)

* Add trusted-by logos

* add more logos

* Fuzzy test crashes (#1408)

* Correctly place filters without referenced variables

* Emit compile-time error on constant non-boolean filter

* Validate references per clause instead of per scope

* Emit error on failed filter placement in optimize_cartesian_product

* Fix free of invalid value in OpUnwind

* Handle NULL sources in variable-length traversals

* Address PR comments

* Post-rebase fixes

* Address PR comments

* introduce AST unsupported op error

Co-authored-by: swilly22 <roi@redislabs.com>

* Graph version (#1382)

* add graph version to graph context object

* change graph version from string to uint32

* testing graph version

* response with an error when graph version mismatch

* release graph context on version mismatch

* address PR comments

* increase command arity to accomadate graph version argument

* address PR comments

Co-authored-by: DvirDukhan <dvir@redislabs.com>

* Release version 2.2.8. Merged from master.

Co-authored-by: DvirDukhan <dvir@redislabs.com>
Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com>
Co-authored-by: swilly22 <roi@redislabs.com>
Co-authored-by: Guy Korland <gkorland@gmail.com>
Co-authored-by: Yahya <5457202+anakaiti@users.noreply.github.com>
Co-authored-by: Simon Prickett <simon@crudworks.org>
pnxguide pushed a commit to CMU-SPEED/RedisGraph that referenced this pull request Mar 22, 2023
@jeffreylovitz @swilly22
Separate ExecutionPlan op construction logic into different files (Re…
83f4b1e 
…disGraph#1352)

* Separate ExecutionPlan op construction logic into different files

* Address PR comments

* Refactor header files

* Address PR comments

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DvirDukhan DvirDukhan Awaiting requested review from DvirDukhan
1 more reviewer
@swilly22 swilly22 swilly22 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@jeffreylovitz jeffreylovitz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jeffreylovitz @swilly22