Graph version #1382

swilly22 · 2020-10-14T19:38:19Z

This PR will continue to fail as long as the new version of RedisGraph-py isn't deployed.
We introduce a Graph Version which "signs" the graph schema: (labels, relationship-types and attribute-set)
on each change to the graph "schema" the version is updated, client libraries taking advantage of the compact result-set format need to be in sync with the graph schema, they should update their internal mapping between labels, relationship-types and attribute-set strings to numeric IDs upon graph version change.

jeffreylovitz

This seems like a sensible change to me. When I test locally with the graph-version redisgraph-py branch, I get 8 errors on queries that should have been cached executions reporting that they were uncached. Is that just me?

lgtm-com · 2020-11-05T14:07:07Z

This pull request introduces 1 alert when merging 9203e38 into 1bb7f20 - view on LGTM.com

new alerts:

1 for Comparison result is always the same

jeffreylovitz

This is looking good to me, but it definitely merits documentation updates.

jeffreylovitz · 2020-11-05T14:48:44Z

src/commands/cmd_query.c

@@ -72,6 +72,7 @@ void Query_SetTimeOut(uint timeout, ExecutionPlan *plan) {
 	Cron_AddTask(timeout, QueryTimedOut, plan);
 }

+


Unnecessary whitespace

jeffreylovitz · 2020-11-05T14:49:55Z

src/commands/cmd_dispatcher.c

+		if(!strcasecmp(arg, "--version")) {
+			int err = REDISMODULE_ERR;
+			if(i < argc - 1) {
+				i++; // Set the current argument to the timeout value.


"version value"

jeffreylovitz · 2020-11-05T14:50:11Z

src/commands/cmd_dispatcher.c

+				*graph_version = v;
+			}
+
+			// Emit error on missing, negative, or non-numeric timeout values.


"version values"

jeffreylovitz · 2020-11-05T14:55:18Z

src/commands/cmd_dispatcher.c

+			}
+
+			// Emit error on missing, negative, or non-numeric timeout values.
+			if(err != REDISMODULE_OK || *graph_version < 0) {


I feel this would be safer with an added check against v > UINT_MAX, we risk an integer overflow with the cast from long long to uint currently.

Also, since *graph_version is unsigned, that comparison always passes - this wouldn't be a problem with comparing against v.

jeffreylovitz · 2020-11-05T14:57:20Z

src/graph/graphcontext.c

+	ASSERT(str != NULL);
+
+	/* Update graph version by hashing 'str' representing the current
+	 * addition to the graph schema: (Label, Relationship-type, Attribute)


👍 This a clever solution.

jeffreylovitz · 2020-11-05T14:58:41Z

src/procedures/proc_bfs.c

@@ -175,6 +175,7 @@ static SIValue *Proc_BFS_Step(ProcedureCtx *ctx) {

 	UNUSED(res);
 	res = GxB_MatrixTupleIter_new(&iter, (GrB_Matrix)bfs_ctx->nodes);
+	UNUSED(res);


This line is unnecessary, dup of 176.

… graph-version

lgtm-com · 2020-11-05T19:41:33Z

This pull request introduces 1 alert when merging 3ff8500 into 1bb7f20 - view on LGTM.com

new alerts:

1 for Comparison result is always the same

* added init time NodeScanCtx update on label scan (#1358) * added init time NodeScanCtx update on label scan * rephrase comment Co-authored-by: Roi Lipman <[email protected]> * 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 <[email protected]> Co-authored-by: Roi Lipman <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * Fix typo (#1368) Co-authored-by: Roi Lipman <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Roi Lipman <[email protected]> * Updates to Astyle config file (#1384) Co-authored-by: Roi Lipman <[email protected]> * 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 <[email protected]> Co-authored-by: Roi Lipman <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: swilly22 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * Release version 2.2.8. Merged from master. Co-authored-by: DvirDukhan <[email protected]> Co-authored-by: Roi Lipman <[email protected]> Co-authored-by: Jeffrey Lovitz <[email protected]> Co-authored-by: swilly22 <[email protected]> Co-authored-by: Guy Korland <[email protected]> Co-authored-by: Yahya <[email protected]> Co-authored-by: Simon Prickett <[email protected]>

* 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 <[email protected]>

swilly22 added 3 commits October 14, 2020 13:40

add graph version to graph context object

c7f88ee

change graph version from string to uint32

f0a556b

testing graph version

988b1e8

swilly22 added the bug label Oct 14, 2020

swilly22 requested review from jeffreylovitz and DvirDukhan October 14, 2020 19:38

swilly22 self-assigned this Oct 14, 2020

jeffreylovitz reviewed Oct 14, 2020

View reviewed changes

swilly22 and others added 2 commits November 5, 2020 15:21

response with an error when graph version mismatch

49c2bda

Merge branch 'master' into graph-version

9203e38

jeffreylovitz suggested changes Nov 5, 2020

View reviewed changes

swilly22 added 2 commits November 5, 2020 20:49

release graph context on version mismatch

08b2e06

Merge branch 'graph-version' of github.com:RedisGraph/RedisGraph into…

3ff8500

… graph-version

swilly22 added 3 commits November 7, 2020 22:04

address PR comments

44e2789

increase command arity to accomadate graph version argument

ae86e7f

address PR comments

489c6d2

DvirDukhan approved these changes Nov 8, 2020

View reviewed changes

Merge branch 'master' into graph-version

4fef806

swilly22 merged commit ea87640 into master Nov 8, 2020

swilly22 deleted the graph-version branch November 8, 2020 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Graph version #1382

Graph version #1382

Uh oh!

swilly22 commented Oct 14, 2020

Uh oh!

jeffreylovitz left a comment

Uh oh!

lgtm-com bot commented Nov 5, 2020

Uh oh!

jeffreylovitz left a comment

Uh oh!

jeffreylovitz Nov 5, 2020

Uh oh!

swilly22 Nov 7, 2020

Uh oh!

jeffreylovitz Nov 5, 2020

Uh oh!

swilly22 Nov 7, 2020

Uh oh!

jeffreylovitz Nov 5, 2020

Uh oh!

swilly22 Nov 7, 2020

Uh oh!

jeffreylovitz Nov 5, 2020 •

edited

Loading

Uh oh!

swilly22 Nov 7, 2020

Uh oh!

jeffreylovitz Nov 5, 2020

Uh oh!

jeffreylovitz Nov 5, 2020

Uh oh!

swilly22 Nov 7, 2020

Uh oh!

lgtm-com bot commented Nov 5, 2020

Uh oh!

Uh oh!

		@@ -72,6 +72,7 @@ void Query_SetTimeOut(uint timeout, ExecutionPlan *plan) {
		Cron_AddTask(timeout, QueryTimedOut, plan);
		}

Graph version #1382

Graph version #1382

Uh oh!

Conversation

swilly22 commented Oct 14, 2020

Uh oh!

jeffreylovitz left a comment

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Nov 5, 2020

Uh oh!

jeffreylovitz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffreylovitz Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Nov 5, 2020

Uh oh!

Uh oh!

jeffreylovitz Nov 5, 2020 •

edited

Loading