From a5b99a867fbef7411db690e6768759d75bf37ce1 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Mon, 16 Mar 2020 13:45:20 -0400 Subject: [PATCH 01/32] Enable TCK tests --- tests/tck/features/MatchAcceptance2.feature | 10 ---------- tests/tck/features/OptionalMatch.feature | 3 --- tests/tck/features/OptionalMatchAcceptance.feature | 10 ---------- 3 files changed, 23 deletions(-) diff --git a/tests/tck/features/MatchAcceptance2.feature b/tests/tck/features/MatchAcceptance2.feature index baa10e8f9b..11e127a96a 100644 --- a/tests/tck/features/MatchAcceptance2.feature +++ b/tests/tck/features/MatchAcceptance2.feature @@ -306,7 +306,6 @@ Feature: MatchAcceptance2 | ({name: 'e'}) | And no side effects -@skip Scenario: MATCH with OPTIONAL MATCH in longer pattern Given an empty graph And having executed: @@ -646,7 +645,6 @@ Feature: MatchAcceptance2 | null | And no side effects -@skip Scenario: OPTIONAL MATCH with previously bound nodes Given an empty graph And having executed: @@ -664,7 +662,6 @@ Feature: MatchAcceptance2 | () | null | And no side effects -@skip Scenario: `collect()` filtering nulls Given an empty graph And having executed: @@ -1102,7 +1099,6 @@ Feature: MatchAcceptance2 | [:T2 {id: 1}] | And no side effects -@skip Scenario: Matching with LIMIT and optionally matching using a relationship that is already bound Given an empty graph And having executed: @@ -1122,7 +1118,6 @@ Feature: MatchAcceptance2 | (:A) | [:T] | (:B) | And no side effects -@skip Scenario: Matching with LIMIT and optionally matching using a relationship and node that are both already bound Given an empty graph And having executed: @@ -1142,7 +1137,6 @@ Feature: MatchAcceptance2 | (:A) | [:T] | (:B) | And no side effects -@skip Scenario: Matching with LIMIT, then matching again using a relationship and node that are both already bound along with an additional predicate Given an empty graph And having executed: @@ -1239,7 +1233,6 @@ Feature: MatchAcceptance2 | (:A) | (:C) | And no side effects -@skip Scenario: Matching relationships into a list and matching variable length using the list, with bound nodes Given an empty graph And having executed: @@ -1261,7 +1254,6 @@ Feature: MatchAcceptance2 | (:A) | (:C) | And no side effects -@skip Scenario: Matching relationships into a list and matching variable length using the list, with bound nodes, wrong direction Given an empty graph And having executed: @@ -1282,7 +1274,6 @@ Feature: MatchAcceptance2 | first | second | And no side effects -@skip Scenario: Matching and optionally matching with bound nodes in reverse direction Given an empty graph And having executed: @@ -1681,7 +1672,6 @@ Feature: MatchAcceptance2 | null | And no side effects -@skip Scenario: Variable length patterns and nulls Given an empty graph And having executed: diff --git a/tests/tck/features/OptionalMatch.feature b/tests/tck/features/OptionalMatch.feature index 4b0780ab36..398105a33e 100644 --- a/tests/tck/features/OptionalMatch.feature +++ b/tests/tck/features/OptionalMatch.feature @@ -30,7 +30,6 @@ Feature: OptionalMatch -@skip Scenario: Satisfies the open world assumption, relationships between same nodes Given an empty graph And having executed: @@ -50,7 +49,6 @@ Feature: OptionalMatch | 1 | false | And no side effects -@skip Scenario: Satisfies the open world assumption, single relationship Given an empty graph And having executed: @@ -69,7 +67,6 @@ Feature: OptionalMatch | 1 | true | And no side effects -@skip Scenario: Satisfies the open world assumption, relationships between different nodes Given an empty graph And having executed: diff --git a/tests/tck/features/OptionalMatchAcceptance.feature b/tests/tck/features/OptionalMatchAcceptance.feature index bd843e01d7..33ba636c4f 100644 --- a/tests/tck/features/OptionalMatchAcceptance.feature +++ b/tests/tck/features/OptionalMatchAcceptance.feature @@ -42,7 +42,6 @@ Feature: OptionalMatchAcceptance (b)-[:LOOP]->(b) """ -@skip Scenario: Return null when no matches due to inline label predicate When executing query: """ @@ -96,7 +95,6 @@ Feature: OptionalMatchAcceptance | null | And no side effects -@skip Scenario: MATCH after OPTIONAL MATCH When executing query: """ @@ -138,7 +136,6 @@ Feature: OptionalMatchAcceptance | null | And no side effects -@skip Scenario: OPTIONAL MATCH and bound nodes When executing query: """ @@ -185,7 +182,6 @@ Feature: OptionalMatchAcceptance | null | And no side effects -@skip Scenario: Variable length optional relationships When executing query: """ @@ -201,7 +197,6 @@ Feature: OptionalMatchAcceptance | (:C) | And no side effects -@skip Scenario: Variable length optional relationships with length predicates When executing query: """ @@ -214,7 +209,6 @@ Feature: OptionalMatchAcceptance | null | And no side effects -@skip Scenario: Optionally matching self-loops When executing query: """ @@ -243,7 +237,6 @@ Feature: OptionalMatchAcceptance | null | And no side effects -@skip Scenario: Variable length optional relationships with bound nodes When executing query: """ @@ -269,7 +262,6 @@ Feature: OptionalMatchAcceptance | null | And no side effects -@skip Scenario: Longer pattern with bound nodes When executing query: """ @@ -282,7 +274,6 @@ Feature: OptionalMatchAcceptance | (:A {num: 42}) | And no side effects -@skip Scenario: Longer pattern with bound nodes without matches When executing query: """ @@ -295,7 +286,6 @@ Feature: OptionalMatchAcceptance | null | And no side effects -@skip Scenario: Handling correlated optional matches; first does not match implies second does not match When executing query: """ From 5325ad18047f8cfce34685f1384820400f7b3697 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Mon, 16 Mar 2020 13:50:52 -0400 Subject: [PATCH 02/32] Introduce Optional and Apply ops --- src/ast/ast_mock.c | 29 +++++- src/ast/ast_mock.h | 5 +- src/ast/ast_validations.c | 7 -- src/execution_plan/execution_plan.c | 30 ++++++ src/execution_plan/execution_plan_modify.c | 8 +- src/execution_plan/ops/op.h | 1 + src/execution_plan/ops/op_apply.c | 112 ++++++++++----------- src/execution_plan/ops/op_apply.h | 10 +- src/execution_plan/ops/op_optional.c | 49 +++++++++ src/execution_plan/ops/op_optional.h | 21 ++++ src/execution_plan/ops/ops.h | 2 + src/execution_plan/record.c | 2 + src/graph/query_graph.c | 2 + 13 files changed, 204 insertions(+), 74 deletions(-) create mode 100644 src/execution_plan/ops/op_optional.c create mode 100644 src/execution_plan/ops/op_optional.h diff --git a/src/ast/ast_mock.c b/src/ast/ast_mock.c index 2401cc3f76..a8c8963cf4 100644 --- a/src/ast/ast_mock.c +++ b/src/ast/ast_mock.c @@ -8,7 +8,7 @@ #include "../query_ctx.h" #include "../util/rmalloc.h" -AST *AST_MockMatchPattern(AST *master_ast, const cypher_astnode_t *original_path) { +AST *AST_MockMatchPath(AST *master_ast, const cypher_astnode_t *original_path) { AST *ast = rm_malloc(sizeof(AST)); ast->referenced_entities = master_ast->referenced_entities; ast->anot_ctx_collection = master_ast->anot_ctx_collection; @@ -35,6 +35,31 @@ AST *AST_MockMatchPattern(AST *master_ast, const cypher_astnode_t *original_path return ast; } +AST *AST_MockOptionalMatch(AST *master_ast, cypher_astnode_t *clause) { + AST *ast = rm_malloc(sizeof(AST)); + ast->referenced_entities = master_ast->referenced_entities; + ast->anot_ctx_collection = master_ast->anot_ctx_collection; + ast->free_root = true; + ast->limit = UNLIMITED; + + // TODO just need to turn off clause->optional, should be better options. + struct cypher_input_range range = {}; + const cypher_astnode_t *pattern = cypher_ast_match_get_pattern(clause); + const cypher_astnode_t *predicate = cypher_ast_match_get_predicate(clause); + uint child_count = cypher_astnode_nchildren(clause); + const cypher_astnode_t *first_child = cypher_astnode_get_child(clause, 0); + const cypher_astnode_t **children = &first_child; + cypher_astnode_t *clone = cypher_ast_match(false, pattern, NULL, 0, predicate, + (cypher_astnode_t **)children, child_count, range); + + // TODO tmp, bad, unsafe + ast->root = cypher_ast_query(NULL, 0, &clone, 1, &clone, 1, range); + + QueryCtx_SetAST(ast); // Update the TLS. + + return ast; +} + void AST_MockFree(AST *ast) { /* When freeing the mock AST, we have to be careful to not free the shared path * or its annotations. We'll free every surrounding layer explicitly - the MATCH @@ -42,7 +67,7 @@ void AST_MockFree(AST *ast) { const cypher_astnode_t *clause = cypher_ast_query_get_clause(ast->root, 0); assert(cypher_astnode_type(clause) == CYPHER_AST_MATCH); const cypher_astnode_t *pattern = cypher_ast_match_get_pattern(clause); - cypher_astnode_free((cypher_astnode_t *)pattern); + // cypher_astnode_free((cypher_astnode_t *)pattern); // TODO tmp cypher_astnode_free((cypher_astnode_t *)clause); cypher_astnode_free((cypher_astnode_t *)ast->root); rm_free(ast); diff --git a/src/ast/ast_mock.h b/src/ast/ast_mock.h index ae9a325af8..5e70c3cff1 100644 --- a/src/ast/ast_mock.h +++ b/src/ast/ast_mock.h @@ -9,7 +9,10 @@ #include "ast.h" // Build a temporary AST with one MATCH clause that holds the given path. -AST *AST_MockMatchPattern(AST *master_ast, const cypher_astnode_t *original_path); +AST *AST_MockMatchPath(AST *master_ast, const cypher_astnode_t *original_path); + +// TODO improve name +AST *AST_MockOptionalMatch(AST *master_ast, cypher_astnode_t *clause); // Free a temporary AST. void AST_MockFree(AST *ast); diff --git a/src/ast/ast_validations.c b/src/ast/ast_validations.c index 598a54ccc7..f891a360f9 100644 --- a/src/ast/ast_validations.c +++ b/src/ast/ast_validations.c @@ -617,13 +617,6 @@ static AST_Validation _Validate_MATCH_Clauses(const AST *ast, char **reason) { uint match_count = array_len(match_clauses); for(uint i = 0; i < match_count; i ++) { const cypher_astnode_t *match_clause = match_clauses[i]; - // We currently do not support optional MATCH. - if(cypher_ast_match_is_optional(match_clause)) { - asprintf(reason, "RedisGraph does not support OPTIONAL MATCH."); - res = AST_INVALID; - goto cleanup; - } - // Validate the pattern described by the MATCH clause res = _ValidatePattern(projections, cypher_ast_match_get_pattern(match_clause), edge_aliases, reason); diff --git a/src/execution_plan/execution_plan.c b/src/execution_plan/execution_plan.c index 87e21d5908..1c009a2ec3 100644 --- a/src/execution_plan/execution_plan.c +++ b/src/execution_plan/execution_plan.c @@ -527,6 +527,32 @@ static void _buildMergeCreateStream(ExecutionPlan *plan, AST_MergeContext *merge } } +static void _BuildOptionalMatchOps(ExecutionPlan *plan, const cypher_astnode_t *clause) { + // TODO dup of _buildMergeOp logic, consider shared function. + // Collect the variables that are bound at this point. + rax *bound_vars = NULL; + const char **arguments = NULL; + if(plan->root) { + bound_vars = raxNew(); + // Rather than cloning the record map, collect the bound variables along with their + // parser-generated constant strings. + ExecutionPlan_BoundVariables(plan->root, bound_vars); + // Collect the variable names from bound_vars to populate the Argument ops we will build. + arguments = (const char **)raxValues(bound_vars); + } + + OpBase *apply_op = NewApplyOp(plan); + _ExecutionPlan_UpdateRoot(plan, apply_op); + + OpBase *optional = NewOptionalOp(plan); + ExecutionPlan_AddOp(apply_op, optional); + OpBase *match_stream = ExecutionPlan_BuildOpsFromPath(plan, arguments, clause); + ExecutionPlan_AddOp(optional, match_stream); // Add Match stream to Merge op. + + if(bound_vars) raxFree(bound_vars); + array_free(arguments); +} + static void _buildMergeOp(GraphContext *gc, AST *ast, ExecutionPlan *plan, const cypher_astnode_t *clause) { /* @@ -608,6 +634,10 @@ static void _ExecutionPlanSegment_ConvertClause(GraphContext *gc, AST *ast, Exec cypher_astnode_type_t t = cypher_astnode_type(clause); // Because 't' is set using the offsetof() call, it cannot be used in switch statements. if(t == CYPHER_AST_MATCH) { + if(cypher_ast_match_is_optional(clause)) { + _BuildOptionalMatchOps(plan, clause); + return; + } // Only add at most one set of traversals per plan. TODO Revisit and improve this logic. if(plan->root && ExecutionPlan_LocateOpMatchingType(plan->root, SCAN_OPS, SCAN_OP_COUNT)) { return; diff --git a/src/execution_plan/execution_plan_modify.c b/src/execution_plan/execution_plan_modify.c index 2b7834bd81..95d992f69a 100644 --- a/src/execution_plan/execution_plan_modify.c +++ b/src/execution_plan/execution_plan_modify.c @@ -316,7 +316,13 @@ OpBase *ExecutionPlan_BuildOpsFromPath(ExecutionPlan *plan, const char **bound_v AST *ast = QueryCtx_GetAST(); // Build a temporary AST holding a MATCH clause. - AST *match_stream_ast = AST_MockMatchPattern(ast, path); + // TODO tmp + AST *match_stream_ast; + if(cypher_astnode_type(path) == CYPHER_AST_MATCH) { + match_stream_ast = AST_MockOptionalMatch(ast, (cypher_astnode_t *)path); + } else { + match_stream_ast = AST_MockMatchPath(ast, path); + } ExecutionPlan_PopulateExecutionPlan(match_stream_plan); diff --git a/src/execution_plan/ops/op.h b/src/execution_plan/ops/op.h index 4d06651fa6..df6d8741aa 100644 --- a/src/execution_plan/ops/op.h +++ b/src/execution_plan/ops/op.h @@ -50,6 +50,7 @@ typedef enum { OpType_ANTI_SEMI_APPLY, OPType_OR_APPLY_MULTIPLEXER, OPType_AND_APPLY_MULTIPLEXER, + OPType_OPTIONAL, } OPType; typedef enum { diff --git a/src/execution_plan/ops/op_apply.c b/src/execution_plan/ops/op_apply.c index 27482e75e9..c4d8336004 100644 --- a/src/execution_plan/ops/op_apply.c +++ b/src/execution_plan/ops/op_apply.c @@ -7,97 +7,91 @@ #include "op_apply.h" /* Forward declarations. */ +static OpResult ApplyInit(OpBase *opBase); static Record ApplyConsume(OpBase *opBase); static OpResult ApplyReset(OpBase *opBase); static void ApplyFree(OpBase *opBase); OpBase *NewApplyOp(const ExecutionPlan *plan) { Apply *op = rm_malloc(sizeof(Apply)); - op->init = true; - op->rhs_idx = 0; - op->lhs_record = NULL; - op->rhs_records = array_new(Record, 32); + op->r = NULL; + op->op_arg = NULL; + op->bound_branch = NULL; + op->rhs_branch = NULL; // Set our Op operations - OpBase_Init((OpBase *)op, OPType_APPLY, "Apply", NULL, ApplyConsume, ApplyReset, NULL, NULL, + OpBase_Init((OpBase *)op, OPType_APPLY, "Apply", ApplyInit, ApplyConsume, ApplyReset, NULL, NULL, ApplyFree, false, plan); return (OpBase *)op; } -static Record ApplyConsume(OpBase *opBase) { - Apply *op = (Apply *)opBase; +static OpResult ApplyInit(OpBase *opBase) { + // TODO init routine necesssary? + assert(opBase->childCount == 2); - assert(op->op.childCount == 2); + Apply *op = (Apply *)opBase; + /* The op bounded branch and match branch are set to be the first and second child, respectively, + * during the operation building procedure at execution_plan_reduce_to_apply.c */ + op->bound_branch = opBase->children[0]; + op->rhs_branch = opBase->children[1]; + + // Locate branch's Argument op tap. + op->op_arg = (Argument *)ExecutionPlan_LocateOp(op->rhs_branch, OPType_ARGUMENT); + assert(op->op_arg && op->op_arg->op.childCount == 0); // TODO maybe wrong + return OP_OK; +} - Record rhs_record; +static Record ApplyConsume(OpBase *opBase) { + Apply *op = (Apply *)opBase; - if(op->init) { - // On the first call to ApplyConsume, we'll read the entirety of the - // right-hand stream and buffer its outputs. - op->init = false; + while(true) { + if(op->r == NULL) { + // Retrieve a Record from the bound branch if we're not currently holding one. + op->r = OpBase_Consume(op->bound_branch); + if(!op->r) return NULL; // Bound branch and this op are depleted. - OpBase *rhs = op->op.children[1]; - while((rhs_record = rhs->consume(rhs))) { - op->rhs_records = array_append(op->rhs_records, rhs_record); + // Successfully pulled a new Record, propagate to the top of the RHS branch. + if(op->op_arg) Argument_AddRecord(op->op_arg, OpBase_CloneRecord(op->r)); } - } - uint rhs_count = array_len(op->rhs_records); - - OpBase *lhs = op->op.children[0]; - if(op->lhs_record == NULL) { - // No pending data from left-hand stream - op->rhs_idx = 0; - op->lhs_record = lhs->consume(lhs); - } - - if(op->lhs_record == NULL) { - // Left-hand stream has been fully depleted - return NULL; - } - - // Clone the left-hand record - Record r = OpBase_CloneRecord(op->lhs_record); - - // No records were produced by the right-hand stream - if(rhs_count == 0) return r; + // Pull a Record from the RHS branch. + Record rhs_record = OpBase_Consume(op->rhs_branch); + + if(rhs_record == NULL) { + /* RHS branch depleted for the current bound Record; + * free it and loop back to retrieve a new one. */ + OpBase_DeleteRecord(op->r); + op->r = NULL; + // Reset the RHS branch. + OpBase_PropagateReset(op->rhs_branch); + continue; + } - rhs_record = op->rhs_records[op->rhs_idx++]; + // Clone the bound Record and merge the RHS Record into it. + Record r = OpBase_CloneRecord(op->r); + Record_Merge(&r, rhs_record); - if(op->rhs_idx == rhs_count) { - // We've joined all data from the right-hand stream with the current - // retrieval from the left-hand stream. - // The next call to ApplyConsume will attempt to pull new data. - OpBase_DeleteRecord(op->lhs_record); - op->lhs_record = NULL; - op->rhs_idx = 0; + return r; } - Record_Merge(&r, rhs_record); - - return r; + return NULL; } static OpResult ApplyReset(OpBase *opBase) { Apply *op = (Apply *)opBase; - op->init = true; + if(op->r) { + OpBase_DeleteRecord(op->r); + op->r = NULL; + } return OP_OK; } static void ApplyFree(OpBase *opBase) { Apply *op = (Apply *)opBase; - if(op->lhs_record) { - OpBase_DeleteRecord(op->lhs_record); - op->lhs_record = NULL; - } - if(op->rhs_records) { - uint len = array_len(op->rhs_records); - for(uint i = 0; i < len; i ++) { - OpBase_DeleteRecord(op->rhs_records[i]); - } - array_free(op->rhs_records); - op->rhs_records = NULL; + if(op->r) { + OpBase_DeleteRecord(op->r); + op->r = NULL; } } diff --git a/src/execution_plan/ops/op_apply.h b/src/execution_plan/ops/op_apply.h index e2cf5c8384..afb2347e08 100644 --- a/src/execution_plan/ops/op_apply.h +++ b/src/execution_plan/ops/op_apply.h @@ -7,15 +7,17 @@ #pragma once #include "op.h" +#include "op_argument.h" #include "../execution_plan.h" typedef struct { OpBase op; - bool init; - uint rhs_idx; - Record lhs_record; - Record *rhs_records; + Record r; // Bound branch record. + OpBase *bound_branch; // Bound branch. + OpBase *rhs_branch; // Right-hand branch. + Argument *op_arg; // Right-hand branch tap. } Apply; // OpBase* NewApplyOp(uint *modifies); OpBase *NewApplyOp(const ExecutionPlan *plan); + diff --git a/src/execution_plan/ops/op_optional.c b/src/execution_plan/ops/op_optional.c new file mode 100644 index 0000000000..2b6f55bbed --- /dev/null +++ b/src/execution_plan/ops/op_optional.c @@ -0,0 +1,49 @@ +/* + * Copyright 2018-2020 Redis Labs Ltd. and Contributors + * + * This file is available under the Redis Labs Source Available License Agreement + */ + +#include "op_optional.h" + +/* Forward declarations. */ +static Record OptionalConsume(OpBase *opBase); +static OpResult OptionalReset(OpBase *opBase); +static OpBase *OptionalClone(const ExecutionPlan *plan, const OpBase *opBase); +static void OptionalFree(OpBase *opBase); + +OpBase *NewOptionalOp(const ExecutionPlan *plan) { + Optional *op = rm_malloc(sizeof(Optional)); + op->emitted_record = false; + + // Set our Op operations + OpBase_Init((OpBase *)op, OPType_OPTIONAL, "Optional", NULL, OptionalConsume, + OptionalReset, NULL, OptionalClone, OptionalFree, false, plan); + + return (OpBase *)op; +} + +static Record OptionalConsume(OpBase *opBase) { + Optional *op = (Optional *)opBase; + Record r = OpBase_Consume(opBase->children[0]); + if(!r && op->emitted_record == false) r = OpBase_CreateRecord(opBase); + op->emitted_record = true; + + return r; +} + +static OpResult OptionalReset(OpBase *opBase) { + Optional *op = (Optional *)opBase; + op->emitted_record = false; + return OP_OK; +} + +static inline OpBase *OptionalClone(const ExecutionPlan *plan, const OpBase *opBase) { + return NewOptionalOp(plan); +} + +static void OptionalFree(OpBase *ctx) { + Optional *op = (Optional *)ctx; + op->emitted_record = false; +} + diff --git a/src/execution_plan/ops/op_optional.h b/src/execution_plan/ops/op_optional.h new file mode 100644 index 0000000000..6db31c2d2c --- /dev/null +++ b/src/execution_plan/ops/op_optional.h @@ -0,0 +1,21 @@ +/* + * Copyright 2018-2020 Redis Labs Ltd. and Contributors + * + * This file is available under the Redis Labs Source Available License Agreement + */ + +#pragma once + +#include "op.h" +#include "../execution_plan.h" + +/* If Optional's child produces records, Optional emits them without modification. + * If the child produces no records, Optional emits an empty Record exactly once. */ +typedef struct { + OpBase op; + bool emitted_record; +} Optional; + +/* Creates a new Optional operation. */ +OpBase *NewOptionalOp(const ExecutionPlan *plan); + diff --git a/src/execution_plan/ops/ops.h b/src/execution_plan/ops/ops.h index fa1bfb530d..14e1b4a717 100644 --- a/src/execution_plan/ops/ops.h +++ b/src/execution_plan/ops/ops.h @@ -36,3 +36,5 @@ #include "op_merge_create.h" #include "op_semi_apply.h" #include "op_apply_multiplexer.h" +#include "op_optional.h" + diff --git a/src/execution_plan/record.c b/src/execution_plan/record.c index 9b7dbae3b9..1d95c0f31a 100644 --- a/src/execution_plan/record.c +++ b/src/execution_plan/record.c @@ -112,6 +112,8 @@ SIValue Record_Get(Record r, int idx) { return SI_Edge(Record_GetEdge(r, idx)); case REC_TYPE_SCALAR: return Record_GetScalar(r, idx); + case REC_TYPE_UNKNOWN: + return SI_NullVal(); default: assert(false); } diff --git a/src/graph/query_graph.c b/src/graph/query_graph.c index addcdeb0a3..d9da8e7465 100644 --- a/src/graph/query_graph.c +++ b/src/graph/query_graph.c @@ -155,6 +155,8 @@ QueryGraph *BuildQueryGraph(const GraphContext *gc, const AST *ast) { if(match_clauses) { uint match_count = array_len(match_clauses); for(uint i = 0; i < match_count; i ++) { + // OPTIONAL MATCH clauses are handled separately. + if(cypher_ast_match_is_optional(match_clauses[i])) continue; const cypher_astnode_t *pattern = cypher_ast_match_get_pattern(match_clauses[i]); uint npaths = cypher_ast_pattern_npaths(pattern); for(uint j = 0; j < npaths; j ++) { From ec9371abda3ff0f43a01db2454e58e49a7fbb3a1 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Mon, 16 Mar 2020 15:39:43 -0400 Subject: [PATCH 03/32] Modify mock AST logic --- src/Makefile | 2 +- src/ast/ast_mock.c | 5 +++-- src/graph/query_graph.c | 1 + tests/tck/features/MatchAcceptance2.feature | 2 -- tests/tck/features/OptionalMatchAcceptance.feature | 1 - 5 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/Makefile b/src/Makefile index f1f750d93d..69178f558b 100644 --- a/src/Makefile +++ b/src/Makefile @@ -154,7 +154,7 @@ ifeq (,$(wildcard $(LIBCYPHER-PARSER))) cd ../deps/libcypher-parser; \ ./autogen.sh; \ ./configure --disable-shared; - $(MAKE) CFLAGS="-fPIC -DYY_BUFFER_SIZE=1048576" clean check -C ../deps/libcypher-parser + $(MAKE) CFLAGS="-g -fPIC -DYY_BUFFER_SIZE=1048576" clean check -C ../deps/libcypher-parser endif .PHONY: $(LIBCYPHER-PARSER) diff --git a/src/ast/ast_mock.c b/src/ast/ast_mock.c index a8c8963cf4..10a109a2cd 100644 --- a/src/ast/ast_mock.c +++ b/src/ast/ast_mock.c @@ -47,13 +47,14 @@ AST *AST_MockOptionalMatch(AST *master_ast, cypher_astnode_t *clause) { const cypher_astnode_t *pattern = cypher_ast_match_get_pattern(clause); const cypher_astnode_t *predicate = cypher_ast_match_get_predicate(clause); uint child_count = cypher_astnode_nchildren(clause); - const cypher_astnode_t *first_child = cypher_astnode_get_child(clause, 0); - const cypher_astnode_t **children = &first_child; + const cypher_astnode_t *children[child_count]; + for(uint i = 0; i < child_count; i ++) children[i] = cypher_astnode_get_child(clause, i); cypher_astnode_t *clone = cypher_ast_match(false, pattern, NULL, 0, predicate, (cypher_astnode_t **)children, child_count, range); // TODO tmp, bad, unsafe ast->root = cypher_ast_query(NULL, 0, &clone, 1, &clone, 1, range); + // ast->root = cypher_ast_query(NULL, 0, &clause, 1, &clause, 1, range); QueryCtx_SetAST(ast); // Update the TLS. diff --git a/src/graph/query_graph.c b/src/graph/query_graph.c index d9da8e7465..28904ced4a 100644 --- a/src/graph/query_graph.c +++ b/src/graph/query_graph.c @@ -157,6 +157,7 @@ QueryGraph *BuildQueryGraph(const GraphContext *gc, const AST *ast) { for(uint i = 0; i < match_count; i ++) { // OPTIONAL MATCH clauses are handled separately. if(cypher_ast_match_is_optional(match_clauses[i])) continue; + // if(match_count > 1 && cypher_ast_match_is_optional(match_clauses[i])) continue; const cypher_astnode_t *pattern = cypher_ast_match_get_pattern(match_clauses[i]); uint npaths = cypher_ast_pattern_npaths(pattern); for(uint j = 0; j < npaths; j ++) { diff --git a/tests/tck/features/MatchAcceptance2.feature b/tests/tck/features/MatchAcceptance2.feature index 11e127a96a..b3c916184f 100644 --- a/tests/tck/features/MatchAcceptance2.feature +++ b/tests/tck/features/MatchAcceptance2.feature @@ -426,7 +426,6 @@ Feature: MatchAcceptance2 | (:B {id: 2}) | And no side effects -@skip Scenario: Do not fail when predicates on optionally matched and missed nodes are invalid Given an empty graph And having executed: @@ -538,7 +537,6 @@ Feature: MatchAcceptance2 | b | And no side effects -@skip Scenario: Variable length relationship in OPTIONAL MATCH Given an empty graph And having executed: diff --git a/tests/tck/features/OptionalMatchAcceptance.feature b/tests/tck/features/OptionalMatchAcceptance.feature index 33ba636c4f..a40fc01be8 100644 --- a/tests/tck/features/OptionalMatchAcceptance.feature +++ b/tests/tck/features/OptionalMatchAcceptance.feature @@ -68,7 +68,6 @@ Feature: OptionalMatchAcceptance | null | And no side effects -@skip Scenario: Respect predicates on the OPTIONAL MATCH When executing query: """ From f95e5ac81d16376bff21450b9e0a627f7dc54716 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Wed, 18 Mar 2020 11:54:34 -0400 Subject: [PATCH 04/32] Emit error on queries beginning with OPTIONAL MATCH --- src/ast/ast_validations.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/ast/ast_validations.c b/src/ast/ast_validations.c index f891a360f9..8ff16b9e51 100644 --- a/src/ast/ast_validations.c +++ b/src/ast/ast_validations.c @@ -1012,6 +1012,15 @@ static AST_Validation _ValidateQuerySequence(const AST *ast, char **reason) { return AST_INVALID; } + /* Disallow queries beginning with an OPTIONAL MATCH. + * Cypher does allow this construction, but it is considered an antipattern + * and introduces a lot of edge cases. We can add support for this later if desired. */ + if(cypher_astnode_type(start_clause) == CYPHER_AST_MATCH && + cypher_ast_match_is_optional(start_clause)) { + asprintf(reason, "RedisGraph does not support beginning a query with OPTIONAL MATCH."); + return AST_INVALID; + } + return AST_VALID; } From 6330061e43271c652e706b3e1682d3e702b49320 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Wed, 18 Mar 2020 11:59:20 -0400 Subject: [PATCH 05/32] Return null on property accesses of null graph entity --- src/arithmetic/arithmetic_expression.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/arithmetic/arithmetic_expression.c b/src/arithmetic/arithmetic_expression.c index 8ac24a8961..3fd9e2fbd6 100644 --- a/src/arithmetic/arithmetic_expression.c +++ b/src/arithmetic/arithmetic_expression.c @@ -344,8 +344,12 @@ static bool _AR_EXP_UpdateEntityIdx(AR_OperandNode *node, const Record r) { static AR_EXP_Result _AR_EXP_EvaluateProperty(AR_ExpNode *node, const Record r, SIValue *result) { RecordEntryType t = Record_GetType(r, node->operand.variadic.entity_alias_idx); - // Property requested on a scalar value. - if(!(t & (REC_TYPE_NODE | REC_TYPE_EDGE))) { + if(t == REC_TYPE_UNKNOWN) { + /* If we attempt to access a null value as a graph entity (due to a + * scenario like a failed OPTIONAL MATCH), return a null value. */ + *result = SI_NullVal(); + return EVAL_OK; + } else if(!(t & (REC_TYPE_NODE | REC_TYPE_EDGE))) { /* Attempted to access a scalar value as a map. * Set an error and invoke the exception handler. */ char *error; From 85e7368a867a9564a208d517d6dae94e57028890 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Wed, 18 Mar 2020 13:46:42 -0400 Subject: [PATCH 06/32] Disallow OPTIONAL MATCH...MATCH queries --- src/ast/ast_validations.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/ast/ast_validations.c b/src/ast/ast_validations.c index 8ff16b9e51..c05af7312a 100644 --- a/src/ast/ast_validations.c +++ b/src/ast/ast_validations.c @@ -1027,9 +1027,11 @@ static AST_Validation _ValidateQuerySequence(const AST *ast, char **reason) { // In any given query scope, reading clauses (MATCH, UNWIND, and InQueryCall) // cannot follow updating clauses (CREATE, MERGE, DELETE, SET, REMOVE). // https://s3.amazonaws.com/artifacts.opencypher.org/railroad/SinglePartQuery.html +// Additionally, a MATCH clause cannot follow an optional MATCH clause. static AST_Validation _ValidateClauseOrder(const AST *ast, char **reason) { uint clause_count = cypher_ast_query_nclauses(ast->root); + bool encountered_optional_match = false; bool encountered_updating_clause = false; for(uint i = 0; i < clause_count; i ++) { const cypher_astnode_t *clause = cypher_ast_query_get_clause(ast->root, i); @@ -1045,6 +1047,17 @@ static AST_Validation _ValidateClauseOrder(const AST *ast, char **reason) { cypher_astnode_typestr(type)); return AST_INVALID; } + + if(type == CYPHER_AST_MATCH) { + // Check whether this match is optional. + bool current_clause_is_optional = cypher_ast_match_is_optional(clause); + // If it is not and we have already processed an optional match, emit an error. + if(!current_clause_is_optional && encountered_optional_match) { + asprintf(reason, "A WITH clause is required to introduce a MATCH clause after an OPTIONAL MATCH."); + return AST_INVALID; + } + encountered_optional_match |= current_clause_is_optional; + } } return AST_VALID; From d2d7a656d391f270a1592f6c11bca60c85c02ad2 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Wed, 18 Mar 2020 13:47:02 -0400 Subject: [PATCH 07/32] Fix OPTIONAL filter placement --- src/ast/ast_build_filter_tree.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/ast/ast_build_filter_tree.c b/src/ast/ast_build_filter_tree.c index debf0f1a53..057c649f37 100644 --- a/src/ast/ast_build_filter_tree.c +++ b/src/ast/ast_build_filter_tree.c @@ -319,6 +319,8 @@ FT_FilterNode *AST_BuildFilterTree(AST *ast) { if(match_clauses) { uint match_count = array_len(match_clauses); for(uint i = 0; i < match_count; i ++) { + // Optional match clauses are handled separately. + if(cypher_ast_match_is_optional(match_clauses[i])) continue; const cypher_astnode_t *pattern = cypher_ast_match_get_pattern(match_clauses[i]); _AST_ConvertGraphPatternToFilter(ast, &filter_tree, pattern); const cypher_astnode_t *predicate = cypher_ast_match_get_predicate(match_clauses[i]); @@ -362,3 +364,4 @@ FT_FilterNode *AST_BuildFilterTree(AST *ast) { return filter_tree; } + From 67ae0239652f7cd192237b2c4fec323f5d11bfe8 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Wed, 18 Mar 2020 14:59:27 -0400 Subject: [PATCH 08/32] Enable TCK tests --- tests/tck/features/MatchAcceptance2.feature | 3 --- tests/tck/features/OptionalMatchAcceptance.feature | 3 --- 2 files changed, 6 deletions(-) diff --git a/tests/tck/features/MatchAcceptance2.feature b/tests/tck/features/MatchAcceptance2.feature index b3c916184f..036e754bf3 100644 --- a/tests/tck/features/MatchAcceptance2.feature +++ b/tests/tck/features/MatchAcceptance2.feature @@ -325,7 +325,6 @@ Feature: MatchAcceptance2 | ({name: 'C'}) | And no side effects -@skip Scenario: Optionally matching named paths Given an empty graph And having executed: @@ -346,7 +345,6 @@ Feature: MatchAcceptance2 | ({name: 'C'}) | null | And no side effects -@skip Scenario: Optionally matching named paths with single and variable length patterns Given an empty graph And having executed: @@ -1291,7 +1289,6 @@ Feature: MatchAcceptance2 | (:A) | [:T] | null | And no side effects -@skip Scenario: Matching and optionally matching with unbound nodes and equality predicate in reverse direction Given an empty graph And having executed: diff --git a/tests/tck/features/OptionalMatchAcceptance.feature b/tests/tck/features/OptionalMatchAcceptance.feature index a40fc01be8..40dbc0c28a 100644 --- a/tests/tck/features/OptionalMatchAcceptance.feature +++ b/tests/tck/features/OptionalMatchAcceptance.feature @@ -122,7 +122,6 @@ Feature: OptionalMatchAcceptance | (:A {num: 42}) | (:B {num: 46}) | And no side effects -@skip Scenario: Named paths in optional matches When executing query: """ @@ -168,7 +167,6 @@ Feature: OptionalMatchAcceptance | (:Y:Z) | And no side effects -@skip Scenario: Named paths inside optional matches with node predicates When executing query: """ @@ -248,7 +246,6 @@ Feature: OptionalMatchAcceptance | (:C) | And no side effects -@skip Scenario: Variable length optional relationships with bound nodes, no matches When executing query: """ From 96af9a1ead55e6b6561f226d0427c31870b42a78 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Wed, 25 Mar 2020 09:34:04 -0400 Subject: [PATCH 09/32] NULL handling for path functions --- src/arithmetic/path_funcs/path_funcs.c | 18 ++++++++++++++---- src/datatypes/path/sipath_builder.c | 5 +++++ src/datatypes/path/sipath_builder.h | 3 +++ tests/tck/features/MatchAcceptance.feature | 1 - .../features/OptionalMatchAcceptance.feature | 3 --- 5 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/arithmetic/path_funcs/path_funcs.c b/src/arithmetic/path_funcs/path_funcs.c index 05475fbb0e..5a0ca11320 100644 --- a/src/arithmetic/path_funcs/path_funcs.c +++ b/src/arithmetic/path_funcs/path_funcs.c @@ -26,6 +26,12 @@ SIValue AR_TOPATH(SIValue *argv, int argc) { SIValue path = SIPathBuilder_New(nelements); for(uint i = 0; i < nelements; i++) { SIValue element = argv[i + 1]; + if(element.type == T_NULL) { + // If any element of the path does not exist, the entire path is invalid and should be cleared. + SIPathBuilder_ClearPath(path); + return SI_NullVal(); + } + if(i % 2 == 0) { // Nodes are in even position. SIPathBuilder_AppendNode(path, element); @@ -55,14 +61,17 @@ SIValue AR_TOPATH(SIValue *argv, int argc) { } SIValue AR_PATH_NODES(SIValue *argv, int argc) { + if(argv[0].type == T_NULL) return SI_NullVal(); return SIPath_Nodes(argv[0]); } SIValue AR_PATH_RELATIONSHIPS(SIValue *argv, int argc) { + if(argv[0].type == T_NULL) return SI_NullVal(); return SIPath_Relationships(argv[0]); } SIValue AR_PATH_LENGTH(SIValue *argv, int argc) { + if(argv[0].type == T_NULL) return SI_NullVal(); return SI_LongVal(SIPath_Length(argv[0])); } @@ -72,22 +81,23 @@ void Register_PathFuncs() { types = array_new(SIType, 2); types = array_append(types, T_PTR); - types = array_append(types, T_NODE | T_EDGE | T_PATH); + types = array_append(types, T_NULL | T_NODE | T_EDGE | T_PATH); func_desc = AR_FuncDescNew("topath", AR_TOPATH, 1, VAR_ARG_LEN, types, false); AR_RegFunc(func_desc); types = array_new(SIType, 1); - types = array_append(types, T_PATH); + types = array_append(types, T_NULL | T_PATH); func_desc = AR_FuncDescNew("nodes", AR_PATH_NODES, 1, 1, types, false); AR_RegFunc(func_desc); types = array_new(SIType, 1); - types = array_append(types, T_PATH); + types = array_append(types, T_NULL | T_PATH); func_desc = AR_FuncDescNew("relationships", AR_PATH_RELATIONSHIPS, 1, 1, types, false); AR_RegFunc(func_desc); types = array_new(SIType, 1); - types = array_append(types, T_PATH); + types = array_append(types, T_NULL | T_PATH); func_desc = AR_FuncDescNew("length", AR_PATH_LENGTH, 1, 1, types, false); AR_RegFunc(func_desc); } + diff --git a/src/datatypes/path/sipath_builder.c b/src/datatypes/path/sipath_builder.c index 32e14306f6..3cafc7efa3 100644 --- a/src/datatypes/path/sipath_builder.c +++ b/src/datatypes/path/sipath_builder.c @@ -91,3 +91,8 @@ void SIPathBuilder_AppendPath(SIValue path, SIValue new_path, bool RTLEdge) { } SIPathBuilder_AppendEdge(path, SIPath_GetRelationship(new_path, new_path_edge_count - 1), RTLEdge); } + +void SIPathBuilder_ClearPath(SIValue path) { + Path_Free(path.ptrval); +} + diff --git a/src/datatypes/path/sipath_builder.h b/src/datatypes/path/sipath_builder.h index 97f6246d0b..7c44e512c2 100644 --- a/src/datatypes/path/sipath_builder.h +++ b/src/datatypes/path/sipath_builder.h @@ -58,3 +58,6 @@ void SIPathBuilder_AppendEdge(SIValue p, SIValue e, bool RTLEdge); * @param RTLEdge: Indicates edges direction if the path (RTL in query, incoming or outgoing). */ void SIPathBuilder_AppendPath(SIValue p, SIValue other, bool RTLEdge); + +void SIPathBuilder_ClearPath(SIValue path); + diff --git a/tests/tck/features/MatchAcceptance.feature b/tests/tck/features/MatchAcceptance.feature index 46b97357b8..6f16069d8f 100644 --- a/tests/tck/features/MatchAcceptance.feature +++ b/tests/tck/features/MatchAcceptance.feature @@ -272,7 +272,6 @@ Feature: MatchAcceptance | (:A {num: 1}) | (:B {num: 2}) | And no side effects -@skip Scenario: Return two subgraphs with bound undirected relationship and optional relationship Given an empty graph And having executed: diff --git a/tests/tck/features/OptionalMatchAcceptance.feature b/tests/tck/features/OptionalMatchAcceptance.feature index 40dbc0c28a..e6ce413bbe 100644 --- a/tests/tck/features/OptionalMatchAcceptance.feature +++ b/tests/tck/features/OptionalMatchAcceptance.feature @@ -345,7 +345,6 @@ Feature: OptionalMatchAcceptance | [] | [42, 43, 44] | And no side effects -@skip Scenario: OPTIONAL MATCH and WHERE And having executed: """ @@ -368,7 +367,6 @@ Feature: OptionalMatchAcceptance | (:X {val: 6}) | null | And no side effects -@skip Scenario: OPTIONAL MATCH on two relationships and WHERE And having executed: """ @@ -391,7 +389,6 @@ Feature: OptionalMatchAcceptance | (:X {val: 6}) | null | null | And no side effects -@skip Scenario: Two OPTIONAL MATCH clauses and WHERE And having executed: """ From cf789b9fbfc37168e46f3b7fc6f829de9c6cc663 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Wed, 25 Mar 2020 09:53:56 -0400 Subject: [PATCH 10/32] NULL handling for GraphEntity and list functions --- src/arithmetic/entity_funcs/entity_funcs.c | 20 ++++++++++++------- src/arithmetic/list_funcs/list_funcs.c | 3 +++ .../tck/features/FunctionsAcceptance.feature | 3 --- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/arithmetic/entity_funcs/entity_funcs.c b/src/arithmetic/entity_funcs/entity_funcs.c index 0e807374af..3ace5d03b0 100644 --- a/src/arithmetic/entity_funcs/entity_funcs.c +++ b/src/arithmetic/entity_funcs/entity_funcs.c @@ -21,6 +21,7 @@ SIValue AR_ID(SIValue *argv, int argc) { /* returns a string representations the label of a node. */ SIValue AR_LABELS(SIValue *argv, int argc) { + if(argv[0].type == T_NULL) return SI_NullVal(); char *label = ""; Node *node = argv[0].ptrval; GraphContext *gc = QueryCtx_GetGraphCtx(); @@ -32,6 +33,7 @@ SIValue AR_LABELS(SIValue *argv, int argc) { /* returns a string representation of the type of a relation. */ SIValue AR_TYPE(SIValue *argv, int argc) { + if(argv[0].type == T_NULL) return SI_NullVal(); char *type = ""; Edge *e = argv[0].ptrval; GraphContext *gc = QueryCtx_GetGraphCtx(); @@ -51,6 +53,7 @@ SIValue AR_EXISTS(SIValue *argv, int argc) { } SIValue _AR_NodeDegree(SIValue *argv, int argc, GRAPH_EDGE_DIR dir) { + if(argv[0].type == T_NULL) return SI_NullVal(); Node *n = (Node *)argv[0].ptrval; Edge *edges = array_new(Edge, 0); GraphContext *gc = QueryCtx_GetGraphCtx(); @@ -79,11 +82,13 @@ SIValue _AR_NodeDegree(SIValue *argv, int argc, GRAPH_EDGE_DIR dir) { /* Returns the number of incoming edges for given node. */ SIValue AR_INCOMEDEGREE(SIValue *argv, int argc) { + if(argv[0].type == T_NULL) return SI_NullVal(); return _AR_NodeDegree(argv, argc, GRAPH_EDGE_DIR_INCOMING); } /* Returns the number of outgoing edges for given node. */ SIValue AR_OUTGOINGDEGREE(SIValue *argv, int argc) { + if(argv[0].type == T_NULL) return SI_NullVal(); return _AR_NodeDegree(argv, argc, GRAPH_EDGE_DIR_OUTGOING); } @@ -92,34 +97,35 @@ void Register_EntityFuncs() { AR_FuncDesc *func_desc; types = array_new(SIType, 1); - types = array_append(types, T_NODE | T_EDGE); + types = array_append(types, T_NULL | T_NODE | T_EDGE); func_desc = AR_FuncDescNew("id", AR_ID, 1, 1, types, false); AR_RegFunc(func_desc); types = array_new(SIType, 1); - types = array_append(types, T_NODE); + types = array_append(types, T_NULL | T_NODE); func_desc = AR_FuncDescNew("labels", AR_LABELS, 1, 1, types, false); AR_RegFunc(func_desc); types = array_new(SIType, 1); - types = array_append(types, T_EDGE); + types = array_append(types, T_NULL | T_EDGE); func_desc = AR_FuncDescNew("type", AR_TYPE, 1, 1, types, false); AR_RegFunc(func_desc); types = array_new(SIType, 1); - types = array_append(types, SI_ALL); + types = array_append(types, T_NULL | SI_ALL); func_desc = AR_FuncDescNew("exists", AR_EXISTS, 1, 1, types, false); AR_RegFunc(func_desc); types = array_new(SIType, 2); - types = array_append(types, T_NODE); + types = array_append(types, T_NULL | T_NODE); types = array_append(types, T_STRING); func_desc = AR_FuncDescNew("indegree", AR_INCOMEDEGREE, 1, VAR_ARG_LEN, types, false); AR_RegFunc(func_desc); - types = array_new(SIType, 1); - types = array_append(types, T_NODE); + types = array_new(SIType, 2); + types = array_append(types, T_NULL | T_NODE); types = array_append(types, T_STRING); func_desc = AR_FuncDescNew("outdegree", AR_OUTGOINGDEGREE, 1, VAR_ARG_LEN, types, false); AR_RegFunc(func_desc); } + diff --git a/src/arithmetic/list_funcs/list_funcs.c b/src/arithmetic/list_funcs/list_funcs.c index 0073e08330..dc9935bfe4 100644 --- a/src/arithmetic/list_funcs/list_funcs.c +++ b/src/arithmetic/list_funcs/list_funcs.c @@ -144,6 +144,8 @@ SIValue AR_SIZE(SIValue *argv, int argc) { return SI_LongVal(SIArray_Length(value)); case T_STRING: return SI_LongVal(strlen(value.stringval)); + case T_NULL: + return SI_NullVal(); default: assert(false); } @@ -227,3 +229,4 @@ void Register_ListFuncs() { func_desc = AR_FuncDescNew("tail", AR_TAIL, 1, 1, types, true); AR_RegFunc(func_desc); } + diff --git a/tests/tck/features/FunctionsAcceptance.feature b/tests/tck/features/FunctionsAcceptance.feature index e47d5949f3..ea10d96701 100644 --- a/tests/tck/features/FunctionsAcceptance.feature +++ b/tests/tck/features/FunctionsAcceptance.feature @@ -47,7 +47,6 @@ Feature: FunctionsAcceptance | 'Nobody' | And no side effects - @skip Scenario: Functions should return null if they get path containing unbound Given any graph When executing query: @@ -383,7 +382,6 @@ Feature: FunctionsAcceptance | 'T1' | 'T2' | And no side effects - @skip Scenario: `type()` on null relationship Given an empty graph And having executed: @@ -401,7 +399,6 @@ Feature: FunctionsAcceptance | null | And no side effects - @skip Scenario: `type()` on mixed null and non-null relationships Given an empty graph And having executed: From d8e58a26c9c8101bde44bd077bedee562b4c5a29 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Wed, 25 Mar 2020 12:49:55 -0400 Subject: [PATCH 11/32] WIP improve mock AST logic --- src/ast/ast_mock.c | 14 +++++++++----- src/ast/ast_mock.h | 2 +- src/execution_plan/execution_plan_modify.c | 6 +++--- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/ast/ast_mock.c b/src/ast/ast_mock.c index 10a109a2cd..01c4cd6ea2 100644 --- a/src/ast/ast_mock.c +++ b/src/ast/ast_mock.c @@ -13,12 +13,14 @@ AST *AST_MockMatchPath(AST *master_ast, const cypher_astnode_t *original_path) { ast->referenced_entities = master_ast->referenced_entities; ast->anot_ctx_collection = master_ast->anot_ctx_collection; ast->free_root = true; + ast->skip = NULL; ast->limit = NULL; struct cypher_input_range range = {}; // Reuse the input path directly. We cannot clone, as this causes annotations (entity names) to be lost. // TODO consider updating parser to improve this. cypher_astnode_t *path = (cypher_astnode_t *)original_path; + // cypher_astnode_t *path = cypher_ast_clone(original_path); // Build a pattern comprised of the input path. cypher_astnode_t *pattern = cypher_ast_pattern(&path, 1, &path, 1, range); @@ -40,7 +42,8 @@ AST *AST_MockOptionalMatch(AST *master_ast, cypher_astnode_t *clause) { ast->referenced_entities = master_ast->referenced_entities; ast->anot_ctx_collection = master_ast->anot_ctx_collection; ast->free_root = true; - ast->limit = UNLIMITED; + ast->skip = NULL; + ast->limit = NULL; // TODO just need to turn off clause->optional, should be better options. struct cypher_input_range range = {}; @@ -54,21 +57,22 @@ AST *AST_MockOptionalMatch(AST *master_ast, cypher_astnode_t *clause) { // TODO tmp, bad, unsafe ast->root = cypher_ast_query(NULL, 0, &clone, 1, &clone, 1, range); - // ast->root = cypher_ast_query(NULL, 0, &clause, 1, &clause, 1, range); QueryCtx_SetAST(ast); // Update the TLS. return ast; } -void AST_MockFree(AST *ast) { +void AST_MockFree(AST *ast, bool free_pattern) { /* When freeing the mock AST, we have to be careful to not free the shared path * or its annotations. We'll free every surrounding layer explicitly - the MATCH * pattern, the MATCH clause, and finally the AST root. */ const cypher_astnode_t *clause = cypher_ast_query_get_clause(ast->root, 0); assert(cypher_astnode_type(clause) == CYPHER_AST_MATCH); - const cypher_astnode_t *pattern = cypher_ast_match_get_pattern(clause); - // cypher_astnode_free((cypher_astnode_t *)pattern); // TODO tmp + if(free_pattern) { + const cypher_astnode_t *pattern = cypher_ast_match_get_pattern(clause); + cypher_astnode_free((cypher_astnode_t *)pattern); + } cypher_astnode_free((cypher_astnode_t *)clause); cypher_astnode_free((cypher_astnode_t *)ast->root); rm_free(ast); diff --git a/src/ast/ast_mock.h b/src/ast/ast_mock.h index 5e70c3cff1..fd3840fad8 100644 --- a/src/ast/ast_mock.h +++ b/src/ast/ast_mock.h @@ -15,5 +15,5 @@ AST *AST_MockMatchPath(AST *master_ast, const cypher_astnode_t *original_path); AST *AST_MockOptionalMatch(AST *master_ast, cypher_astnode_t *clause); // Free a temporary AST. -void AST_MockFree(AST *ast); +void AST_MockFree(AST *ast, bool free_pattern); diff --git a/src/execution_plan/execution_plan_modify.c b/src/execution_plan/execution_plan_modify.c index 95d992f69a..2130d6f25a 100644 --- a/src/execution_plan/execution_plan_modify.c +++ b/src/execution_plan/execution_plan_modify.c @@ -316,9 +316,9 @@ OpBase *ExecutionPlan_BuildOpsFromPath(ExecutionPlan *plan, const char **bound_v AST *ast = QueryCtx_GetAST(); // Build a temporary AST holding a MATCH clause. - // TODO tmp + bool mock_entire_pattern = (cypher_astnode_type(path) == CYPHER_AST_MATCH); AST *match_stream_ast; - if(cypher_astnode_type(path) == CYPHER_AST_MATCH) { + if(mock_entire_pattern) { match_stream_ast = AST_MockOptionalMatch(ast, (cypher_astnode_t *)path); } else { match_stream_ast = AST_MockMatchPath(ast, path); @@ -326,7 +326,7 @@ OpBase *ExecutionPlan_BuildOpsFromPath(ExecutionPlan *plan, const char **bound_v ExecutionPlan_PopulateExecutionPlan(match_stream_plan); - AST_MockFree(match_stream_ast); + AST_MockFree(match_stream_ast, !mock_entire_pattern); QueryCtx_SetAST(ast); // Reset the AST. // Add filter ops to sub-ExecutionPlan. if(match_stream_plan->filter_tree) ExecutionPlan_PlaceFilterOps(match_stream_plan, NULL); From 7906473f48a062113cc9565041cff45d319f8110 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Wed, 25 Mar 2020 12:50:03 -0400 Subject: [PATCH 12/32] Add flow tests --- tests/flow/test_optional_match.py | 193 ++++++++++++++++++++ tests/tck/features/MatchAcceptance2.feature | 2 + 2 files changed, 195 insertions(+) create mode 100644 tests/flow/test_optional_match.py diff --git a/tests/flow/test_optional_match.py b/tests/flow/test_optional_match.py new file mode 100644 index 0000000000..54da5bda7f --- /dev/null +++ b/tests/flow/test_optional_match.py @@ -0,0 +1,193 @@ +import os +import sys +from RLTest import Env +from redisgraph import Graph, Node, Edge +from base import FlowTestsBase + +redis_graph = None + +class testOptionalFlow(FlowTestsBase): + def __init__(self): + self.env = Env() + global redis_graph + redis_con = self.env.getConnection() + redis_graph = Graph("G", redis_con) + self.populate_graph() + + def populate_graph(self): + # Construct a graph with the form: + # (v1)-[:E1]->(v2)-[:E2]->(v3), (v4) + node_props = ['v1', 'v2', 'v3', 'v4'] + + nodes = [] + for idx, v in enumerate(node_props): + node = Node(label="L", properties={"v": v}) + nodes.append(node) + redis_graph.add_node(node) + + edge = Edge(nodes[0], "E1", nodes[1]) + redis_graph.add_edge(edge) + + edge = Edge(nodes[1], "E2", nodes[2]) + redis_graph.add_edge(edge) + + redis_graph.commit() + + # Optional MATCH clause that does not interact with the mandatory MATCH. + def test01_disjoint_optional(self): + global redis_graph + query = """MATCH (a {v: 'v1'}) OPTIONAL MATCH (b) RETURN a.v, b.v ORDER BY a.v, b.v""" + actual_result = redis_graph.query(query) + expected_result = [['v1', 'v1'], + ['v1', 'v2'], + ['v1', 'v3'], + ['v1', 'v4']] + self.env.assertEquals(actual_result.result_set, expected_result) + + # Optional MATCH clause that extends the mandatory MATCH pattern and has matches for all results. + def test02_optional_traverse(self): + global redis_graph + query = """MATCH (a) WHERE a.v IN ['v1', 'v2'] OPTIONAL MATCH (a)-[]->(b) RETURN a.v, b.v ORDER BY a.v, b.v""" + actual_result = redis_graph.query(query) + expected_result = [['v1', 'v2'], + ['v2', 'v3']] + self.env.assertEquals(actual_result.result_set, expected_result) + + # Optional MATCH clause that extends the mandatory MATCH pattern and has null results. + def test03_optional_traverse_with_nulls(self): + global redis_graph + query = """MATCH (a) OPTIONAL MATCH (a)-[]->(b) RETURN a.v, b.v ORDER BY a.v, b.v""" + actual_result = redis_graph.query(query) + # (v3) and (v4) have no outgoing edges. + expected_result = [['v1', 'v2'], + ['v2', 'v3'], + ['v3', None], + ['v4', None]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # Optional MATCH clause that extends the mandatory MATCH pattern and has a WHERE clause. + def test04_optional_traverse_with_predicate(self): + global redis_graph + query = """MATCH (a) OPTIONAL MATCH (a)-[]->(b) WHERE b.v = 'v2' RETURN a.v, b.v ORDER BY a.v, b.v""" + actual_result = redis_graph.query(query) + # only (v1) has an outgoing edge to (v2). + expected_result = [['v1', 'v2'], + ['v2', None], + ['v3', None], + ['v4', None]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # Optional MATCH clause with endpoints resolved by the mandatory MATCH pattern. + def test05_optional_expand_into(self): + global redis_graph + query = """MATCH (a)-[]->(b) OPTIONAL MATCH (a)-[e]->(b) RETURN a.v, b.v, TYPE(e) ORDER BY a.v, b.v""" + actual_result = redis_graph.query(query) + expected_result = [['v1', 'v2', 'E1'], + ['v2', 'v3', 'E2']] + self.env.assertEquals(actual_result.result_set, expected_result) + + # The OPTIONAL MATCH exactly repeats the MATCH, producing identical results. + query_without_optional = """MATCH (a)-[e]->(b) RETURN a.v, b.v, TYPE(e) ORDER BY a.v, b.v""" + result_without_optional = redis_graph.query(query_without_optional) + self.env.assertEquals(actual_result.result_set, result_without_optional.result_set) + + # Optional MATCH clause with endpoints resolved by the mandatory MATCH pattern and new filters introduced. + def test06_optional_expand_into_with_reltype(self): + global redis_graph + query = """MATCH (a)-[]->(b) OPTIONAL MATCH (a)-[e:E2]->(b) RETURN a.v, b.v, TYPE(e) ORDER BY a.v, b.v""" + actual_result = redis_graph.query(query) + # Only (v2)-[E2]->(v3) fulfills the constraint of the OPTIONAL MATCH clause. + expected_result = [['v1', 'v2', None], + ['v2', 'v3', 'E2']] + self.env.assertEquals(actual_result.result_set, expected_result) + + # Optional MATCH clause with endpoints resolved by the mandatory MATCH pattern, but no mandatory traversal. + def test07_optional_expand_into_cartesian_product(self): + global redis_graph + query = """MATCH (a {v: 'v1'}), (b) OPTIONAL MATCH (a)-[e]->(b) RETURN a.v, b.v, TYPE(e) ORDER BY a.v, b.v""" + actual_result = redis_graph.query(query) + # All nodes are represented, but (v1)-[E1]->(v2) is the only matching connection. + expected_result = [['v1', 'v1', None], + ['v1', 'v2', 'E1'], + ['v1', 'v3', None], + ['v1', 'v4', None]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # TODO ExpandInto doesn't evaluate bidirectionally properly + # Optional MATCH clause with endpoints resolved by the mandatory MATCH pattern and a bidirectional optional pattern. + # def test08_optional_expand_into_bidirectional(self): + # global redis_graph + # query = """MATCH (a), (b {v: 'v2'}) OPTIONAL MATCH (a)-[e]-(b) RETURN a.v, b.v, TYPE(e) ORDER BY a.v, b.v""" + # actual_result = redis_graph.query(query) + # # All nodes are represented, but only edges with (v2) as an endpoint match. + # expected_result = [['v1', 'v2', 'E1'], + # ['v2', 'v2', None], + # ['v3', 'v2', 'E2'], + # ['v3', 'v2', None]] + # self.env.assertEquals(actual_result.result_set, expected_result) + + # Optional MATCH clause with variable-length traversal and some results match. + def test09_optional_variable_length(self): + global redis_graph + query = """MATCH (a) OPTIONAL MATCH (a)-[*]->(b) RETURN a.v, b.v ORDER BY a.v, b.v""" + actual_result = redis_graph.query(query) + expected_result = [['v1', 'v2'], + ['v1', 'v3'], + ['v2', 'v3'], + ['v3', None], + ['v4', None]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # Optional MATCH clause with variable-length traversal and all results match. + def test10_optional_variable_length_all_matches(self): + global redis_graph + query = """MATCH (a {v: 'v1'}) OPTIONAL MATCH (a)-[*]->(b) RETURN a.v, b.v ORDER BY a.v, b.v""" + actual_result = redis_graph.query(query) + expected_result = [['v1', 'v2'], + ['v1', 'v3']] + self.env.assertEquals(actual_result.result_set, expected_result) + + # Optional MATCH clause with a variable-length traversal that has no matches. + def test11_optional_variable_length_no_matches(self): + global redis_graph + query = """MATCH (a {v: 'v3'}) OPTIONAL MATCH (a)-[*]->(b) RETURN a.v, b.v ORDER BY a.v, b.v""" + actual_result = redis_graph.query(query) + expected_result = [['v3', None]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # Multiple interdependent optional MATCH clauses. + def test12_multiple_optional_traversals(self): + global redis_graph + query = """MATCH (a) OPTIONAL MATCH (a)-[]->(b) OPTIONAL MATCH (b)-[]->(c) RETURN a.v, b.v, c.v ORDER BY a.v, b.v, c.v""" + actual_result = redis_graph.query(query) + expected_result = [['v1', 'v2', 'v3'], + ['v2', 'v3', None], + ['v3', None, None], + ['v4', None, None]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # Multiple interdependent optional MATCH clauses with both directed and bidirectional traversals. + def test13_multiple_optional_multi_directional_traversals(self): + global redis_graph + query = """MATCH (a) OPTIONAL MATCH (a)-[]-(b) OPTIONAL MATCH (b)-[]->(c) RETURN a.v, b.v, c.v ORDER BY a.v, b.v, c.v""" + actual_result = redis_graph.query(query) + expected_result = [['v1', 'v2', 'v3'], + ['v2', 'v1', 'v2'], + ['v2', 'v3', None], + ['v3', 'v2', 'v3'], + ['v4', None, None]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # Multiple interdependent optional MATCH clauses with exclusively bidirectional traversals. + def test14_multiple_optional_bidirectional_traversals(self): + global redis_graph + query = """MATCH (a) OPTIONAL MATCH (a)-[]-(b) OPTIONAL MATCH (b)-[]-(c) RETURN a.v, b.v, c.v ORDER BY a.v, b.v, c.v""" + actual_result = redis_graph.query(query) + expected_result = [['v1', 'v2', 'v1'], + ['v1', 'v2', 'v3'], + ['v2', 'v1', 'v2'], + ['v2', 'v3', 'v2'], + ['v3', 'v2', 'v1'], + ['v3', 'v2', 'v3'], + ['v4', None, None]] + self.env.assertEquals(actual_result.result_set, expected_result) diff --git a/tests/tck/features/MatchAcceptance2.feature b/tests/tck/features/MatchAcceptance2.feature index 036e754bf3..d0eb1066e9 100644 --- a/tests/tck/features/MatchAcceptance2.feature +++ b/tests/tck/features/MatchAcceptance2.feature @@ -1229,6 +1229,7 @@ Feature: MatchAcceptance2 | (:A) | (:C) | And no side effects +@skip Scenario: Matching relationships into a list and matching variable length using the list, with bound nodes Given an empty graph And having executed: @@ -1250,6 +1251,7 @@ Feature: MatchAcceptance2 | (:A) | (:C) | And no side effects +@skip Scenario: Matching relationships into a list and matching variable length using the list, with bound nodes, wrong direction Given an empty graph And having executed: From a6332fe7fb3e3fb2424049dead73ba4f0003a703 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Wed, 25 Mar 2020 15:34:03 -0400 Subject: [PATCH 13/32] Improve AST mock logic --- src/ast/ast_mock.c | 65 ++++++++++------------ src/ast/ast_mock.h | 7 +-- src/execution_plan/execution_plan_modify.c | 14 ++--- src/execution_plan/ops/op_expand_into.c | 2 +- 4 files changed, 36 insertions(+), 52 deletions(-) diff --git a/src/ast/ast_mock.c b/src/ast/ast_mock.c index 01c4cd6ea2..4ce193cd8c 100644 --- a/src/ast/ast_mock.c +++ b/src/ast/ast_mock.c @@ -8,26 +8,43 @@ #include "../query_ctx.h" #include "../util/rmalloc.h" -AST *AST_MockMatchPath(AST *master_ast, const cypher_astnode_t *original_path) { +AST *AST_MockMatchClause(AST *master_ast, cypher_astnode_t *node, bool node_is_path) { + /* The AST node and its children must be reused directly, + * as cloning causes annotations (entity names) to be lost. + * TODO consider updating parser to improve this. */ AST *ast = rm_malloc(sizeof(AST)); ast->referenced_entities = master_ast->referenced_entities; ast->anot_ctx_collection = master_ast->anot_ctx_collection; ast->free_root = true; ast->skip = NULL; ast->limit = NULL; + cypher_astnode_t *pattern; struct cypher_input_range range = {}; + const cypher_astnode_t *predicate = NULL; + uint child_count = (node_is_path) ? 1 : cypher_astnode_nchildren(node); + cypher_astnode_t *children[child_count]; - // Reuse the input path directly. We cannot clone, as this causes annotations (entity names) to be lost. - // TODO consider updating parser to improve this. - cypher_astnode_t *path = (cypher_astnode_t *)original_path; - // cypher_astnode_t *path = cypher_ast_clone(original_path); + if(node_is_path) { + /* MERGE clauses and path filters are comprised of a single path. + * Construct a pattern node containing just this path. */ + pattern = cypher_ast_pattern(&node, 1, &node, 1, range); + children[0] = pattern; + } else { + /* OPTIONAL MATCH clauses contain a pattern node + * and possibly a predicate node (containing WHERE conditions). */ + assert(cypher_astnode_type(node) == CYPHER_AST_MATCH); + pattern = (cypher_astnode_t *)cypher_ast_match_get_pattern(node); + predicate = cypher_ast_match_get_predicate(node); - // Build a pattern comprised of the input path. - cypher_astnode_t *pattern = cypher_ast_pattern(&path, 1, &path, 1, range); + // Explicitly collect all child nodes from the clause. + for(uint i = 0; i < child_count; i ++) { + children[i] = (cypher_astnode_t *)cypher_astnode_get_child(node, i); + } + } // Build a new match clause that holds this pattern. - cypher_astnode_t *match_clause = cypher_ast_match(false, pattern, NULL, 0, NULL, &pattern, 1, - range); + cypher_astnode_t *match_clause = cypher_ast_match(false, pattern, NULL, 0, predicate, + children, child_count, range); // Build a query node holding this clause. ast->root = cypher_ast_query(NULL, 0, &match_clause, 1, &match_clause, 1, range); @@ -37,36 +54,10 @@ AST *AST_MockMatchPath(AST *master_ast, const cypher_astnode_t *original_path) { return ast; } -AST *AST_MockOptionalMatch(AST *master_ast, cypher_astnode_t *clause) { - AST *ast = rm_malloc(sizeof(AST)); - ast->referenced_entities = master_ast->referenced_entities; - ast->anot_ctx_collection = master_ast->anot_ctx_collection; - ast->free_root = true; - ast->skip = NULL; - ast->limit = NULL; - - // TODO just need to turn off clause->optional, should be better options. - struct cypher_input_range range = {}; - const cypher_astnode_t *pattern = cypher_ast_match_get_pattern(clause); - const cypher_astnode_t *predicate = cypher_ast_match_get_predicate(clause); - uint child_count = cypher_astnode_nchildren(clause); - const cypher_astnode_t *children[child_count]; - for(uint i = 0; i < child_count; i ++) children[i] = cypher_astnode_get_child(clause, i); - cypher_astnode_t *clone = cypher_ast_match(false, pattern, NULL, 0, predicate, - (cypher_astnode_t **)children, child_count, range); - - // TODO tmp, bad, unsafe - ast->root = cypher_ast_query(NULL, 0, &clone, 1, &clone, 1, range); - - QueryCtx_SetAST(ast); // Update the TLS. - - return ast; -} - void AST_MockFree(AST *ast, bool free_pattern) { - /* When freeing the mock AST, we have to be careful to not free the shared path + /* When freeing the mock AST, we have to be careful to not free any shared node * or its annotations. We'll free every surrounding layer explicitly - the MATCH - * pattern, the MATCH clause, and finally the AST root. */ + * pattern (if we constructed it), the MATCH clause, and finally the AST root. */ const cypher_astnode_t *clause = cypher_ast_query_get_clause(ast->root, 0); assert(cypher_astnode_type(clause) == CYPHER_AST_MATCH); if(free_pattern) { diff --git a/src/ast/ast_mock.h b/src/ast/ast_mock.h index fd3840fad8..6de3ea065c 100644 --- a/src/ast/ast_mock.h +++ b/src/ast/ast_mock.h @@ -8,11 +8,8 @@ #include "ast.h" -// Build a temporary AST with one MATCH clause that holds the given path. -AST *AST_MockMatchPath(AST *master_ast, const cypher_astnode_t *original_path); - -// TODO improve name -AST *AST_MockOptionalMatch(AST *master_ast, cypher_astnode_t *clause); +// Build a temporary AST with one MATCH clause that holds the given path or pattern. +AST *AST_MockMatchClause(AST *master_ast, cypher_astnode_t *node, bool node_is_pattern); // Free a temporary AST. void AST_MockFree(AST *ast, bool free_pattern); diff --git a/src/execution_plan/execution_plan_modify.c b/src/execution_plan/execution_plan_modify.c index 2130d6f25a..a417237580 100644 --- a/src/execution_plan/execution_plan_modify.c +++ b/src/execution_plan/execution_plan_modify.c @@ -306,7 +306,7 @@ void ExecutionPlan_BindPlanToOps(ExecutionPlan *plan, OpBase *root) { } OpBase *ExecutionPlan_BuildOpsFromPath(ExecutionPlan *plan, const char **bound_vars, - const cypher_astnode_t *path) { + const cypher_astnode_t *node) { // Initialize an ExecutionPlan that shares this plan's Record mapping. ExecutionPlan *match_stream_plan = ExecutionPlan_NewEmptyExecutionPlan(); match_stream_plan->record_map = plan->record_map; @@ -316,17 +316,13 @@ OpBase *ExecutionPlan_BuildOpsFromPath(ExecutionPlan *plan, const char **bound_v AST *ast = QueryCtx_GetAST(); // Build a temporary AST holding a MATCH clause. - bool mock_entire_pattern = (cypher_astnode_type(path) == CYPHER_AST_MATCH); - AST *match_stream_ast; - if(mock_entire_pattern) { - match_stream_ast = AST_MockOptionalMatch(ast, (cypher_astnode_t *)path); - } else { - match_stream_ast = AST_MockMatchPath(ast, path); - } + cypher_astnode_type_t type = cypher_astnode_type(node); + bool node_is_path = (type == CYPHER_AST_PATTERN_PATH || type == CYPHER_AST_NAMED_PATH); + AST *match_stream_ast = AST_MockMatchClause(ast, (cypher_astnode_t *)node, node_is_path); ExecutionPlan_PopulateExecutionPlan(match_stream_plan); - AST_MockFree(match_stream_ast, !mock_entire_pattern); + AST_MockFree(match_stream_ast, node_is_path); QueryCtx_SetAST(ast); // Reset the AST. // Add filter ops to sub-ExecutionPlan. if(match_stream_plan->filter_tree) ExecutionPlan_PlaceFilterOps(match_stream_plan, NULL); diff --git a/src/execution_plan/ops/op_expand_into.c b/src/execution_plan/ops/op_expand_into.c index 3c36578862..021d0e337a 100644 --- a/src/execution_plan/ops/op_expand_into.c +++ b/src/execution_plan/ops/op_expand_into.c @@ -103,7 +103,7 @@ OpBase *NewExpandIntoOp(const ExecutionPlan *plan, Graph *g, AlgebraicExpression op->recordCount = 0; op->edgeRelationTypes = NULL; op->recordsCap = 0; - op->records = rm_calloc(op->recordsCap, sizeof(Record)); + op->records = NULL; // Set our Op operations OpBase_Init((OpBase *)op, OPType_EXPAND_INTO, "Expand Into", ExpandIntoInit, ExpandIntoConsume, From b3843c1e623a408e3432dad6d29b1c4c4db103f5 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Wed, 25 Mar 2020 15:54:06 -0400 Subject: [PATCH 14/32] Error handling for SET and CREATE on null entities --- src/execution_plan/ops/op_create.c | 9 +++++++++ src/execution_plan/ops/op_update.c | 9 ++++++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/execution_plan/ops/op_create.c b/src/execution_plan/ops/op_create.c index 95a1158b41..da45c68e00 100644 --- a/src/execution_plan/ops/op_create.c +++ b/src/execution_plan/ops/op_create.c @@ -73,6 +73,15 @@ static void _CreateEdges(OpCreate *op, Record r) { /* Get specified edge to create. */ QGEdge *e = op->pending.edges_to_create[i].edge; + /* Verify that the endpoints of the new edge resolved properly; failing otherwise. */ + if((Record_GetType(r, op->pending.edges_to_create[i].src_idx) != REC_TYPE_NODE) || + (Record_GetType(r, op->pending.edges_to_create[i].dest_idx) != REC_TYPE_NODE)) { + char *error; + asprintf(&error, "Failed to create relationship; endpoint was not found."); + QueryCtx_SetError(error); + QueryCtx_RaiseRuntimeException(); + } + /* Retrieve source and dest nodes. */ Node *src_node = Record_GetNode(r, op->pending.edges_to_create[i].src_idx); Node *dest_node = Record_GetNode(r, op->pending.edges_to_create[i].dest_idx); diff --git a/src/execution_plan/ops/op_update.c b/src/execution_plan/ops/op_update.c index 15682c2e62..d2ea5c9892 100644 --- a/src/execution_plan/ops/op_update.c +++ b/src/execution_plan/ops/op_update.c @@ -181,13 +181,16 @@ static Record UpdateConsume(OpBase *opBase) { * for later execution. */ EntityUpdateEvalCtx *update_expression = op->update_expressions; for(uint i = 0; i < op->update_expressions_count; i++, update_expression++) { - SIValue new_value = SI_CloneValue(AR_EXP_Evaluate(update_expression->exp, r)); - // Make sure we're updating either a node or an edge. + // Get the type of the entity to update. RecordEntryType t = Record_GetType(r, update_expression->record_idx); + // If the expected entity was not found, make no updates but do not error. + if(t == REC_TYPE_UNKNOWN) continue; + // Make sure we're updating either a node or an edge. assert(t == REC_TYPE_NODE || t == REC_TYPE_EDGE); GraphEntityType type = (t == REC_TYPE_NODE) ? GETYPE_NODE : GETYPE_EDGE; - GraphEntity *entity = Record_GetGraphEntity(r, update_expression->record_idx); + + SIValue new_value = SI_CloneValue(AR_EXP_Evaluate(update_expression->exp, r)); _QueueUpdate(op, entity, type, update_expression->attribute, new_value); } From 8117f07e5943407cb01247578789ac48689402cd Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Fri, 27 Mar 2020 11:16:31 -0400 Subject: [PATCH 15/32] Record_Get refactor --- src/execution_plan/ops/op_all_node_scan.c | 9 ++--- .../ops/op_conditional_traverse.c | 6 ++-- src/execution_plan/ops/op_create.c | 27 ++++++++------ src/execution_plan/ops/op_delete.c | 3 ++ src/execution_plan/ops/op_index_scan.c | 9 ++--- src/execution_plan/ops/op_merge_create.c | 34 ++++++++++++------ .../ops/op_node_by_label_scan.c | 7 ++-- src/execution_plan/record.c | 35 +++++++++++++++---- src/execution_plan/record.h | 12 +++---- 9 files changed, 96 insertions(+), 46 deletions(-) diff --git a/src/execution_plan/ops/op_all_node_scan.c b/src/execution_plan/ops/op_all_node_scan.c index 1ee1589e93..db883ab72f 100644 --- a/src/execution_plan/ops/op_all_node_scan.c +++ b/src/execution_plan/ops/op_all_node_scan.c @@ -70,8 +70,8 @@ static Record AllNodeScanConsumeFromChild(OpBase *opBase) { Record r = OpBase_CloneRecord(op->child_record); // Populate the Record with the graph entity data. - Node *n = Record_GetNode(r, op->nodeRecIdx); - n->entity = en; + Node n = { .entity = en }; + Record_AddNode(r, op->nodeRecIdx, n); return r; } @@ -83,8 +83,8 @@ static Record AllNodeScanConsume(OpBase *opBase) { if(en == NULL) return NULL; Record r = OpBase_CreateRecord((OpBase *)op); - Node *n = Record_GetNode(r, op->nodeRecIdx); - n->entity = en; + Node n = { .entity = en }; + Record_AddNode(r, op->nodeRecIdx, n); return r; } @@ -113,3 +113,4 @@ static void AllNodeScanFree(OpBase *ctx) { op->child_record = NULL; } } + diff --git a/src/execution_plan/ops/op_conditional_traverse.c b/src/execution_plan/ops/op_conditional_traverse.c index 1ee8d15bc7..bc597f917f 100644 --- a/src/execution_plan/ops/op_conditional_traverse.c +++ b/src/execution_plan/ops/op_conditional_traverse.c @@ -78,6 +78,7 @@ static void _populate_filter_matrix(CondTraverse *op) { /* Update filter matrix F, set row i at position srcId * F[i, srcId] = true. */ Node *n = Record_GetNode(r, op->srcNodeIdx); + if(!n) continue; // The expected entity may not be found on optional traversals. NodeID srcId = ENTITY_GET_ID(n); GrB_Matrix_setElement_BOOL(op->F, true, i, srcId); } @@ -213,8 +214,9 @@ static Record CondTraverseConsume(OpBase *opBase) { /* Get node from current column. */ op->r = op->records[src_id]; - Node *destNode = Record_GetNode(op->r, op->destNodeIdx); - Graph_GetNode(op->graph, dest_id, destNode); + Node destNode; + Graph_GetNode(op->graph, dest_id, &destNode); + Record_AddNode(op->r, op->destNodeIdx, destNode); if(op->setEdge) { _CondTraverse_CollectEdges(op, op->destNodeIdx, op->srcNodeIdx); diff --git a/src/execution_plan/ops/op_create.c b/src/execution_plan/ops/op_create.c index da45c68e00..fd61731e4a 100644 --- a/src/execution_plan/ops/op_create.c +++ b/src/execution_plan/ops/op_create.c @@ -48,10 +48,14 @@ static void _CreateNodes(OpCreate *op, Record r) { QGNode *n = op->pending.nodes_to_create[i].node; /* Create a new node. */ - Node *newNode = Record_GetNode(r, op->pending.nodes_to_create[i].node_idx); - newNode->entity = NULL; - newNode->label = n->label; - newNode->labelID = n->labelID; + Node newNode = { + .entity = NULL, + .label = n->label, + .labelID = n->labelID + }; + + /* Add new node to Record and save a reference to it. */ + Node *node_ref = Record_AddNode(r, op->pending.nodes_to_create[i].node_idx, newNode); /* Convert query-level properties. */ PropertyMap *map = op->pending.nodes_to_create[i].properties; @@ -59,7 +63,7 @@ static void _CreateNodes(OpCreate *op, Record r) { if(map) converted_properties = ConvertPropertyMap(r, map); /* Save node for later insertion. */ - op->pending.created_nodes = array_append(op->pending.created_nodes, newNode); + op->pending.created_nodes = array_append(op->pending.created_nodes, node_ref); /* Save properties to insert with node. */ op->pending.node_properties = array_append(op->pending.node_properties, converted_properties); @@ -73,7 +77,7 @@ static void _CreateEdges(OpCreate *op, Record r) { /* Get specified edge to create. */ QGEdge *e = op->pending.edges_to_create[i].edge; - /* Verify that the endpoints of the new edge resolved properly; failing otherwise. */ + /* Verify that the endpoints of the new edge resolved properly; fail otherwise. */ if((Record_GetType(r, op->pending.edges_to_create[i].src_idx) != REC_TYPE_NODE) || (Record_GetType(r, op->pending.edges_to_create[i].dest_idx) != REC_TYPE_NODE)) { char *error; @@ -87,10 +91,11 @@ static void _CreateEdges(OpCreate *op, Record r) { Node *dest_node = Record_GetNode(r, op->pending.edges_to_create[i].dest_idx); /* Create the actual edge. */ - Edge *newEdge = Record_GetEdge(r, op->pending.edges_to_create[i].edge_idx); - if(array_len(e->reltypes) > 0) newEdge->relationship = e->reltypes[0]; - Edge_SetSrcNode(newEdge, src_node); - Edge_SetDestNode(newEdge, dest_node); + Edge newEdge = {}; + if(array_len(e->reltypes) > 0) newEdge.relationship = e->reltypes[0]; + Edge_SetSrcNode(&newEdge, src_node); + Edge_SetDestNode(&newEdge, dest_node); + Edge *edge_ref = Record_AddEdge(r, op->pending.edges_to_create[i].edge_idx, newEdge); /* Convert query-level properties. */ PropertyMap *map = op->pending.edges_to_create[i].properties; @@ -98,7 +103,7 @@ static void _CreateEdges(OpCreate *op, Record r) { if(map) converted_properties = ConvertPropertyMap(r, map); /* Save edge for later insertion. */ - op->pending.created_edges = array_append(op->pending.created_edges, newEdge); + op->pending.created_edges = array_append(op->pending.created_edges, edge_ref); /* Save properties to insert with node. */ op->pending.edge_properties = array_append(op->pending.edge_properties, converted_properties); diff --git a/src/execution_plan/ops/op_delete.c b/src/execution_plan/ops/op_delete.c index 971218a730..80925a6e50 100644 --- a/src/execution_plan/ops/op_delete.c +++ b/src/execution_plan/ops/op_delete.c @@ -85,6 +85,9 @@ static Record DeleteConsume(OpBase *opBase) { } else if(SI_TYPE(value) & T_EDGE) { Edge *e = (Edge *)value.ptrval; op->deleted_edges = array_append(op->deleted_edges, *e); + } else if(SI_TYPE(value) & T_NULL) { + // Ignore null values. + continue; } else { /* Expression evaluated to a non-graph entity type * clear pending deletions and raise an exception. */ diff --git a/src/execution_plan/ops/op_index_scan.c b/src/execution_plan/ops/op_index_scan.c index a3c7436f00..caeee18ace 100644 --- a/src/execution_plan/ops/op_index_scan.c +++ b/src/execution_plan/ops/op_index_scan.c @@ -41,10 +41,11 @@ static OpResult IndexScanInit(OpBase *opBase) { } static inline void _UpdateRecord(IndexScan *op, Record r, EntityID node_id) { - // Get a pointer to a heap allocated node. - Node *n = Record_GetNode(r, op->nodeRecIdx); - // Update node's internal entity pointer. - assert(Graph_GetNode(op->g, node_id, n)); + // Populate the Record with the graph entity data. + Node n = {}; + assert(Graph_GetNode(op->g, node_id, &n)); + // Get a pointer to the node's allocated space within the Record. + Record_AddNode(r, op->nodeRecIdx, n); } static Record IndexScanConsumeFromChild(OpBase *opBase) { diff --git a/src/execution_plan/ops/op_merge_create.c b/src/execution_plan/ops/op_merge_create.c index 90dbe05918..9fd7798467 100644 --- a/src/execution_plan/ops/op_merge_create.c +++ b/src/execution_plan/ops/op_merge_create.c @@ -93,10 +93,13 @@ static bool _CreateEntities(OpMergeCreate *op, Record r) { QGNode *n = op->pending.nodes_to_create[i].node; /* Create a new node. */ - Node *newNode = Record_GetNode(r, op->pending.nodes_to_create[i].node_idx); - newNode->entity = NULL; - newNode->label = n->label; - newNode->labelID = n->labelID; + Node newNode = { + .entity = NULL, + .label = n->label, + .labelID = n->labelID + }; + /* Add new node to Record and save a reference to it. */ + Node *node_ref = Record_AddNode(r, op->pending.nodes_to_create[i].node_idx, newNode); /* Convert query-level properties. */ PropertyMap *map = op->pending.nodes_to_create[i].properties; @@ -107,7 +110,7 @@ static bool _CreateEntities(OpMergeCreate *op, Record r) { _IncrementalHashEntity(op->hash_state, n->label, converted_properties); /* Save node for later insertion. */ - op->pending.created_nodes = array_append(op->pending.created_nodes, newNode); + op->pending.created_nodes = array_append(op->pending.created_nodes, node_ref); /* Save properties to insert with node. */ op->pending.node_properties = array_append(op->pending.node_properties, converted_properties); @@ -118,15 +121,26 @@ static bool _CreateEntities(OpMergeCreate *op, Record r) { /* Get specified edge to create. */ QGEdge *e = op->pending.edges_to_create[i].edge; + /* Verify that the endpoints of the new edge resolved properly; fail otherwise. */ + if((Record_GetType(r, op->pending.edges_to_create[i].src_idx) != REC_TYPE_NODE) || + (Record_GetType(r, op->pending.edges_to_create[i].dest_idx) != REC_TYPE_NODE)) { + char *error; + asprintf(&error, "Failed to create relationship; endpoint was not found."); + QueryCtx_SetError(error); + QueryCtx_RaiseRuntimeException(); + } + /* Retrieve source and dest nodes. */ Node *src_node = Record_GetNode(r, op->pending.edges_to_create[i].src_idx); Node *dest_node = Record_GetNode(r, op->pending.edges_to_create[i].dest_idx); /* Create the actual edge. */ - Edge *newEdge = Record_GetEdge(r, op->pending.edges_to_create[i].edge_idx); - if(array_len(e->reltypes) > 0) newEdge->relationship = e->reltypes[0]; - Edge_SetSrcNode(newEdge, src_node); - Edge_SetDestNode(newEdge, dest_node); + Edge newEdge = {}; + if(array_len(e->reltypes) > 0) newEdge.relationship = e->reltypes[0]; + Edge_SetSrcNode(&newEdge, src_node); + Edge_SetDestNode(&newEdge, dest_node); + + Edge *edge_ref = Record_AddEdge(r, op->pending.edges_to_create[i].edge_idx, newEdge); /* Convert query-level properties. */ PropertyMap *map = op->pending.edges_to_create[i].properties; @@ -153,7 +167,7 @@ static bool _CreateEntities(OpMergeCreate *op, Record r) { } /* Save edge for later insertion. */ - op->pending.created_edges = array_append(op->pending.created_edges, newEdge); + op->pending.created_edges = array_append(op->pending.created_edges, edge_ref); /* Save properties to insert with node. */ op->pending.edge_properties = array_append(op->pending.edge_properties, converted_properties); diff --git a/src/execution_plan/ops/op_node_by_label_scan.c b/src/execution_plan/ops/op_node_by_label_scan.c index 3b01bd7e5f..3bdd1a46a4 100644 --- a/src/execution_plan/ops/op_node_by_label_scan.c +++ b/src/execution_plan/ops/op_node_by_label_scan.c @@ -89,10 +89,11 @@ static OpResult NodeByLabelScanInit(OpBase *opBase) { } static inline void _UpdateRecord(NodeByLabelScan *op, Record r, GrB_Index node_id) { - // Get a pointer to the node's allocated space within the Record. - Node *n = Record_GetNode(r, op->nodeRecIdx); // Populate the Record with the graph entity data. - Graph_GetNode(op->g, node_id, n); + Node n = {}; + Graph_GetNode(op->g, node_id, &n); + // Get a pointer to the node's allocated space within the Record. + Record_AddNode(r, op->nodeRecIdx, n); } static inline void _ResetIterator(NodeByLabelScan *op) { diff --git a/src/execution_plan/record.c b/src/execution_plan/record.c index 1d95c0f31a..fbd3c78dd5 100644 --- a/src/execution_plan/record.c +++ b/src/execution_plan/record.c @@ -89,17 +89,37 @@ RecordEntryType Record_GetType(const Record r, int idx) { } SIValue Record_GetScalar(Record r, int idx) { - r->entries[idx].type = REC_TYPE_SCALAR; + assert(r->entries[idx].type == REC_TYPE_SCALAR); return r->entries[idx].value.s; } Node *Record_GetNode(const Record r, int idx) { - r->entries[idx].type = REC_TYPE_NODE; + switch(r->entries[idx].type) { + case REC_TYPE_NODE: + return &(r->entries[idx].value.n); + case REC_TYPE_UNKNOWN: + return NULL; + case REC_TYPE_SCALAR: + // Null scalar values are expected here; otherwise fall through. + if(SIValue_IsNull(r->entries[idx].value.s)) return NULL; + default: + assert("encountered unexpected type in Record; expected Node" && false); + } return &(r->entries[idx].value.n); } Edge *Record_GetEdge(const Record r, int idx) { - r->entries[idx].type = REC_TYPE_EDGE; + switch(r->entries[idx].type) { + case REC_TYPE_EDGE: + return &(r->entries[idx].value.e); + case REC_TYPE_UNKNOWN: + return NULL; + case REC_TYPE_SCALAR: + // Null scalar values are expected here; otherwise fall through. + if(SIValue_IsNull(r->entries[idx].value.s)) return NULL; + default: + assert("encountered unexpected type in Record; expected Edge" && false); + } return &(r->entries[idx].value.e); } @@ -149,19 +169,22 @@ void Record_Add(Record r, int idx, SIValue v) { } } -void Record_AddScalar(Record r, int idx, SIValue v) { +SIValue *Record_AddScalar(Record r, int idx, SIValue v) { r->entries[idx].value.s = v; r->entries[idx].type = REC_TYPE_SCALAR; + return &(r->entries[idx].value.s); } -void Record_AddNode(Record r, int idx, Node node) { +Node *Record_AddNode(Record r, int idx, Node node) { r->entries[idx].value.n = node; r->entries[idx].type = REC_TYPE_NODE; + return &(r->entries[idx].value.n); } -void Record_AddEdge(Record r, int idx, Edge edge) { +Edge *Record_AddEdge(Record r, int idx, Edge edge) { r->entries[idx].value.e = edge; r->entries[idx].type = REC_TYPE_EDGE; + return &(r->entries[idx].value.e); } void Record_PersistScalars(Record r) { diff --git a/src/execution_plan/record.h b/src/execution_plan/record.h index 7f1820dbe0..f2305f2894 100644 --- a/src/execution_plan/record.h +++ b/src/execution_plan/record.h @@ -79,14 +79,14 @@ GraphEntity *Record_GetGraphEntity(const Record r, int idx); // Add a scalar, node, or edge to the record, depending on the SIValue type. void Record_Add(Record r, int idx, SIValue v); -// Add a scalar to record at position idx. -void Record_AddScalar(Record r, int idx, SIValue v); +// Add a scalar to record at position idx and return a reference to it. +SIValue *Record_AddScalar(Record r, int idx, SIValue v); -// Add a node to record at position idx. -void Record_AddNode(Record r, int idx, Node node); +// Add a node to record at position idx and return a reference to it. +Node *Record_AddNode(Record r, int idx, Node node); -// Add an edge to record at position idx. -void Record_AddEdge(Record r, int idx, Edge edge); +// Add an edge to record at position idx and return a reference to it. +Edge *Record_AddEdge(Record r, int idx, Edge edge); // Ensure that all scalar values in record are access-safe. void Record_PersistScalars(Record r); From 59083af162ce17d8d040c5fae5d4dd7c86fa7497 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Fri, 27 Mar 2020 13:21:31 -0400 Subject: [PATCH 16/32] Test null handling --- tests/flow/test_null_handling.py | 63 +++++++++++++++++++ .../features/AggregationAcceptance.feature | 1 - tests/tck/features/ReturnAcceptance.feature | 2 - 3 files changed, 63 insertions(+), 3 deletions(-) create mode 100644 tests/flow/test_null_handling.py diff --git a/tests/flow/test_null_handling.py b/tests/flow/test_null_handling.py new file mode 100644 index 0000000000..b50d0692d8 --- /dev/null +++ b/tests/flow/test_null_handling.py @@ -0,0 +1,63 @@ +import os +import sys +import redis +from RLTest import Env +from redisgraph import Graph, Node, Edge +from base import FlowTestsBase + +redis_graph = None + +class testNullHandlingFlow(FlowTestsBase): + def __init__(self): + self.env = Env() + global redis_graph + redis_con = self.env.getConnection() + redis_graph = Graph("G", redis_con) + self.populate_graph() + + def populate_graph(self): + # Create a single node. + node = Node(label="L", properties={"v": "v1"}) + redis_graph.add_node(node) + redis_graph.commit() + + # Error when attempting to create a relationship with a null endpoint. + def test01_create_null(self): + try: + query = """MATCH (a) OPTIONAL MATCH (a)-[nonexistent_edge]->(nonexistent_node) CREATE (nonexistent_node)-[:E]->(b)""" + redis_graph.query(query) + assert(False) + except redis.exceptions.ResponseError: + # Expecting an error. + pass + + # SET should update attributes on non-null entities and ignore null entities. + def test02_set_null(self): + query = """MATCH (a) OPTIONAL MATCH (a)-[nonexistent_edge]->(nonexistent_node) SET a.v2 = true, nonexistent_node.v2 = true RETURN a.v2, nonexistent_node.v2""" + actual_result = redis_graph.query(query) + # The property should be set on the real node and ignored on the null entity. + assert(actual_result.properties_set == 1) + expected_result = [[True, None]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # DELETE should ignore null entities. + def test03_delete_null(self): + query = """MATCH (a) OPTIONAL MATCH (a)-[nonexistent_edge]->(nonexistent_node) DELETE nonexistent_node""" + actual_result = redis_graph.query(query) + # The property should be set on the real node and ignored on the null entity. + assert(actual_result.nodes_deleted == 0) + + # Functions should handle null inputs appropriately. + def test04_null_function_inputs(self): + query = """MATCH (a) OPTIONAL MATCH (a)-[r]->(b) RETURN type(r), labels(b), b.v * 5""" + actual_result = redis_graph.query(query) + expected_result = [[None, None, None]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # Path functions should handle null inputs appropriately. + def test05_null_named_path_function_inputs(self): + query = """MATCH (a) OPTIONAL MATCH p = (a)-[r]->() RETURN p, length(p), collect(relationships(p))""" + actual_result = redis_graph.query(query) + # The path and function calls on it should return NULL, while collect() returns an empty array. + expected_result = [[None, None, []]] + self.env.assertEquals(actual_result.result_set, expected_result) diff --git a/tests/tck/features/AggregationAcceptance.feature b/tests/tck/features/AggregationAcceptance.feature index b7656c3ec9..b75e2e0e3f 100644 --- a/tests/tck/features/AggregationAcceptance.feature +++ b/tests/tck/features/AggregationAcceptance.feature @@ -111,7 +111,6 @@ Feature: AggregationAcceptance | (:L) | 2 | And no side effects - @skip Scenario: Sort on aggregate function and normal property Given an empty graph And having executed: diff --git a/tests/tck/features/ReturnAcceptance.feature b/tests/tck/features/ReturnAcceptance.feature index 28df8a1f57..f72ca01819 100644 --- a/tests/tck/features/ReturnAcceptance.feature +++ b/tests/tck/features/ReturnAcceptance.feature @@ -108,7 +108,6 @@ Feature: ReturnAcceptanceTest | ({name: 'E'}) | And no side effects -@skip Scenario: Start the result from the second row by param Given an empty graph And having executed: @@ -160,7 +159,6 @@ Feature: ReturnAcceptanceTest | ({name: 'D'}) | And no side effects -@skip Scenario: Get rows in the middle by param Given an empty graph And having executed: From 24cc90a410ec401b42aa8cb810b18ccae5f24fdc Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Fri, 27 Mar 2020 13:56:40 -0400 Subject: [PATCH 17/32] Minor cleanup --- src/Makefile | 2 +- src/arithmetic/entity_funcs/entity_funcs.c | 1 + src/execution_plan/execution_plan.c | 53 +++++++++++----------- src/execution_plan/ops/op_apply.c | 8 ++-- 4 files changed, 32 insertions(+), 32 deletions(-) diff --git a/src/Makefile b/src/Makefile index 69178f558b..f1f750d93d 100644 --- a/src/Makefile +++ b/src/Makefile @@ -154,7 +154,7 @@ ifeq (,$(wildcard $(LIBCYPHER-PARSER))) cd ../deps/libcypher-parser; \ ./autogen.sh; \ ./configure --disable-shared; - $(MAKE) CFLAGS="-g -fPIC -DYY_BUFFER_SIZE=1048576" clean check -C ../deps/libcypher-parser + $(MAKE) CFLAGS="-fPIC -DYY_BUFFER_SIZE=1048576" clean check -C ../deps/libcypher-parser endif .PHONY: $(LIBCYPHER-PARSER) diff --git a/src/arithmetic/entity_funcs/entity_funcs.c b/src/arithmetic/entity_funcs/entity_funcs.c index 3ace5d03b0..bf0eade230 100644 --- a/src/arithmetic/entity_funcs/entity_funcs.c +++ b/src/arithmetic/entity_funcs/entity_funcs.c @@ -15,6 +15,7 @@ /* returns the id of a relationship or node. */ SIValue AR_ID(SIValue *argv, int argc) { + if(argv[0].type == T_NULL) return SI_NullVal(); GraphEntity *graph_entity = (GraphEntity *)argv[0].ptrval; return SI_LongVal(ENTITY_GET_ID(graph_entity)); } diff --git a/src/execution_plan/execution_plan.c b/src/execution_plan/execution_plan.c index 1c009a2ec3..695b2633f7 100644 --- a/src/execution_plan/execution_plan.c +++ b/src/execution_plan/execution_plan.c @@ -527,32 +527,6 @@ static void _buildMergeCreateStream(ExecutionPlan *plan, AST_MergeContext *merge } } -static void _BuildOptionalMatchOps(ExecutionPlan *plan, const cypher_astnode_t *clause) { - // TODO dup of _buildMergeOp logic, consider shared function. - // Collect the variables that are bound at this point. - rax *bound_vars = NULL; - const char **arguments = NULL; - if(plan->root) { - bound_vars = raxNew(); - // Rather than cloning the record map, collect the bound variables along with their - // parser-generated constant strings. - ExecutionPlan_BoundVariables(plan->root, bound_vars); - // Collect the variable names from bound_vars to populate the Argument ops we will build. - arguments = (const char **)raxValues(bound_vars); - } - - OpBase *apply_op = NewApplyOp(plan); - _ExecutionPlan_UpdateRoot(plan, apply_op); - - OpBase *optional = NewOptionalOp(plan); - ExecutionPlan_AddOp(apply_op, optional); - OpBase *match_stream = ExecutionPlan_BuildOpsFromPath(plan, arguments, clause); - ExecutionPlan_AddOp(optional, match_stream); // Add Match stream to Merge op. - - if(bound_vars) raxFree(bound_vars); - array_free(arguments); -} - static void _buildMergeOp(GraphContext *gc, AST *ast, ExecutionPlan *plan, const cypher_astnode_t *clause) { /* @@ -617,6 +591,31 @@ static void _buildMergeOp(GraphContext *gc, AST *ast, ExecutionPlan *plan, array_free(arguments); } +static void _buildOptionalMatchOps(ExecutionPlan *plan, const cypher_astnode_t *clause) { + // Collect the variables that are bound at this point. + rax *bound_vars = raxNew(); + // Rather than cloning the record map, collect the bound variables along with their + // parser-generated constant strings. + ExecutionPlan_BoundVariables(plan->root, bound_vars); + // Collect the variable names from bound_vars to populate the Argument op we will build. + const char **arguments = (const char **)raxValues(bound_vars); + raxFree(bound_vars); + + // Create an Apply operator and make it the new root. + OpBase *apply_op = NewApplyOp(plan); + _ExecutionPlan_UpdateRoot(plan, apply_op); + + // Create an Optional op and add it as an Apply child as a right-hand stream. + OpBase *optional = NewOptionalOp(plan); + ExecutionPlan_AddOp(apply_op, optional); + + // Build the new Match stream and add it to the Optional stream. + OpBase *match_stream = ExecutionPlan_BuildOpsFromPath(plan, arguments, clause); + ExecutionPlan_AddOp(optional, match_stream); + + array_free(arguments); +} + static inline void _buildUpdateOp(ExecutionPlan *plan, const cypher_astnode_t *clause) { EntityUpdateEvalCtx *update_exps = AST_PrepareUpdateOp(clause); OpBase *op = NewUpdateOp(plan, update_exps); @@ -635,7 +634,7 @@ static void _ExecutionPlanSegment_ConvertClause(GraphContext *gc, AST *ast, Exec // Because 't' is set using the offsetof() call, it cannot be used in switch statements. if(t == CYPHER_AST_MATCH) { if(cypher_ast_match_is_optional(clause)) { - _BuildOptionalMatchOps(plan, clause); + _buildOptionalMatchOps(plan, clause); return; } // Only add at most one set of traversals per plan. TODO Revisit and improve this logic. diff --git a/src/execution_plan/ops/op_apply.c b/src/execution_plan/ops/op_apply.c index c4d8336004..15a8fe6cd8 100644 --- a/src/execution_plan/ops/op_apply.c +++ b/src/execution_plan/ops/op_apply.c @@ -27,18 +27,18 @@ OpBase *NewApplyOp(const ExecutionPlan *plan) { } static OpResult ApplyInit(OpBase *opBase) { - // TODO init routine necesssary? assert(opBase->childCount == 2); Apply *op = (Apply *)opBase; - /* The op bounded branch and match branch are set to be the first and second child, respectively, - * during the operation building procedure at execution_plan_reduce_to_apply.c */ + /* The op's bound branch and optional match branch have already been built as + * the Apply op's first and second child ops, respectively. */ op->bound_branch = opBase->children[0]; op->rhs_branch = opBase->children[1]; // Locate branch's Argument op tap. op->op_arg = (Argument *)ExecutionPlan_LocateOp(op->rhs_branch, OPType_ARGUMENT); - assert(op->op_arg && op->op_arg->op.childCount == 0); // TODO maybe wrong + assert(op->op_arg && op->op_arg->op.childCount == 0); + return OP_OK; } From 98f057401a092958d2194745b4bd675ba6c36284 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Mon, 30 Mar 2020 13:28:09 -0400 Subject: [PATCH 18/32] Add documentation --- docs/commands.md | 42 ++++++++++++++++++++++++++++++++++++++++++ docs/cypher_support.md | 6 +----- 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index 877e0a3e21..a6da24d3fa 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -23,6 +23,7 @@ supported. ### Query structure - MATCH +- OPTIONAL MATCH - WHERE - RETURN - ORDER BY @@ -128,6 +129,47 @@ The syntactic sugar `(person_a)<-[:KNOWS]->(person_b)` will return the same resu The bracketed edge description can be omitted if all relations should be considered: `(person_a)--(person_b)`. +#### OPTIONAL MATCH + +The OPTIONAL MATCH clause is a MATCH variant that produces null values for elements that do not match successfully, rather than the all-or-nothing logic for patterns in MATCH clauses. + +It can be considered to fill the same role as LEFT/RIGHT JOIN does in SQL, as MATCH entities must be resolved but nodes and edges introduced in OPTIONAL MATCH will be returned as nulls if they cannot be found. + +OPTIONAL MATCH clauses accept the same patterns as standard MATCH clauses, and may similarly be modified by WHERE clauses. + +RedisGraph does not currently support beginning a query with an OPTIONAL MATCH, as this construction is considered an antipattern in Cypher and can lead to unexpected results. + +Multiple MATCH and OPTIONAL MATCH clauses can be chained together, though a mandatory MATCH cannot follow an optional one. + +```sh +GRAPH.QUERY DEMO_GRAPH +"MATCH (p:Person) OPTIONAL MATCH (p)-[w:WORKS_AT]->(c:Company) +WHERE w.start_date > 2016 +RETURN p, w, c" +``` + +All `Person` nodes are returned, as well as any `WORKS_AT` relations and `Company` nodes that can be resolved and satisfy the `start_date` constraint. For each `Person` that does not resolve the optional pattern, the person will be returned as normal and the non-matching elements will be returned as null. + +Cypher is lenient in its handling of null values, so actions like property accesses and function calls on null values will return null values rather than emit errors. + +```sh +GRAPH.QUERY DEMO_GRAPH +"MATCH (p:Person) OPTIONAL MATCH (p)-[w:WORKS_AT]->(c:Company) +RETURN p, w.department, ID(c) as ID" +``` + +In this case, `w.department` and `ID` will be returned if the OPTIONAL MATCH was successful, and will be null otherwise. + +Clauses like SET, CREATE, and DELETE will ignore null inputs and perform the expected updates on real inputs. One exception to this is that attempting to create a relation with a null endpoint will cause an error: + +```sh +GRAPH.QUERY DEMO_GRAPH +"MATCH (p:Person) OPTIONAL MATCH (p)-[w:WORKS_AT]->(c:Company) +CREATE (c)-[:NEW_RELATION]->(:NEW_NODE)" +``` + +If `c` is null for any record, this query will emit an error. In this case, no changes to the graph are committed, even if some values for `c` were resolved. + #### WHERE This clause is not mandatory, but if you want to filter results, you can specify your predicates here. diff --git a/docs/cypher_support.md b/docs/cypher_support.md index 4369cd48ad..86bd7da2c2 100644 --- a/docs/cypher_support.md +++ b/docs/cypher_support.md @@ -42,11 +42,7 @@ We do not support any of these properties at the type level, meaning nodes and r ## Clauses ### Reading Clauses + MATCH - - **Unsupported:** - -- OPTIONAL MATCH -- MANDATORY MATCH ++ OPTIONAL MATCH ### Projecting Clauses + RETURN From 0cbd1a634a465552f111b0afa3e8fb7628c15ab5 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Mon, 30 Mar 2020 14:34:49 -0400 Subject: [PATCH 19/32] Simplify toPath null handling --- src/arithmetic/path_funcs/path_funcs.c | 11 +++++------ src/datatypes/path/sipath_builder.c | 5 ----- src/datatypes/path/sipath_builder.h | 3 --- tests/tck/features/DeleteAcceptance.feature | 1 - 4 files changed, 5 insertions(+), 15 deletions(-) diff --git a/src/arithmetic/path_funcs/path_funcs.c b/src/arithmetic/path_funcs/path_funcs.c index 5a0ca11320..726becf8a4 100644 --- a/src/arithmetic/path_funcs/path_funcs.c +++ b/src/arithmetic/path_funcs/path_funcs.c @@ -23,15 +23,14 @@ SIValue AR_TOPATH(SIValue *argv, int argc) { const cypher_astnode_t *ast_path = argv[0].ptrval; uint nelements = cypher_ast_pattern_path_nelements(ast_path); assert(argc == (nelements + 1)); + for(uint i = 1; i < argc; i++) { + // If any element of the path does not exist, the entire path is invalid and NULL should be returned. + if(argv[i].type == T_NULL) return SI_NullVal(); + } + SIValue path = SIPathBuilder_New(nelements); for(uint i = 0; i < nelements; i++) { SIValue element = argv[i + 1]; - if(element.type == T_NULL) { - // If any element of the path does not exist, the entire path is invalid and should be cleared. - SIPathBuilder_ClearPath(path); - return SI_NullVal(); - } - if(i % 2 == 0) { // Nodes are in even position. SIPathBuilder_AppendNode(path, element); diff --git a/src/datatypes/path/sipath_builder.c b/src/datatypes/path/sipath_builder.c index 3cafc7efa3..32e14306f6 100644 --- a/src/datatypes/path/sipath_builder.c +++ b/src/datatypes/path/sipath_builder.c @@ -91,8 +91,3 @@ void SIPathBuilder_AppendPath(SIValue path, SIValue new_path, bool RTLEdge) { } SIPathBuilder_AppendEdge(path, SIPath_GetRelationship(new_path, new_path_edge_count - 1), RTLEdge); } - -void SIPathBuilder_ClearPath(SIValue path) { - Path_Free(path.ptrval); -} - diff --git a/src/datatypes/path/sipath_builder.h b/src/datatypes/path/sipath_builder.h index 7c44e512c2..97f6246d0b 100644 --- a/src/datatypes/path/sipath_builder.h +++ b/src/datatypes/path/sipath_builder.h @@ -58,6 +58,3 @@ void SIPathBuilder_AppendEdge(SIValue p, SIValue e, bool RTLEdge); * @param RTLEdge: Indicates edges direction if the path (RTL in query, incoming or outgoing). */ void SIPathBuilder_AppendPath(SIValue p, SIValue other, bool RTLEdge); - -void SIPathBuilder_ClearPath(SIValue path); - diff --git a/tests/tck/features/DeleteAcceptance.feature b/tests/tck/features/DeleteAcceptance.feature index 6b31a953d3..f62cd77f39 100644 --- a/tests/tck/features/DeleteAcceptance.feature +++ b/tests/tck/features/DeleteAcceptance.feature @@ -195,7 +195,6 @@ Feature: DeleteAcceptance Then the result should be empty And no side effects -@skip Scenario: Delete optionally matched relationship Given an empty graph And having executed: From c312e9a7f2c29688e05d51aa8a2f9d4f2450804f Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Mon, 30 Mar 2020 15:00:15 -0400 Subject: [PATCH 20/32] Improve comments --- src/ast/ast_validations.c | 9 +++++---- src/execution_plan/ops/op_conditional_traverse.c | 2 +- src/execution_plan/ops/op_delete.c | 3 +-- src/execution_plan/ops/op_optional.c | 7 ++++++- src/execution_plan/ops/op_optional.h | 2 +- src/graph/query_graph.c | 1 - 6 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/ast/ast_validations.c b/src/ast/ast_validations.c index c05af7312a..53ed8a9199 100644 --- a/src/ast/ast_validations.c +++ b/src/ast/ast_validations.c @@ -1024,10 +1024,10 @@ static AST_Validation _ValidateQuerySequence(const AST *ast, char **reason) { return AST_VALID; } -// In any given query scope, reading clauses (MATCH, UNWIND, and InQueryCall) -// cannot follow updating clauses (CREATE, MERGE, DELETE, SET, REMOVE). -// https://s3.amazonaws.com/artifacts.opencypher.org/railroad/SinglePartQuery.html -// Additionally, a MATCH clause cannot follow an optional MATCH clause. +/* In any given query scope, reading clauses (MATCH, UNWIND, and InQueryCall) + * cannot follow updating clauses (CREATE, MERGE, DELETE, SET, REMOVE). + * https://s3.amazonaws.com/artifacts.opencypher.org/railroad/SinglePartQuery.html + * Additionally, a MATCH clause cannot follow an OPTIONAL MATCH clause. */ static AST_Validation _ValidateClauseOrder(const AST *ast, char **reason) { uint clause_count = cypher_ast_query_nclauses(ast->root); @@ -1694,3 +1694,4 @@ AST_Validation AST_Validate_QueryParams(RedisModuleCtx *ctx, const cypher_parse_ return AST_VALID; } + diff --git a/src/execution_plan/ops/op_conditional_traverse.c b/src/execution_plan/ops/op_conditional_traverse.c index bc597f917f..9689da049f 100644 --- a/src/execution_plan/ops/op_conditional_traverse.c +++ b/src/execution_plan/ops/op_conditional_traverse.c @@ -214,7 +214,7 @@ static Record CondTraverseConsume(OpBase *opBase) { /* Get node from current column. */ op->r = op->records[src_id]; - Node destNode; + Node destNode = {}; Graph_GetNode(op->graph, dest_id, &destNode); Record_AddNode(op->r, op->destNodeIdx, destNode); diff --git a/src/execution_plan/ops/op_delete.c b/src/execution_plan/ops/op_delete.c index 80925a6e50..ef14581379 100644 --- a/src/execution_plan/ops/op_delete.c +++ b/src/execution_plan/ops/op_delete.c @@ -86,8 +86,7 @@ static Record DeleteConsume(OpBase *opBase) { Edge *e = (Edge *)value.ptrval; op->deleted_edges = array_append(op->deleted_edges, *e); } else if(SI_TYPE(value) & T_NULL) { - // Ignore null values. - continue; + continue; // Ignore null values. } else { /* Expression evaluated to a non-graph entity type * clear pending deletions and raise an exception. */ diff --git a/src/execution_plan/ops/op_optional.c b/src/execution_plan/ops/op_optional.c index 2b6f55bbed..244bab6602 100644 --- a/src/execution_plan/ops/op_optional.c +++ b/src/execution_plan/ops/op_optional.c @@ -25,8 +25,13 @@ OpBase *NewOptionalOp(const ExecutionPlan *plan) { static Record OptionalConsume(OpBase *opBase) { Optional *op = (Optional *)opBase; + // Try to produce a Record from the child op. Record r = OpBase_Consume(opBase->children[0]); - if(!r && op->emitted_record == false) r = OpBase_CreateRecord(opBase); + + // Create an empty Record if the child returned NULL and this op has not yet returned data. + if(!r && !op->emitted_record) r = OpBase_CreateRecord(opBase); + + // Don't produce multiple empty Records. op->emitted_record = true; return r; diff --git a/src/execution_plan/ops/op_optional.h b/src/execution_plan/ops/op_optional.h index 6db31c2d2c..3de9e1a57d 100644 --- a/src/execution_plan/ops/op_optional.h +++ b/src/execution_plan/ops/op_optional.h @@ -13,7 +13,7 @@ * If the child produces no records, Optional emits an empty Record exactly once. */ typedef struct { OpBase op; - bool emitted_record; + bool emitted_record; // True if this operation has returned at least one Record. } Optional; /* Creates a new Optional operation. */ diff --git a/src/graph/query_graph.c b/src/graph/query_graph.c index 28904ced4a..d9da8e7465 100644 --- a/src/graph/query_graph.c +++ b/src/graph/query_graph.c @@ -157,7 +157,6 @@ QueryGraph *BuildQueryGraph(const GraphContext *gc, const AST *ast) { for(uint i = 0; i < match_count; i ++) { // OPTIONAL MATCH clauses are handled separately. if(cypher_ast_match_is_optional(match_clauses[i])) continue; - // if(match_count > 1 && cypher_ast_match_is_optional(match_clauses[i])) continue; const cypher_astnode_t *pattern = cypher_ast_match_get_pattern(match_clauses[i]); uint npaths = cypher_ast_pattern_npaths(pattern); for(uint j = 0; j < npaths; j ++) { From de7a126ca538d7073962cd02c1246f82f2ebe808 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Mon, 30 Mar 2020 16:17:13 -0400 Subject: [PATCH 21/32] Allow OPTIONAL MATCH as first clause --- docs/commands.md | 2 - src/ast/ast_validations.c | 9 ----- src/execution_plan/execution_plan.c | 37 +++++++++++-------- src/execution_plan/ops/op_expand_into.c | 11 +++++- .../features/AggregationAcceptance.feature | 1 - tests/tck/features/DeleteAcceptance.feature | 3 -- tests/tck/features/MatchAcceptance2.feature | 5 --- tests/tck/features/NullAcceptance.feature | 4 -- tests/tck/features/NullOperator.feature | 4 -- .../features/OptionalMatchAcceptance.feature | 4 -- tests/tck/features/WithAcceptance.feature | 1 - 11 files changed, 31 insertions(+), 50 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index a6da24d3fa..83fa250e29 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -137,8 +137,6 @@ It can be considered to fill the same role as LEFT/RIGHT JOIN does in SQL, as MA OPTIONAL MATCH clauses accept the same patterns as standard MATCH clauses, and may similarly be modified by WHERE clauses. -RedisGraph does not currently support beginning a query with an OPTIONAL MATCH, as this construction is considered an antipattern in Cypher and can lead to unexpected results. - Multiple MATCH and OPTIONAL MATCH clauses can be chained together, though a mandatory MATCH cannot follow an optional one. ```sh diff --git a/src/ast/ast_validations.c b/src/ast/ast_validations.c index 53ed8a9199..80da5522f8 100644 --- a/src/ast/ast_validations.c +++ b/src/ast/ast_validations.c @@ -1012,15 +1012,6 @@ static AST_Validation _ValidateQuerySequence(const AST *ast, char **reason) { return AST_INVALID; } - /* Disallow queries beginning with an OPTIONAL MATCH. - * Cypher does allow this construction, but it is considered an antipattern - * and introduces a lot of edge cases. We can add support for this later if desired. */ - if(cypher_astnode_type(start_clause) == CYPHER_AST_MATCH && - cypher_ast_match_is_optional(start_clause)) { - asprintf(reason, "RedisGraph does not support beginning a query with OPTIONAL MATCH."); - return AST_INVALID; - } - return AST_VALID; } diff --git a/src/execution_plan/execution_plan.c b/src/execution_plan/execution_plan.c index 695b2633f7..846f374ba6 100644 --- a/src/execution_plan/execution_plan.c +++ b/src/execution_plan/execution_plan.c @@ -592,27 +592,34 @@ static void _buildMergeOp(GraphContext *gc, AST *ast, ExecutionPlan *plan, } static void _buildOptionalMatchOps(ExecutionPlan *plan, const cypher_astnode_t *clause) { - // Collect the variables that are bound at this point. - rax *bound_vars = raxNew(); - // Rather than cloning the record map, collect the bound variables along with their - // parser-generated constant strings. - ExecutionPlan_BoundVariables(plan->root, bound_vars); - // Collect the variable names from bound_vars to populate the Argument op we will build. - const char **arguments = (const char **)raxValues(bound_vars); - raxFree(bound_vars); - - // Create an Apply operator and make it the new root. - OpBase *apply_op = NewApplyOp(plan); - _ExecutionPlan_UpdateRoot(plan, apply_op); - - // Create an Optional op and add it as an Apply child as a right-hand stream. OpBase *optional = NewOptionalOp(plan); - ExecutionPlan_AddOp(apply_op, optional); + const char **arguments = NULL; + // The root will be non-null unless the first clause is an OPTIONAL MATCH. + if(plan->root) { + // Collect the variables that are bound at this point. + rax *bound_vars = raxNew(); + // Rather than cloning the record map, collect the bound variables along with their + // parser-generated constant strings. + ExecutionPlan_BoundVariables(plan->root, bound_vars); + // Collect the variable names from bound_vars to populate the Argument op we will build. + arguments = (const char **)raxValues(bound_vars); + raxFree(bound_vars); + + // Create an Apply operator and make it the new root. + OpBase *apply_op = NewApplyOp(plan); + _ExecutionPlan_UpdateRoot(plan, apply_op); + + // Create an Optional op and add it as an Apply child as a right-hand stream. + ExecutionPlan_AddOp(apply_op, optional); + } // Build the new Match stream and add it to the Optional stream. OpBase *match_stream = ExecutionPlan_BuildOpsFromPath(plan, arguments, clause); ExecutionPlan_AddOp(optional, match_stream); + // If no root has been set (OPTIONAL was the first clause), set it to the Optional op. + if(!plan->root) _ExecutionPlan_UpdateRoot(plan, optional); + array_free(arguments); } diff --git a/src/execution_plan/ops/op_expand_into.c b/src/execution_plan/ops/op_expand_into.c index 021d0e337a..1a24a3a429 100644 --- a/src/execution_plan/ops/op_expand_into.c +++ b/src/execution_plan/ops/op_expand_into.c @@ -58,6 +58,7 @@ static void _populate_filter_matrix(OpExpandInto *op) { /* Update filter matrix F, set row i at position srcId * F[i, srcId] = true. */ Node *n = Record_GetNode(r, op->srcNodeIdx); + if(!n) continue; // The expected entity may not be found on optional expansions. NodeID srcId = ENTITY_GET_ID(n); GrB_Matrix_setElement_BOOL(op->F, true, i, srcId); } @@ -156,10 +157,16 @@ static Record _handoff(OpExpandInto *op) { // Current record resides at row recordCount. int rowIdx = op->recordCount; op->r = op->records[op->recordCount]; - assert(Record_GetType(op->r, op->srcNodeIdx) == REC_TYPE_NODE); - assert(Record_GetType(op->r, op->destNodeIdx) == REC_TYPE_NODE); srcNode = Record_GetNode(op->r, op->srcNodeIdx); destNode = Record_GetNode(op->r, op->destNodeIdx); + if(!srcNode || !destNode) { + // An endpoint may not resolve if an Optional op failed to match, + // in which case we can skip this record. + // Mark as NULL to avoid double free. + op->records[op->recordCount] = NULL; + continue; + + } srcId = ENTITY_GET_ID(srcNode); destId = ENTITY_GET_ID(destNode); bool x; diff --git a/tests/tck/features/AggregationAcceptance.feature b/tests/tck/features/AggregationAcceptance.feature index b75e2e0e3f..e5c6b7575b 100644 --- a/tests/tck/features/AggregationAcceptance.feature +++ b/tests/tck/features/AggregationAcceptance.feature @@ -207,7 +207,6 @@ Feature: AggregationAcceptance | () | 1.0 | And no side effects - @skip Scenario: Distinct on unbound node Given an empty graph When executing query: diff --git a/tests/tck/features/DeleteAcceptance.feature b/tests/tck/features/DeleteAcceptance.feature index f62cd77f39..4975ebf5eb 100644 --- a/tests/tck/features/DeleteAcceptance.feature +++ b/tests/tck/features/DeleteAcceptance.feature @@ -211,7 +211,6 @@ Feature: DeleteAcceptance And the side effects should be: | -nodes | 1 | -@skip Scenario: Delete on null node Given an empty graph When executing query: @@ -222,7 +221,6 @@ Feature: DeleteAcceptance Then the result should be empty And no side effects -@skip Scenario: Detach delete on null node Given an empty graph When executing query: @@ -233,7 +231,6 @@ Feature: DeleteAcceptance Then the result should be empty And no side effects -@skip Scenario: Delete on null path Given an empty graph When executing query: diff --git a/tests/tck/features/MatchAcceptance2.feature b/tests/tck/features/MatchAcceptance2.feature index d0eb1066e9..cca6f2455d 100644 --- a/tests/tck/features/MatchAcceptance2.feature +++ b/tests/tck/features/MatchAcceptance2.feature @@ -628,7 +628,6 @@ Feature: MatchAcceptance2 | <(:B)<-[:T]-(:A)> | And no side effects -@skip Scenario: Simple OPTIONAL MATCH on empty graph Given an empty graph When executing query: @@ -920,7 +919,6 @@ Feature: MatchAcceptance2 | [[:T]] | And no side effects -@skip Scenario: Matching from null nodes should return no results owing to finding no matches Given an empty graph When executing query: @@ -934,7 +932,6 @@ Feature: MatchAcceptance2 | b | And no side effects -@skip Scenario: Matching from null nodes should return no results owing to matches being filtered out Given an empty graph And having executed: @@ -952,7 +949,6 @@ Feature: MatchAcceptance2 | b | And no side effects -@skip Scenario: Optionally matching from null nodes should return null Given an empty graph When executing query: @@ -967,7 +963,6 @@ Feature: MatchAcceptance2 | null | And no side effects -@skip Scenario: OPTIONAL MATCH returns null Given an empty graph When executing query: diff --git a/tests/tck/features/NullAcceptance.feature b/tests/tck/features/NullAcceptance.feature index 278ba2815e..18c36a7ec1 100755 --- a/tests/tck/features/NullAcceptance.feature +++ b/tests/tck/features/NullAcceptance.feature @@ -47,7 +47,6 @@ Feature: NullAcceptance | false | true | And no side effects -@skip Scenario: Property existence check on optional non-null node Given an empty graph And having executed: @@ -78,7 +77,6 @@ Feature: NullAcceptance | null | And no side effects -@skip Scenario: Ignore null when setting property Given an empty graph When executing query: @@ -162,7 +160,6 @@ Feature: NullAcceptance | null | And no side effects -@skip Scenario: Ignore null when deleting node Given an empty graph When executing query: @@ -176,7 +173,6 @@ Feature: NullAcceptance | null | And no side effects -@skip Scenario: Ignore null when deleting relationship Given an empty graph When executing query: diff --git a/tests/tck/features/NullOperator.feature b/tests/tck/features/NullOperator.feature index 9bcf0a663f..220a547061 100755 --- a/tests/tck/features/NullOperator.feature +++ b/tests/tck/features/NullOperator.feature @@ -64,7 +64,6 @@ Feature: NullOperator | false | true | And no side effects - @skip Scenario: Property null check on optional non-null node Given an empty graph And having executed: @@ -82,7 +81,6 @@ Feature: NullOperator | true | false | And no side effects - @skip Scenario: Property not null check on optional non-null node Given an empty graph And having executed: @@ -100,7 +98,6 @@ Feature: NullOperator | false | true | And no side effects - @skip Scenario: Property null check on null node Given an empty graph When executing query: @@ -113,7 +110,6 @@ Feature: NullOperator | true | And no side effects - @skip Scenario: Property not null check on null node Given an empty graph When executing query: diff --git a/tests/tck/features/OptionalMatchAcceptance.feature b/tests/tck/features/OptionalMatchAcceptance.feature index e6ce413bbe..17821f0266 100644 --- a/tests/tck/features/OptionalMatchAcceptance.feature +++ b/tests/tck/features/OptionalMatchAcceptance.feature @@ -108,7 +108,6 @@ Feature: OptionalMatchAcceptance | d | And no side effects -@skip Scenario: WITH after OPTIONAL MATCH When executing query: """ @@ -295,7 +294,6 @@ Feature: OptionalMatchAcceptance | (:C) | null | And no side effects -@skip Scenario: Handling optional matches between optionally matched entities When executing query: """ @@ -311,7 +309,6 @@ Feature: OptionalMatchAcceptance | null | (:B {num: 46}) | null | And no side effects -@skip Scenario: Handling optional matches between nulls When executing query: """ @@ -326,7 +323,6 @@ Feature: OptionalMatchAcceptance | null | null | null | And no side effects -@skip Scenario: OPTIONAL MATCH and `collect()` And having executed: """ diff --git a/tests/tck/features/WithAcceptance.feature b/tests/tck/features/WithAcceptance.feature index e1c718da1d..5d8d837c4c 100644 --- a/tests/tck/features/WithAcceptance.feature +++ b/tests/tck/features/WithAcceptance.feature @@ -286,7 +286,6 @@ Feature: WithAcceptance | (:A) | [:REL] | (:B) | And no side effects - @skip Scenario: Null handling Given an empty graph When executing query: From 7defbfd8140ca22a87147bbe02bc9ed60714ec40 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Mon, 30 Mar 2020 16:38:57 -0400 Subject: [PATCH 22/32] Simplify null-checking logic in create ops --- src/execution_plan/ops/op_create.c | 10 ++++------ src/execution_plan/ops/op_merge_create.c | 11 +++++------ 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/execution_plan/ops/op_create.c b/src/execution_plan/ops/op_create.c index fd61731e4a..1c6b15fc5a 100644 --- a/src/execution_plan/ops/op_create.c +++ b/src/execution_plan/ops/op_create.c @@ -77,19 +77,17 @@ static void _CreateEdges(OpCreate *op, Record r) { /* Get specified edge to create. */ QGEdge *e = op->pending.edges_to_create[i].edge; + /* Retrieve source and dest nodes. */ + Node *src_node = Record_GetNode(r, op->pending.edges_to_create[i].src_idx); + Node *dest_node = Record_GetNode(r, op->pending.edges_to_create[i].dest_idx); /* Verify that the endpoints of the new edge resolved properly; fail otherwise. */ - if((Record_GetType(r, op->pending.edges_to_create[i].src_idx) != REC_TYPE_NODE) || - (Record_GetType(r, op->pending.edges_to_create[i].dest_idx) != REC_TYPE_NODE)) { + if(!src_node || !dest_node) { char *error; asprintf(&error, "Failed to create relationship; endpoint was not found."); QueryCtx_SetError(error); QueryCtx_RaiseRuntimeException(); } - /* Retrieve source and dest nodes. */ - Node *src_node = Record_GetNode(r, op->pending.edges_to_create[i].src_idx); - Node *dest_node = Record_GetNode(r, op->pending.edges_to_create[i].dest_idx); - /* Create the actual edge. */ Edge newEdge = {}; if(array_len(e->reltypes) > 0) newEdge.relationship = e->reltypes[0]; diff --git a/src/execution_plan/ops/op_merge_create.c b/src/execution_plan/ops/op_merge_create.c index 9fd7798467..d27b05657c 100644 --- a/src/execution_plan/ops/op_merge_create.c +++ b/src/execution_plan/ops/op_merge_create.c @@ -121,19 +121,18 @@ static bool _CreateEntities(OpMergeCreate *op, Record r) { /* Get specified edge to create. */ QGEdge *e = op->pending.edges_to_create[i].edge; + /* Retrieve source and dest nodes. */ + Node *src_node = Record_GetNode(r, op->pending.edges_to_create[i].src_idx); + Node *dest_node = Record_GetNode(r, op->pending.edges_to_create[i].dest_idx); + /* Verify that the endpoints of the new edge resolved properly; fail otherwise. */ - if((Record_GetType(r, op->pending.edges_to_create[i].src_idx) != REC_TYPE_NODE) || - (Record_GetType(r, op->pending.edges_to_create[i].dest_idx) != REC_TYPE_NODE)) { + if(!src_node || !dest_node) { char *error; asprintf(&error, "Failed to create relationship; endpoint was not found."); QueryCtx_SetError(error); QueryCtx_RaiseRuntimeException(); } - /* Retrieve source and dest nodes. */ - Node *src_node = Record_GetNode(r, op->pending.edges_to_create[i].src_idx); - Node *dest_node = Record_GetNode(r, op->pending.edges_to_create[i].dest_idx); - /* Create the actual edge. */ Edge newEdge = {}; if(array_len(e->reltypes) > 0) newEdge.relationship = e->reltypes[0]; From 61f4965efadb51821b1e1422f2ca89b140c5bdca Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Mon, 30 Mar 2020 17:56:05 -0400 Subject: [PATCH 23/32] Use branch of Python client for testing --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index bd3487f6e9..181593ae20 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -27,7 +27,7 @@ jobs: - run: name: Install prerequisite command: >- - pip install git+https://github.com/RedisGraph/redisgraph-py.git@master && + pip install git+https://github.com/RedisGraph/redisgraph-py.git@null-graph-entities && pip install psutil behave && pip install redis-py-cluster && pip install git+https://github.com/RedisLabsModules/RLTest.git@master && From a55e6324177a20f5e3377ca80bcad48ee43e05f9 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Tue, 31 Mar 2020 09:58:33 -0400 Subject: [PATCH 24/32] PR fixes --- docs/commands.md | 2 +- src/arithmetic/entity_funcs/entity_funcs.c | 12 +- src/arithmetic/list_funcs/list_funcs.c | 44 +++--- src/arithmetic/path_funcs/path_funcs.c | 10 +- src/execution_plan/record.c | 2 - tests/flow/test_null_handling.py | 144 +++++++++++++++++- tests/flow/test_optional_match.py | 16 +- .../features/TernaryLogicAcceptance.feature | 1 - 8 files changed, 188 insertions(+), 43 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index 83fa250e29..9eac3a7817 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -158,7 +158,7 @@ RETURN p, w.department, ID(c) as ID" In this case, `w.department` and `ID` will be returned if the OPTIONAL MATCH was successful, and will be null otherwise. -Clauses like SET, CREATE, and DELETE will ignore null inputs and perform the expected updates on real inputs. One exception to this is that attempting to create a relation with a null endpoint will cause an error: +Clauses like SET, CREATE, MERGE, and DELETE will ignore null inputs and perform the expected updates on real inputs. One exception to this is that attempting to create a relation with a null endpoint will cause an error: ```sh GRAPH.QUERY DEMO_GRAPH diff --git a/src/arithmetic/entity_funcs/entity_funcs.c b/src/arithmetic/entity_funcs/entity_funcs.c index bf0eade230..788912b1e1 100644 --- a/src/arithmetic/entity_funcs/entity_funcs.c +++ b/src/arithmetic/entity_funcs/entity_funcs.c @@ -15,14 +15,14 @@ /* returns the id of a relationship or node. */ SIValue AR_ID(SIValue *argv, int argc) { - if(argv[0].type == T_NULL) return SI_NullVal(); + if(SI_TYPE(argv[0]) == T_NULL) return SI_NullVal(); GraphEntity *graph_entity = (GraphEntity *)argv[0].ptrval; return SI_LongVal(ENTITY_GET_ID(graph_entity)); } /* returns a string representations the label of a node. */ SIValue AR_LABELS(SIValue *argv, int argc) { - if(argv[0].type == T_NULL) return SI_NullVal(); + if(SI_TYPE(argv[0]) == T_NULL) return SI_NullVal(); char *label = ""; Node *node = argv[0].ptrval; GraphContext *gc = QueryCtx_GetGraphCtx(); @@ -34,7 +34,7 @@ SIValue AR_LABELS(SIValue *argv, int argc) { /* returns a string representation of the type of a relation. */ SIValue AR_TYPE(SIValue *argv, int argc) { - if(argv[0].type == T_NULL) return SI_NullVal(); + if(SI_TYPE(argv[0]) == T_NULL) return SI_NullVal(); char *type = ""; Edge *e = argv[0].ptrval; GraphContext *gc = QueryCtx_GetGraphCtx(); @@ -54,7 +54,7 @@ SIValue AR_EXISTS(SIValue *argv, int argc) { } SIValue _AR_NodeDegree(SIValue *argv, int argc, GRAPH_EDGE_DIR dir) { - if(argv[0].type == T_NULL) return SI_NullVal(); + if(SI_TYPE(argv[0]) == T_NULL) return SI_NullVal(); Node *n = (Node *)argv[0].ptrval; Edge *edges = array_new(Edge, 0); GraphContext *gc = QueryCtx_GetGraphCtx(); @@ -83,13 +83,13 @@ SIValue _AR_NodeDegree(SIValue *argv, int argc, GRAPH_EDGE_DIR dir) { /* Returns the number of incoming edges for given node. */ SIValue AR_INCOMEDEGREE(SIValue *argv, int argc) { - if(argv[0].type == T_NULL) return SI_NullVal(); + if(SI_TYPE(argv[0]) == T_NULL) return SI_NullVal(); return _AR_NodeDegree(argv, argc, GRAPH_EDGE_DIR_INCOMING); } /* Returns the number of outgoing edges for given node. */ SIValue AR_OUTGOINGDEGREE(SIValue *argv, int argc) { - if(argv[0].type == T_NULL) return SI_NullVal(); + if(SI_TYPE(argv[0]) == T_NULL) return SI_NullVal(); return _AR_NodeDegree(argv, argc, GRAPH_EDGE_DIR_OUTGOING); } diff --git a/src/arithmetic/list_funcs/list_funcs.c b/src/arithmetic/list_funcs/list_funcs.c index dc9935bfe4..5eae3abbcf 100644 --- a/src/arithmetic/list_funcs/list_funcs.c +++ b/src/arithmetic/list_funcs/list_funcs.c @@ -25,7 +25,9 @@ SIValue AR_TOLIST(SIValue *argv, int argc) { Invalid index will return null. "RETURN [1, 2, 3][0]" will yield 1. */ SIValue AR_SUBSCRIPT(SIValue *argv, int argc) { - assert(argc == 2 && argv[0].type == T_ARRAY && argv[1].type == T_INT64); + assert(argc == 2); + if(SI_TYPE(argv[0]) == T_NULL || SI_TYPE(argv[1]) == T_NULL) return SI_NullVal(); + assert(SI_TYPE(argv[0]) == T_ARRAY && SI_TYPE(argv[1]) == T_INT64); SIValue list = argv[0]; int32_t index = (int32_t)argv[1].longval; uint32_t arrayLen = SIArray_Length(list); @@ -48,9 +50,13 @@ SIValue AR_SUBSCRIPT(SIValue *argv, int argc) { If one of the indices is null, null will be returnd. "RETURN [1, 2, 3][0..1]" will yield [1, 2] */ SIValue AR_SLICE(SIValue *argv, int argc) { - assert(argc == 3 && argv[0].type == T_ARRAY); - if(argv[0].type == T_NULL || argv[1].type == T_NULL || argv[2].type == T_NULL) return SI_NullVal(); - assert(argv[1].type == T_INT64 && argv[2].type == T_INT64); + assert(argc == 3); + if(SI_TYPE(argv[0]) == T_NULL || + SI_TYPE(argv[1]) == T_NULL || + SI_TYPE(argv[2]) == T_NULL) { + return SI_NullVal(); + } + assert(SI_TYPE(argv[0]) == T_ARRAY && SI_TYPE(argv[1]) == T_INT64 && SI_TYPE(argv[2]) == T_INT64); SIValue array = argv[0]; // get array length @@ -92,7 +98,7 @@ SIValue AR_RANGE(SIValue *argv, int argc) { int64_t end = argv[1].longval; int64_t interval = 1; if(argc == 3) { - assert(argv[2].type == T_INT64); + assert(SI_TYPE(argv[2]) == T_INT64); interval = argv[2].longval; if(interval < 1) { char *error; @@ -114,7 +120,9 @@ SIValue AR_RANGE(SIValue *argv, int argc) { /* Checks if a value is in a given list. "RETURN 3 IN [1, 2, 3]" will return true */ SIValue AR_IN(SIValue *argv, int argc) { - assert(argc == 2 && argv[1].type == T_ARRAY); + assert(argc == 2); + if(SI_TYPE(argv[1]) == T_NULL) return SI_NullVal(); + assert(SI_TYPE(argv[1]) == T_ARRAY); SIValue lookupValue = argv[0]; SIValue lookupList = argv[1]; // indicate if there was a null comparison during the array scan @@ -139,7 +147,7 @@ SIValue AR_IN(SIValue *argv, int argc) { SIValue AR_SIZE(SIValue *argv, int argc) { assert(argc == 1); SIValue value = argv[0]; - switch(value.type) { + switch(SI_TYPE(value)) { case T_ARRAY: return SI_LongVal(SIArray_Length(value)); case T_STRING: @@ -156,8 +164,8 @@ SIValue AR_SIZE(SIValue *argv, int argc) { SIValue AR_HEAD(SIValue *argv, int argc) { assert(argc == 1); SIValue value = argv[0]; - if(value.type == T_NULL) return SI_NullVal(); - assert(value.type == T_ARRAY); + if(SI_TYPE(value) == T_NULL) return SI_NullVal(); + assert(SI_TYPE(value) == T_ARRAY); uint arrayLen = SIArray_Length(value); if(arrayLen == 0) return SI_NullVal(); return SIArray_Get(value, 0); @@ -168,8 +176,8 @@ SIValue AR_HEAD(SIValue *argv, int argc) { SIValue AR_TAIL(SIValue *argv, int argc) { assert(argc == 1); SIValue value = argv[0]; - if(value.type == T_NULL) return SI_NullVal(); - assert(value.type == T_ARRAY); + if(SI_TYPE(value) == T_NULL) return SI_NullVal(); + assert(SI_TYPE(value) == T_ARRAY); uint arrayLen = SIArray_Length(value); SIValue array = SI_Array(arrayLen); if(arrayLen < 2) return array; @@ -189,13 +197,13 @@ void Register_ListFuncs() { AR_RegFunc(func_desc); types = array_new(SIType, 2); - types = array_append(types, T_ARRAY); + types = array_append(types, T_ARRAY | T_NULL); types = array_append(types, T_INT64 | T_NULL); func_desc = AR_FuncDescNew("subscript", AR_SUBSCRIPT, 2, 2, types, true); AR_RegFunc(func_desc); types = array_new(SIType, 3); - types = array_append(types, T_ARRAY); + types = array_append(types, T_ARRAY | T_NULL); types = array_append(types, T_INT64 | T_NULL); types = array_append(types, T_INT64 | T_NULL); func_desc = AR_FuncDescNew("slice", AR_SLICE, 3, 3, types, true); @@ -204,28 +212,28 @@ void Register_ListFuncs() { types = array_new(SIType, 3); types = array_append(types, T_INT64); types = array_append(types, T_INT64); - types = array_append(types, T_INT64 | T_NULL); + types = array_append(types, T_INT64); func_desc = AR_FuncDescNew("range", AR_RANGE, 2, 3, types, true); AR_RegFunc(func_desc); types = array_new(SIType, 2); types = array_append(types, SI_ALL); - types = array_append(types, T_ARRAY); + types = array_append(types, T_ARRAY | T_NULL); func_desc = AR_FuncDescNew("in", AR_IN, 2, 2, types, true); AR_RegFunc(func_desc); types = array_new(SIType, 1); - types = array_append(types, T_ARRAY); + types = array_append(types, T_ARRAY | T_NULL); func_desc = AR_FuncDescNew("size", AR_SIZE, 1, 1, types, true); AR_RegFunc(func_desc); types = array_new(SIType, 1); - types = array_append(types, T_ARRAY); + types = array_append(types, T_ARRAY | T_NULL); func_desc = AR_FuncDescNew("head", AR_HEAD, 1, 1, types, true); AR_RegFunc(func_desc); types = array_new(SIType, 1); - types = array_append(types, T_ARRAY); + types = array_append(types, T_ARRAY | T_NULL); func_desc = AR_FuncDescNew("tail", AR_TAIL, 1, 1, types, true); AR_RegFunc(func_desc); } diff --git a/src/arithmetic/path_funcs/path_funcs.c b/src/arithmetic/path_funcs/path_funcs.c index 726becf8a4..8a5c991a09 100644 --- a/src/arithmetic/path_funcs/path_funcs.c +++ b/src/arithmetic/path_funcs/path_funcs.c @@ -25,7 +25,7 @@ SIValue AR_TOPATH(SIValue *argv, int argc) { assert(argc == (nelements + 1)); for(uint i = 1; i < argc; i++) { // If any element of the path does not exist, the entire path is invalid and NULL should be returned. - if(argv[i].type == T_NULL) return SI_NullVal(); + if(SI_TYPE(argv[i]) == T_NULL) return SI_NullVal(); } SIValue path = SIPathBuilder_New(nelements); @@ -39,7 +39,7 @@ SIValue AR_TOPATH(SIValue *argv, int argc) { const cypher_astnode_t *ast_rel_pattern = cypher_ast_pattern_path_get_element(ast_path, i); bool RTL_pattern = cypher_ast_rel_pattern_get_direction(ast_rel_pattern) == CYPHER_REL_INBOUND; // Element type can be either edge, or path. - if(element.type == T_EDGE) SIPathBuilder_AppendEdge(path, element, RTL_pattern); + if(SI_TYPE(element) == T_EDGE) SIPathBuilder_AppendEdge(path, element, RTL_pattern); // If element is not an edge, it is a path. else { /* Path with 0 edges should not be appended. Their source and destination nodes are the same, @@ -60,17 +60,17 @@ SIValue AR_TOPATH(SIValue *argv, int argc) { } SIValue AR_PATH_NODES(SIValue *argv, int argc) { - if(argv[0].type == T_NULL) return SI_NullVal(); + if(SI_TYPE(argv[0]) == T_NULL) return SI_NullVal(); return SIPath_Nodes(argv[0]); } SIValue AR_PATH_RELATIONSHIPS(SIValue *argv, int argc) { - if(argv[0].type == T_NULL) return SI_NullVal(); + if(SI_TYPE(argv[0]) == T_NULL) return SI_NullVal(); return SIPath_Relationships(argv[0]); } SIValue AR_PATH_LENGTH(SIValue *argv, int argc) { - if(argv[0].type == T_NULL) return SI_NullVal(); + if(SI_TYPE(argv[0]) == T_NULL) return SI_NullVal(); return SI_LongVal(SIPath_Length(argv[0])); } diff --git a/src/execution_plan/record.c b/src/execution_plan/record.c index fbd3c78dd5..e3b62cc78a 100644 --- a/src/execution_plan/record.c +++ b/src/execution_plan/record.c @@ -105,7 +105,6 @@ Node *Record_GetNode(const Record r, int idx) { default: assert("encountered unexpected type in Record; expected Node" && false); } - return &(r->entries[idx].value.n); } Edge *Record_GetEdge(const Record r, int idx) { @@ -120,7 +119,6 @@ Edge *Record_GetEdge(const Record r, int idx) { default: assert("encountered unexpected type in Record; expected Edge" && false); } - return &(r->entries[idx].value.e); } SIValue Record_Get(Record r, int idx) { diff --git a/tests/flow/test_null_handling.py b/tests/flow/test_null_handling.py index b50d0692d8..53f37df95e 100644 --- a/tests/flow/test_null_handling.py +++ b/tests/flow/test_null_handling.py @@ -12,19 +12,45 @@ def __init__(self): self.env = Env() global redis_graph redis_con = self.env.getConnection() - redis_graph = Graph("G", redis_con) + redis_graph = Graph("null_handling", redis_con) self.populate_graph() def populate_graph(self): # Create a single node. node = Node(label="L", properties={"v": "v1"}) redis_graph.add_node(node) - redis_graph.commit() + redis_graph.flush() # Error when attempting to create a relationship with a null endpoint. def test01_create_null(self): try: - query = """MATCH (a) OPTIONAL MATCH (a)-[nonexistent_edge]->(nonexistent_node) CREATE (nonexistent_node)-[:E]->(b)""" + query = """MATCH (a) OPTIONAL MATCH (a)-[nonexistent_edge]->(nonexistent_node) CREATE (nonexistent_node)-[:E]->(a)""" + redis_graph.query(query) + assert(False) + except redis.exceptions.ResponseError: + # Expecting an error. + pass + + try: + query = """MATCH (a) OPTIONAL MATCH (a)-[nonexistent_edge]->(nonexistent_node) CREATE (a)-[:E]->(nonexistent_node)""" + redis_graph.query(query) + assert(False) + except redis.exceptions.ResponseError: + # Expecting an error. + pass + + # Error when attempting to merge a relationship with a null endpoint. + def test02_merge_null(self): + try: + query = """MATCH (a) OPTIONAL MATCH (a)-[nonexistent_edge]->(nonexistent_node) MERGE (nonexistent_node)-[:E]->(a)""" + redis_graph.query(query) + assert(False) + except redis.exceptions.ResponseError: + # Expecting an error. + pass + + try: + query = """MATCH (a) OPTIONAL MATCH (a)-[nonexistent_edge]->(nonexistent_node) MERGE (a)-[:E]->(nonexistent_node)""" redis_graph.query(query) assert(False) except redis.exceptions.ResponseError: @@ -32,7 +58,7 @@ def test01_create_null(self): pass # SET should update attributes on non-null entities and ignore null entities. - def test02_set_null(self): + def test03_set_null(self): query = """MATCH (a) OPTIONAL MATCH (a)-[nonexistent_edge]->(nonexistent_node) SET a.v2 = true, nonexistent_node.v2 = true RETURN a.v2, nonexistent_node.v2""" actual_result = redis_graph.query(query) # The property should be set on the real node and ignored on the null entity. @@ -41,23 +67,125 @@ def test02_set_null(self): self.env.assertEquals(actual_result.result_set, expected_result) # DELETE should ignore null entities. - def test03_delete_null(self): + def test04_delete_null(self): query = """MATCH (a) OPTIONAL MATCH (a)-[nonexistent_edge]->(nonexistent_node) DELETE nonexistent_node""" actual_result = redis_graph.query(query) - # The property should be set on the real node and ignored on the null entity. assert(actual_result.nodes_deleted == 0) # Functions should handle null inputs appropriately. - def test04_null_function_inputs(self): + def test05_null_function_inputs(self): query = """MATCH (a) OPTIONAL MATCH (a)-[r]->(b) RETURN type(r), labels(b), b.v * 5""" actual_result = redis_graph.query(query) expected_result = [[None, None, None]] self.env.assertEquals(actual_result.result_set, expected_result) # Path functions should handle null inputs appropriately. - def test05_null_named_path_function_inputs(self): + def test06_null_named_path_function_inputs(self): query = """MATCH (a) OPTIONAL MATCH p = (a)-[r]->() RETURN p, length(p), collect(relationships(p))""" actual_result = redis_graph.query(query) # The path and function calls on it should return NULL, while collect() returns an empty array. expected_result = [[None, None, []]] self.env.assertEquals(actual_result.result_set, expected_result) + + # List functions should handle null inputs appropriately. + def test07_null_list_function_inputs(self): + expected_result = [[None]] + + # NULL list argument to subscript. + query = """WITH NULL as list RETURN list[0]""" + actual_result = redis_graph.query(query) + self.env.assertEquals(actual_result.result_set, expected_result) + + # NULL list argument to slice. + query = """WITH NULL as list RETURN list[0..5]""" + actual_result = redis_graph.query(query) + self.env.assertEquals(actual_result.result_set, expected_result) + + # NULL list argument to HEAD. + query = """WITH NULL as list RETURN head(list)""" + actual_result = redis_graph.query(query) + self.env.assertEquals(actual_result.result_set, expected_result) + + # NULL list argument to TAIL. + query = """WITH NULL as list RETURN tail(list)""" + actual_result = redis_graph.query(query) + self.env.assertEquals(actual_result.result_set, expected_result) + + # NULL list argument to IN. + query = """WITH NULL as list RETURN 'val' in list""" + actual_result = redis_graph.query(query) + self.env.assertEquals(actual_result.result_set, expected_result) + + # NULL list argument to SIZE. + query = """WITH NULL as list RETURN size(list)""" + actual_result = redis_graph.query(query) + self.env.assertEquals(actual_result.result_set, expected_result) + + # NULL subscript argument. + query = """WITH ['a'] as list RETURN list[NULL]""" + actual_result = redis_graph.query(query) + self.env.assertEquals(actual_result.result_set, expected_result) + + # NULL IN non-empty list should return NULL. + query = """RETURN NULL in ['val']""" + actual_result = redis_graph.query(query) + expected_result = [[None]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # NULL arguments to slice. + query = """WITH ['a'] as list RETURN list[0..NULL]""" + actual_result = redis_graph.query(query) + self.env.assertEquals(actual_result.result_set, expected_result) + + # NULL range argument should produce an error. + query = """RETURN range(NULL, 5)""" + try: + redis_graph.query(query) + assert(False) + except redis.exceptions.ResponseError: + # Expecting an error. + pass + + # NULL IN empty list should return false. + query = """RETURN NULL in []""" + actual_result = redis_graph.query(query) + expected_result = [[False]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # Aggregate functions should handle null inputs appropriately. + def test08_aggregate_function_inputs(self): + # SUM should sum all non-null inputs. + query = """UNWIND [1, NULL, 3] AS a RETURN sum(a)""" + actual_result = redis_graph.query(query) + expected_result = [[4]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # SUM should return 0 given a fully NULL input. + query = """WITH NULL AS a RETURN sum(a)""" + actual_result = redis_graph.query(query) + expected_result = [[0]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # COUNT should count all non-null inputs. + query = """UNWIND [1, NULL, 3] AS a RETURN count(a)""" + actual_result = redis_graph.query(query) + expected_result = [[2]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # COUNT should return 0 given a fully NULL input. + query = """WITH NULL AS a RETURN count(a)""" + actual_result = redis_graph.query(query) + expected_result = [[0]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # COLLECT should ignore null inputs. + query = """UNWIND [1, NULL, 3] AS a RETURN collect(a)""" + actual_result = redis_graph.query(query) + expected_result = [[[1, 3]]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # COLLECT should return an empty array on all null inputs. + query = """WITH NULL AS a RETURN collect(a)""" + actual_result = redis_graph.query(query) + expected_result = [[[]]] + self.env.assertEquals(actual_result.result_set, expected_result) diff --git a/tests/flow/test_optional_match.py b/tests/flow/test_optional_match.py index 54da5bda7f..3a63ffc182 100644 --- a/tests/flow/test_optional_match.py +++ b/tests/flow/test_optional_match.py @@ -11,7 +11,7 @@ def __init__(self): self.env = Env() global redis_graph redis_con = self.env.getConnection() - redis_graph = Graph("G", redis_con) + redis_graph = Graph("optional_match", redis_con) self.populate_graph() def populate_graph(self): @@ -31,7 +31,7 @@ def populate_graph(self): edge = Edge(nodes[1], "E2", nodes[2]) redis_graph.add_edge(edge) - redis_graph.commit() + redis_graph.flush() # Optional MATCH clause that does not interact with the mandatory MATCH. def test01_disjoint_optional(self): @@ -191,3 +191,15 @@ def test14_multiple_optional_bidirectional_traversals(self): ['v3', 'v2', 'v3'], ['v4', None, None]] self.env.assertEquals(actual_result.result_set, expected_result) + + # Build a named path in an optional clause. + def test15_optional_named_path(self): + global redis_graph + query = """MATCH (a) OPTIONAL MATCH p = (a)-[]->(b) RETURN length(p) ORDER BY length(p)""" + actual_result = redis_graph.query(query) + # 2 nodes have outgoing edges and 2 do not, so expected 2 paths of length 1 and 2 null results. + expected_result = [[1], + [1], + [None], + [None]] + self.env.assertEquals(actual_result.result_set, expected_result) diff --git a/tests/tck/features/TernaryLogicAcceptance.feature b/tests/tck/features/TernaryLogicAcceptance.feature index 14e2408718..8e12dee680 100755 --- a/tests/tck/features/TernaryLogicAcceptance.feature +++ b/tests/tck/features/TernaryLogicAcceptance.feature @@ -149,7 +149,6 @@ Feature: TernaryLogicAcceptanceTest | null | false | null | | false | null | null | - @skip Scenario Outline: Using null in IN And parameters are: | name | value | From e9d78adf60a259b0b5f76d74b3cac72ab543c0ea Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Thu, 2 Apr 2020 17:50:11 -0400 Subject: [PATCH 25/32] PR fixes --- src/arithmetic/arithmetic_expression.c | 14 ++- src/arithmetic/path_funcs/path_funcs.c | 11 +- src/ast/ast_validations.c | 2 +- src/execution_plan/execution_plan_modify.c | 6 + .../ops/op_conditional_traverse.c | 10 +- src/execution_plan/ops/op_delete.c | 7 +- src/execution_plan/ops/op_expand_into.c | 9 +- src/execution_plan/ops/op_index_scan.c | 2 +- .../ops/op_node_by_label_scan.c | 2 +- src/execution_plan/ops/op_optional.c | 8 +- src/execution_plan/ops/op_optional.h | 39 ++++++- src/execution_plan/ops/op_update.c | 19 ++-- tests/flow/test_function_calls.py | 38 +++++++ tests/flow/test_list.py | 65 +++++++++++ tests/flow/test_null_handling.py | 107 +----------------- 15 files changed, 200 insertions(+), 139 deletions(-) diff --git a/src/arithmetic/arithmetic_expression.c b/src/arithmetic/arithmetic_expression.c index 3fd9e2fbd6..6b61237396 100644 --- a/src/arithmetic/arithmetic_expression.c +++ b/src/arithmetic/arithmetic_expression.c @@ -344,12 +344,14 @@ static bool _AR_EXP_UpdateEntityIdx(AR_OperandNode *node, const Record r) { static AR_EXP_Result _AR_EXP_EvaluateProperty(AR_ExpNode *node, const Record r, SIValue *result) { RecordEntryType t = Record_GetType(r, node->operand.variadic.entity_alias_idx); - if(t == REC_TYPE_UNKNOWN) { - /* If we attempt to access a null value as a graph entity (due to a - * scenario like a failed OPTIONAL MATCH), return a null value. */ - *result = SI_NullVal(); - return EVAL_OK; - } else if(!(t & (REC_TYPE_NODE | REC_TYPE_EDGE))) { + if(t != REC_TYPE_NODE && t != REC_TYPE_EDGE) { + if(t == REC_TYPE_UNKNOWN) { + /* If we attempt to access an unset Record entry as a graph entity + * (due to a scenario like a failed OPTIONAL MATCH), return a null value. */ + *result = SI_NullVal(); + return EVAL_OK; + } + /* Attempted to access a scalar value as a map. * Set an error and invoke the exception handler. */ char *error; diff --git a/src/arithmetic/path_funcs/path_funcs.c b/src/arithmetic/path_funcs/path_funcs.c index 8a5c991a09..8d5a54de11 100644 --- a/src/arithmetic/path_funcs/path_funcs.c +++ b/src/arithmetic/path_funcs/path_funcs.c @@ -23,14 +23,17 @@ SIValue AR_TOPATH(SIValue *argv, int argc) { const cypher_astnode_t *ast_path = argv[0].ptrval; uint nelements = cypher_ast_pattern_path_nelements(ast_path); assert(argc == (nelements + 1)); - for(uint i = 1; i < argc; i++) { - // If any element of the path does not exist, the entire path is invalid and NULL should be returned. - if(SI_TYPE(argv[i]) == T_NULL) return SI_NullVal(); - } SIValue path = SIPathBuilder_New(nelements); for(uint i = 0; i < nelements; i++) { SIValue element = argv[i + 1]; + if(SI_TYPE(element) == T_NULL) { + /* If any element of the path does not exist, the entire path is invalid. + * Free it and return a null value. */ + SIValue_Free(path); + return SI_NullVal(); + } + if(i % 2 == 0) { // Nodes are in even position. SIPathBuilder_AppendNode(path, element); diff --git a/src/ast/ast_validations.c b/src/ast/ast_validations.c index 80da5522f8..473e958d0a 100644 --- a/src/ast/ast_validations.c +++ b/src/ast/ast_validations.c @@ -1042,7 +1042,7 @@ static AST_Validation _ValidateClauseOrder(const AST *ast, char **reason) { if(type == CYPHER_AST_MATCH) { // Check whether this match is optional. bool current_clause_is_optional = cypher_ast_match_is_optional(clause); - // If it is not and we have already processed an optional match, emit an error. + // If the current clause is non-optional but we have already encountered an optional match, emit an error. if(!current_clause_is_optional && encountered_optional_match) { asprintf(reason, "A WITH clause is required to introduce a MATCH clause after an OPTIONAL MATCH."); return AST_INVALID; diff --git a/src/execution_plan/execution_plan_modify.c b/src/execution_plan/execution_plan_modify.c index a417237580..7e9479670d 100644 --- a/src/execution_plan/execution_plan_modify.c +++ b/src/execution_plan/execution_plan_modify.c @@ -317,6 +317,12 @@ OpBase *ExecutionPlan_BuildOpsFromPath(ExecutionPlan *plan, const char **bound_v AST *ast = QueryCtx_GetAST(); // Build a temporary AST holding a MATCH clause. cypher_astnode_type_t type = cypher_astnode_type(node); + + /* The AST node we're building a mock MATCH clause for will be a path + * if we're converting a MERGE clause or WHERE filter, and we must build + * and later free a CYPHER_AST_PATTERN node to contain it. + * If instead we're converting an OPTIONAL MATCH, the node is itself a MATCH clause, + * and we will reuse its CYPHER_AST_PATTERN node rather than building a new one. */ bool node_is_path = (type == CYPHER_AST_PATTERN_PATH || type == CYPHER_AST_NAMED_PATH); AST *match_stream_ast = AST_MockMatchClause(ast, (cypher_astnode_t *)node, node_is_path); diff --git a/src/execution_plan/ops/op_conditional_traverse.c b/src/execution_plan/ops/op_conditional_traverse.c index 9689da049f..b5e7c97c20 100644 --- a/src/execution_plan/ops/op_conditional_traverse.c +++ b/src/execution_plan/ops/op_conditional_traverse.c @@ -78,7 +78,7 @@ static void _populate_filter_matrix(CondTraverse *op) { /* Update filter matrix F, set row i at position srcId * F[i, srcId] = true. */ Node *n = Record_GetNode(r, op->srcNodeIdx); - if(!n) continue; // The expected entity may not be found on optional traversals. + assert(n && "failed to resolve source node for traversal"); NodeID srcId = ENTITY_GET_ID(n); GrB_Matrix_setElement_BOOL(op->F, true, i, srcId); } @@ -200,7 +200,15 @@ static Record CondTraverseConsume(OpBase *opBase) { // Ask child operations for data. for(op->recordsLen = 0; op->recordsLen < op->recordsCap; op->recordsLen++) { Record childRecord = OpBase_Consume(child); + // If the Record is NULL, the child has been depleted. if(!childRecord) break; + if(!Record_GetNode(childRecord, op->srcNodeIdx)) { + /* The child Record may not contain the source node in scenarios like + * a failed OPTIONAL MATCH. In this case, delete the Record and try again. */ + OpBase_DeleteRecord(childRecord); + op->recordsLen--; + continue; + } // Store received record. Record_PersistScalars(childRecord); op->records[op->recordsLen] = childRecord; diff --git a/src/execution_plan/ops/op_delete.c b/src/execution_plan/ops/op_delete.c index ef14581379..cab793b2d8 100644 --- a/src/execution_plan/ops/op_delete.c +++ b/src/execution_plan/ops/op_delete.c @@ -78,14 +78,15 @@ static Record DeleteConsume(OpBase *opBase) { for(int i = 0; i < op->exp_count; i++) { AR_ExpNode *exp = op->exps[i]; SIValue value = AR_EXP_Evaluate(exp, r); + SIType type = SI_TYPE(value); /* Enqueue entities for deletion. */ - if(SI_TYPE(value) & T_NODE) { + if(type & T_NODE) { Node *n = (Node *)value.ptrval; op->deleted_nodes = array_append(op->deleted_nodes, *n); - } else if(SI_TYPE(value) & T_EDGE) { + } else if(type & T_EDGE) { Edge *e = (Edge *)value.ptrval; op->deleted_edges = array_append(op->deleted_edges, *e); - } else if(SI_TYPE(value) & T_NULL) { + } else if(type & T_NULL) { continue; // Ignore null values. } else { /* Expression evaluated to a non-graph entity type diff --git a/src/execution_plan/ops/op_expand_into.c b/src/execution_plan/ops/op_expand_into.c index 1a24a3a429..791519be7d 100644 --- a/src/execution_plan/ops/op_expand_into.c +++ b/src/execution_plan/ops/op_expand_into.c @@ -58,7 +58,7 @@ static void _populate_filter_matrix(OpExpandInto *op) { /* Update filter matrix F, set row i at position srcId * F[i, srcId] = true. */ Node *n = Record_GetNode(r, op->srcNodeIdx); - if(!n) continue; // The expected entity may not be found on optional expansions. + assert(n && "failed to resolve source node for ExpandInto"); NodeID srcId = ENTITY_GET_ID(n); GrB_Matrix_setElement_BOOL(op->F, true, i, srcId); } @@ -222,6 +222,13 @@ static Record ExpandIntoConsume(OpBase *opBase) { Record childRecord = OpBase_Consume(child); // Did not managed to get new data, break. if(!childRecord) break; + if(!Record_GetNode(childRecord, op->srcNodeIdx)) { + /* The child Record may not contain the source node in scenarios like + * a failed OPTIONAL MATCH. In this case, delete the Record and try again. */ + OpBase_DeleteRecord(childRecord); + op->recordCount--; + continue; + } // Store received record. op->records[op->recordCount] = childRecord; diff --git a/src/execution_plan/ops/op_index_scan.c b/src/execution_plan/ops/op_index_scan.c index caeee18ace..dc6c2f5d2e 100644 --- a/src/execution_plan/ops/op_index_scan.c +++ b/src/execution_plan/ops/op_index_scan.c @@ -42,7 +42,7 @@ static OpResult IndexScanInit(OpBase *opBase) { static inline void _UpdateRecord(IndexScan *op, Record r, EntityID node_id) { // Populate the Record with the graph entity data. - Node n = {}; + Node n = {0}; assert(Graph_GetNode(op->g, node_id, &n)); // Get a pointer to the node's allocated space within the Record. Record_AddNode(r, op->nodeRecIdx, n); diff --git a/src/execution_plan/ops/op_node_by_label_scan.c b/src/execution_plan/ops/op_node_by_label_scan.c index 3bdd1a46a4..7604713cce 100644 --- a/src/execution_plan/ops/op_node_by_label_scan.c +++ b/src/execution_plan/ops/op_node_by_label_scan.c @@ -90,7 +90,7 @@ static OpResult NodeByLabelScanInit(OpBase *opBase) { static inline void _UpdateRecord(NodeByLabelScan *op, Record r, GrB_Index node_id) { // Populate the Record with the graph entity data. - Node n = {}; + Node n = {0}; Graph_GetNode(op->g, node_id, &n); // Get a pointer to the node's allocated space within the Record. Record_AddNode(r, op->nodeRecIdx, n); diff --git a/src/execution_plan/ops/op_optional.c b/src/execution_plan/ops/op_optional.c index 244bab6602..694f619877 100644 --- a/src/execution_plan/ops/op_optional.c +++ b/src/execution_plan/ops/op_optional.c @@ -10,7 +10,6 @@ static Record OptionalConsume(OpBase *opBase); static OpResult OptionalReset(OpBase *opBase); static OpBase *OptionalClone(const ExecutionPlan *plan, const OpBase *opBase); -static void OptionalFree(OpBase *opBase); OpBase *NewOptionalOp(const ExecutionPlan *plan) { Optional *op = rm_malloc(sizeof(Optional)); @@ -18,7 +17,7 @@ OpBase *NewOptionalOp(const ExecutionPlan *plan) { // Set our Op operations OpBase_Init((OpBase *)op, OPType_OPTIONAL, "Optional", NULL, OptionalConsume, - OptionalReset, NULL, OptionalClone, OptionalFree, false, plan); + OptionalReset, NULL, OptionalClone, NULL, false, plan); return (OpBase *)op; } @@ -47,8 +46,3 @@ static inline OpBase *OptionalClone(const ExecutionPlan *plan, const OpBase *opB return NewOptionalOp(plan); } -static void OptionalFree(OpBase *ctx) { - Optional *op = (Optional *)ctx; - op->emitted_record = false; -} - diff --git a/src/execution_plan/ops/op_optional.h b/src/execution_plan/ops/op_optional.h index 3de9e1a57d..25fe406ddb 100644 --- a/src/execution_plan/ops/op_optional.h +++ b/src/execution_plan/ops/op_optional.h @@ -9,8 +9,43 @@ #include "op.h" #include "../execution_plan.h" -/* If Optional's child produces records, Optional emits them without modification. - * If the child produces no records, Optional emits an empty Record exactly once. */ +/* Optional is an operation that manages the output of its child op tree rather + * than producing or mutating data in itself. + * + * It attempts to pull a Record from its child op tree, which is typically + * comprised of scan and traversal operations that model a MATCH pattern in a query. + * + * If the child successfully produces data, all of its Records are passed to Optional's + * parent without modification. + * If the child fails to produce any data, Optional will instead emit a single empty Record. + * + * This allows for the expression of complex patterns in which not all elements need to be resolved, + * rather than the all-or-nothing matching logic traditionally used to resolve MATCH patterns. + * + * For example, the query: + * MATCH (a:A) OPTIONAL MATCH (a)-[e]->(b) RETURN a, e, b + * Produces the operation tree: + * Results + * | + * Project + * | + * Apply + * / \ + * LabelScan (a) Optional + * \ + * CondTraverse (a)-[e]->(b) + * \ + * Argument (a) + * + * We want to retrieve all 'a' nodes regardless of whether they have an outgoing edge. + * For each 'a' that does have at least one outgoing edge, all matching paths + * will be resolved by Optional's CondTraverse child and returned as they would + * be for a standard MATCH pattern. + * For each 'a' that lacks an outgoing edge, the CondTraverse fails to produce any data, + * and Optional instead emits one empty Record. + * Optional's parent Apply op merges the empty Record with the left-hand Record + * produced by the Label Scan, causing 'a' to be emitted unmodified with NULL entries + * representing the elements of the failed traversal. */ typedef struct { OpBase op; bool emitted_record; // True if this operation has returned at least one Record. diff --git a/src/execution_plan/ops/op_update.c b/src/execution_plan/ops/op_update.c index d2ea5c9892..4c4c1fd3d9 100644 --- a/src/execution_plan/ops/op_update.c +++ b/src/execution_plan/ops/op_update.c @@ -53,7 +53,7 @@ static void _UpdateIndex(EntityUpdateCtx *ctx, GraphContext *gc, Schema *s, SIVa Schema_AddNodeToIndices(s, n, true); } -static void _UpdateNode(OpUpdate *op, EntityUpdateCtx *ctx) { +static bool _UpdateNode(OpUpdate *op, EntityUpdateCtx *ctx) { /* Retrieve GraphEntity: * Due to Record freeing we can't maintain the original pointer to GraphEntity object, * but only a pointer to an Entity object, @@ -71,7 +71,8 @@ static void _UpdateNode(OpUpdate *op, EntityUpdateCtx *ctx) { SIValue *old_value = GraphEntity_GetProperty((GraphEntity *)node, ctx->attr_id); if(old_value == PROPERTY_NOTFOUND) { - // Add new property. + // Adding a new property; do nothing if its value is NULL. + if(SI_TYPE(ctx->new_value) == T_NULL) return false; GraphEntity_AddProperty((GraphEntity *)node, ctx->attr_id, ctx->new_value); } else { // Update property. @@ -80,9 +81,10 @@ static void _UpdateNode(OpUpdate *op, EntityUpdateCtx *ctx) { // Update index for node entities. _UpdateIndex(ctx, op->gc, s, old_value, &ctx->new_value); + return true; } -static void _UpdateEdge(OpUpdate *op, EntityUpdateCtx *ctx) { +static bool _UpdateEdge(OpUpdate *op, EntityUpdateCtx *ctx) { /* Retrieve GraphEntity: * Due to Record freeing we can't maintain the original pointer to GraphEntity object, * but only a pointer to an Entity object, @@ -96,16 +98,19 @@ static void _UpdateEdge(OpUpdate *op, EntityUpdateCtx *ctx) { SIValue *old_value = GraphEntity_GetProperty((GraphEntity *)edge, ctx->attr_id); if(old_value == PROPERTY_NOTFOUND) { - // Add new property. + // Adding a new property; do nothing if its value is NULL. + if(SI_TYPE(ctx->new_value) == T_NULL) return false; GraphEntity_AddProperty((GraphEntity *)edge, ctx->attr_id, ctx->new_value); } else { // Update property. GraphEntity_SetProperty((GraphEntity *)edge, ctx->attr_id, ctx->new_value); } + return true; } /* Executes delayed updates. */ static void _CommitUpdates(OpUpdate *op) { + uint properties_set = 0; for(uint i = 0; i < op->pending_updates_count; i++) { EntityUpdateCtx *ctx = &op->pending_updates[i]; // Map the attribute key if it has not been encountered before @@ -113,14 +118,14 @@ static void _CommitUpdates(OpUpdate *op) { ctx->attr_id = GraphContext_FindOrAddAttribute(op->gc, ctx->attribute); } if(ctx->entity_type == GETYPE_NODE) { - _UpdateNode(op, ctx); + properties_set += _UpdateNode(op, ctx); } else { - _UpdateEdge(op, ctx); + properties_set += _UpdateEdge(op, ctx); } SIValue_Free(ctx->new_value); } - if(op->stats) op->stats->properties_set += op->pending_updates_count; + if(op->stats) op->stats->properties_set += properties_set; } /* We only cache records if op_update is not the last diff --git a/tests/flow/test_function_calls.py b/tests/flow/test_function_calls.py index f7b480385e..916eebed51 100644 --- a/tests/flow/test_function_calls.py +++ b/tests/flow/test_function_calls.py @@ -204,3 +204,41 @@ def test10_modulo_inputs(self): actual_result = graph.query(query) expected_result = [[-0.5]] self.env.assertEquals(actual_result.result_set, expected_result) + + # Aggregate functions should handle null inputs appropriately. + def test11_null_aggregate_function_inputs(self): + # SUM should sum all non-null inputs. + query = """UNWIND [1, NULL, 3] AS a RETURN sum(a)""" + actual_result = graph.query(query) + expected_result = [[4]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # SUM should return 0 given a fully NULL input. + query = """WITH NULL AS a RETURN sum(a)""" + actual_result = graph.query(query) + expected_result = [[0]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # COUNT should count all non-null inputs. + query = """UNWIND [1, NULL, 3] AS a RETURN count(a)""" + actual_result = graph.query(query) + expected_result = [[2]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # COUNT should return 0 given a fully NULL input. + query = """WITH NULL AS a RETURN count(a)""" + actual_result = graph.query(query) + expected_result = [[0]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # COLLECT should ignore null inputs. + query = """UNWIND [1, NULL, 3] AS a RETURN collect(a)""" + actual_result = graph.query(query) + expected_result = [[[1, 3]]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # COLLECT should return an empty array on all null inputs. + query = """WITH NULL AS a RETURN collect(a)""" + actual_result = graph.query(query) + expected_result = [[[]]] + self.env.assertEquals(actual_result.result_set, expected_result) diff --git a/tests/flow/test_list.py b/tests/flow/test_list.py index 007256a93c..249baf57f9 100644 --- a/tests/flow/test_list.py +++ b/tests/flow/test_list.py @@ -33,3 +33,68 @@ def test02_unwind(self): expected_result = [[0], [1], [2], [3], [4], [5], [6], [7], [8], [9], [10]] self.env.assertEquals(result_set, expected_result) + + # List functions should handle null inputs appropriately. + def test03_null_list_function_inputs(self): + expected_result = [[None]] + + # NULL list argument to subscript. + query = """WITH NULL as list RETURN list[0]""" + actual_result = redis_graph.query(query) + self.env.assertEquals(actual_result.result_set, expected_result) + + # NULL list argument to slice. + query = """WITH NULL as list RETURN list[0..5]""" + actual_result = redis_graph.query(query) + self.env.assertEquals(actual_result.result_set, expected_result) + + # NULL list argument to HEAD. + query = """WITH NULL as list RETURN head(list)""" + actual_result = redis_graph.query(query) + self.env.assertEquals(actual_result.result_set, expected_result) + + # NULL list argument to TAIL. + query = """WITH NULL as list RETURN tail(list)""" + actual_result = redis_graph.query(query) + self.env.assertEquals(actual_result.result_set, expected_result) + + # NULL list argument to IN. + query = """WITH NULL as list RETURN 'val' in list""" + actual_result = redis_graph.query(query) + self.env.assertEquals(actual_result.result_set, expected_result) + + # NULL list argument to SIZE. + query = """WITH NULL as list RETURN size(list)""" + actual_result = redis_graph.query(query) + self.env.assertEquals(actual_result.result_set, expected_result) + + # NULL subscript argument. + query = """WITH ['a'] as list RETURN list[NULL]""" + actual_result = redis_graph.query(query) + self.env.assertEquals(actual_result.result_set, expected_result) + + # NULL IN non-empty list should return NULL. + query = """RETURN NULL in ['val']""" + actual_result = redis_graph.query(query) + expected_result = [[None]] + self.env.assertEquals(actual_result.result_set, expected_result) + + # NULL arguments to slice. + query = """WITH ['a'] as list RETURN list[0..NULL]""" + actual_result = redis_graph.query(query) + self.env.assertEquals(actual_result.result_set, expected_result) + + # NULL range argument should produce an error. + query = """RETURN range(NULL, 5)""" + try: + redis_graph.query(query) + assert(False) + except redis.exceptions.ResponseError: + # Expecting an error. + pass + + # NULL IN empty list should return false. + query = """RETURN NULL in []""" + actual_result = redis_graph.query(query) + expected_result = [[False]] + self.env.assertEquals(actual_result.result_set, expected_result) diff --git a/tests/flow/test_null_handling.py b/tests/flow/test_null_handling.py index 53f37df95e..2fb0f0cbca 100644 --- a/tests/flow/test_null_handling.py +++ b/tests/flow/test_null_handling.py @@ -59,11 +59,11 @@ def test02_merge_null(self): # SET should update attributes on non-null entities and ignore null entities. def test03_set_null(self): - query = """MATCH (a) OPTIONAL MATCH (a)-[nonexistent_edge]->(nonexistent_node) SET a.v2 = true, nonexistent_node.v2 = true RETURN a.v2, nonexistent_node.v2""" + query = """MATCH (a) OPTIONAL MATCH (a)-[nonexistent_edge]->(nonexistent_node) SET a.v2 = true, nonexistent_node.v2 = true, a.v3 = nonexistent_node.v3 RETURN a.v2, nonexistent_node.v2, a.v3""" actual_result = redis_graph.query(query) # The property should be set on the real node and ignored on the null entity. assert(actual_result.properties_set == 1) - expected_result = [[True, None]] + expected_result = [[True, None, None]] self.env.assertEquals(actual_result.result_set, expected_result) # DELETE should ignore null entities. @@ -86,106 +86,3 @@ def test06_null_named_path_function_inputs(self): # The path and function calls on it should return NULL, while collect() returns an empty array. expected_result = [[None, None, []]] self.env.assertEquals(actual_result.result_set, expected_result) - - # List functions should handle null inputs appropriately. - def test07_null_list_function_inputs(self): - expected_result = [[None]] - - # NULL list argument to subscript. - query = """WITH NULL as list RETURN list[0]""" - actual_result = redis_graph.query(query) - self.env.assertEquals(actual_result.result_set, expected_result) - - # NULL list argument to slice. - query = """WITH NULL as list RETURN list[0..5]""" - actual_result = redis_graph.query(query) - self.env.assertEquals(actual_result.result_set, expected_result) - - # NULL list argument to HEAD. - query = """WITH NULL as list RETURN head(list)""" - actual_result = redis_graph.query(query) - self.env.assertEquals(actual_result.result_set, expected_result) - - # NULL list argument to TAIL. - query = """WITH NULL as list RETURN tail(list)""" - actual_result = redis_graph.query(query) - self.env.assertEquals(actual_result.result_set, expected_result) - - # NULL list argument to IN. - query = """WITH NULL as list RETURN 'val' in list""" - actual_result = redis_graph.query(query) - self.env.assertEquals(actual_result.result_set, expected_result) - - # NULL list argument to SIZE. - query = """WITH NULL as list RETURN size(list)""" - actual_result = redis_graph.query(query) - self.env.assertEquals(actual_result.result_set, expected_result) - - # NULL subscript argument. - query = """WITH ['a'] as list RETURN list[NULL]""" - actual_result = redis_graph.query(query) - self.env.assertEquals(actual_result.result_set, expected_result) - - # NULL IN non-empty list should return NULL. - query = """RETURN NULL in ['val']""" - actual_result = redis_graph.query(query) - expected_result = [[None]] - self.env.assertEquals(actual_result.result_set, expected_result) - - # NULL arguments to slice. - query = """WITH ['a'] as list RETURN list[0..NULL]""" - actual_result = redis_graph.query(query) - self.env.assertEquals(actual_result.result_set, expected_result) - - # NULL range argument should produce an error. - query = """RETURN range(NULL, 5)""" - try: - redis_graph.query(query) - assert(False) - except redis.exceptions.ResponseError: - # Expecting an error. - pass - - # NULL IN empty list should return false. - query = """RETURN NULL in []""" - actual_result = redis_graph.query(query) - expected_result = [[False]] - self.env.assertEquals(actual_result.result_set, expected_result) - - # Aggregate functions should handle null inputs appropriately. - def test08_aggregate_function_inputs(self): - # SUM should sum all non-null inputs. - query = """UNWIND [1, NULL, 3] AS a RETURN sum(a)""" - actual_result = redis_graph.query(query) - expected_result = [[4]] - self.env.assertEquals(actual_result.result_set, expected_result) - - # SUM should return 0 given a fully NULL input. - query = """WITH NULL AS a RETURN sum(a)""" - actual_result = redis_graph.query(query) - expected_result = [[0]] - self.env.assertEquals(actual_result.result_set, expected_result) - - # COUNT should count all non-null inputs. - query = """UNWIND [1, NULL, 3] AS a RETURN count(a)""" - actual_result = redis_graph.query(query) - expected_result = [[2]] - self.env.assertEquals(actual_result.result_set, expected_result) - - # COUNT should return 0 given a fully NULL input. - query = """WITH NULL AS a RETURN count(a)""" - actual_result = redis_graph.query(query) - expected_result = [[0]] - self.env.assertEquals(actual_result.result_set, expected_result) - - # COLLECT should ignore null inputs. - query = """UNWIND [1, NULL, 3] AS a RETURN collect(a)""" - actual_result = redis_graph.query(query) - expected_result = [[[1, 3]]] - self.env.assertEquals(actual_result.result_set, expected_result) - - # COLLECT should return an empty array on all null inputs. - query = """WITH NULL AS a RETURN collect(a)""" - actual_result = redis_graph.query(query) - expected_result = [[[]]] - self.env.assertEquals(actual_result.result_set, expected_result) From b7a9884b68dfe5135623f95d18bf812045d2338f Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Thu, 2 Apr 2020 17:51:04 -0400 Subject: [PATCH 26/32] Remove Record_GetScalar interface --- src/arithmetic/arithmetic_expression.c | 2 +- src/execution_plan/ops/op_cond_var_len_traverse.c | 2 +- src/execution_plan/ops/op_value_hash_join.c | 10 +++++----- src/execution_plan/record.c | 11 +++-------- src/execution_plan/record.h | 3 --- src/resultset/formatters/resultset_replycompact.c | 3 ++- src/resultset/formatters/resultset_replyverbose.c | 3 ++- tests/flow/test_list.py | 1 + 8 files changed, 15 insertions(+), 20 deletions(-) diff --git a/src/arithmetic/arithmetic_expression.c b/src/arithmetic/arithmetic_expression.c index 6b61237396..5aa554c15e 100644 --- a/src/arithmetic/arithmetic_expression.c +++ b/src/arithmetic/arithmetic_expression.c @@ -355,7 +355,7 @@ static AR_EXP_Result _AR_EXP_EvaluateProperty(AR_ExpNode *node, const Record r, /* Attempted to access a scalar value as a map. * Set an error and invoke the exception handler. */ char *error; - SIValue v = Record_GetScalar(r, node->operand.variadic.entity_alias_idx); + SIValue v = Record_Get(r, node->operand.variadic.entity_alias_idx); asprintf(&error, "Type mismatch: expected a map but was %s", SIType_ToString(SI_TYPE(v))); QueryCtx_SetError(error); // Set the query-level error. return EVAL_ERR; diff --git a/src/execution_plan/ops/op_cond_var_len_traverse.c b/src/execution_plan/ops/op_cond_var_len_traverse.c index 693c1161ef..fa57cc88be 100644 --- a/src/execution_plan/ops/op_cond_var_len_traverse.c +++ b/src/execution_plan/ops/op_cond_var_len_traverse.c @@ -144,7 +144,7 @@ static Record CondVarLenTraverseConsume(OpBase *opBase) { if(op->edgesIdx >= 0) { // If we're returning a new path from a previously-used Record, // free the previous path to avoid a memory leak. - if(reused_record) SIValue_Free(Record_GetScalar(op->r, op->edgesIdx)); + if(reused_record) SIValue_Free(Record_Get(op->r, op->edgesIdx)); // Add new path to Record. Record_AddScalar(op->r, op->edgesIdx, SI_Path(p)); } diff --git a/src/execution_plan/ops/op_value_hash_join.c b/src/execution_plan/ops/op_value_hash_join.c index a4b02d66a1..8edb538b8b 100644 --- a/src/execution_plan/ops/op_value_hash_join.c +++ b/src/execution_plan/ops/op_value_hash_join.c @@ -21,8 +21,8 @@ static void ValueHashJoinFree(OpBase *opBase); /* Determins order between two records by inspecting * element stored at postion idx. */ static bool _record_islt(Record l, Record r, uint idx) { - SIValue lv = Record_GetScalar(l, idx); - SIValue rv = Record_GetScalar(r, idx); + SIValue lv = Record_Get(l, idx); + SIValue rv = Record_Get(r, idx); return SIValue_Compare(lv, rv, NULL) < 0; } @@ -39,7 +39,7 @@ static int64_t _binarySearchLeftmost(Record *array, int join_key_idx, SIValue v) int64_t right = recordCount; while(left < right) { pos = (right + left) / 2; - SIValue x = Record_GetScalar(array[pos], join_key_idx); + SIValue x = Record_Get(array[pos], join_key_idx); if(SIValue_Compare(x, v, NULL) < 0) left = pos + 1; else right = pos; } @@ -54,7 +54,7 @@ static int64_t _binarySearchRightmost(Record *array, int64_t array_len, int join int64_t right = array_len; while(left < right) { pos = (right + left) / 2; - SIValue x = Record_GetScalar(array[pos], join_key_idx); + SIValue x = Record_Get(array[pos], join_key_idx); if(SIValue_Compare(v, x, NULL) < 0) right = pos; else left = pos + 1; } @@ -89,7 +89,7 @@ static bool _set_intersection_idx(OpValueHashJoin *op, SIValue v) { // Make sure value was found. Record r = op->cached_records[leftmost_idx]; - SIValue x = Record_GetScalar(r, op->join_value_rec_idx); + SIValue x = Record_Get(r, op->join_value_rec_idx); if(SIValue_Compare(x, v, NULL) != 0) return false; // Value wasn't found. /* Value was found diff --git a/src/execution_plan/record.c b/src/execution_plan/record.c index e3b62cc78a..b3e909c4d7 100644 --- a/src/execution_plan/record.c +++ b/src/execution_plan/record.c @@ -88,11 +88,6 @@ RecordEntryType Record_GetType(const Record r, int idx) { return r->entries[idx].type; } -SIValue Record_GetScalar(Record r, int idx) { - assert(r->entries[idx].type == REC_TYPE_SCALAR); - return r->entries[idx].value.s; -} - Node *Record_GetNode(const Record r, int idx) { switch(r->entries[idx].type) { case REC_TYPE_NODE: @@ -129,7 +124,7 @@ SIValue Record_Get(Record r, int idx) { case REC_TYPE_EDGE: return SI_Edge(Record_GetEdge(r, idx)); case REC_TYPE_SCALAR: - return Record_GetScalar(r, idx); + return r->entries[idx].value.s; case REC_TYPE_UNKNOWN: return SI_NullVal(); default: @@ -145,7 +140,7 @@ GraphEntity *Record_GetGraphEntity(const Record r, int idx) { case REC_TYPE_EDGE: return (GraphEntity *)Record_GetEdge(r, idx); case REC_TYPE_SCALAR: - return (GraphEntity *)(Record_GetScalar(r, idx).ptrval); + return (GraphEntity *)(Record_Get(r, idx).ptrval); default: assert(false); } @@ -241,7 +236,7 @@ unsigned long long Record_Hash64(const Record r) { len = sizeof(id); break; case REC_TYPE_SCALAR: - si = Record_GetScalar(r, i); + si = Record_Get(r, i); switch(si.type) { case T_NULL: data = &_null; diff --git a/src/execution_plan/record.h b/src/execution_plan/record.h index f2305f2894..e6982ee3dc 100644 --- a/src/execution_plan/record.h +++ b/src/execution_plan/record.h @@ -61,9 +61,6 @@ int Record_GetEntryIdx(Record r, const char *alias); // Get entry type. RecordEntryType Record_GetType(const Record r, int idx); -// Get a scalar from record at position idx. -SIValue Record_GetScalar(const Record r, int idx); - // Get a node from record at position idx. Node *Record_GetNode(const Record r, int idx); diff --git a/src/resultset/formatters/resultset_replycompact.c b/src/resultset/formatters/resultset_replycompact.c index 5bba433d43..8e89c10880 100644 --- a/src/resultset/formatters/resultset_replycompact.c +++ b/src/resultset/formatters/resultset_replycompact.c @@ -238,7 +238,7 @@ void ResultSet_EmitCompactRecord(RedisModuleCtx *ctx, GraphContext *gc, const Re break; default: RedisModule_ReplyWithArray(ctx, 2); // Reply with array with space for type and value - _ResultSet_CompactReplyWithSIValue(ctx, gc, Record_GetScalar(r, idx)); + _ResultSet_CompactReplyWithSIValue(ctx, gc, Record_Get(r, idx)); } } } @@ -282,3 +282,4 @@ void ResultSet_ReplyWithCompactHeader(RedisModuleCtx *ctx, const char **columns, RedisModule_ReplyWithStringBuffer(ctx, columns[i], strlen(columns[i])); } } + diff --git a/src/resultset/formatters/resultset_replyverbose.c b/src/resultset/formatters/resultset_replyverbose.c index f030debf27..1ccf811247 100644 --- a/src/resultset/formatters/resultset_replyverbose.c +++ b/src/resultset/formatters/resultset_replyverbose.c @@ -180,7 +180,7 @@ void ResultSet_EmitVerboseRecord(RedisModuleCtx *ctx, GraphContext *gc, const Re _ResultSet_VerboseReplyWithEdge(ctx, gc, Record_GetEdge(r, idx)); break; default: - _ResultSet_VerboseReplyWithSIValue(ctx, gc, Record_GetScalar(r, idx)); + _ResultSet_VerboseReplyWithSIValue(ctx, gc, Record_Get(r, idx)); } } } @@ -195,3 +195,4 @@ void ResultSet_ReplyWithVerboseHeader(RedisModuleCtx *ctx, const char **columns, RedisModule_ReplyWithStringBuffer(ctx, columns[i], strlen(columns[i])); } } + diff --git a/tests/flow/test_list.py b/tests/flow/test_list.py index 249baf57f9..1e9c0759a3 100644 --- a/tests/flow/test_list.py +++ b/tests/flow/test_list.py @@ -1,4 +1,5 @@ from RLTest import Env +import redis from redisgraph import Graph, Node, Edge from base import FlowTestsBase From c2c3145bb4cdefa4b77b63b66dec91f3d6b66f27 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Thu, 2 Apr 2020 18:00:53 -0400 Subject: [PATCH 27/32] PR fixes --- src/ast/ast_mock.c | 2 +- src/execution_plan/ops/op_conditional_traverse.c | 2 +- src/execution_plan/ops/op_create.c | 2 +- src/execution_plan/ops/op_merge_create.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ast/ast_mock.c b/src/ast/ast_mock.c index 4ce193cd8c..b0ca80ea65 100644 --- a/src/ast/ast_mock.c +++ b/src/ast/ast_mock.c @@ -19,7 +19,7 @@ AST *AST_MockMatchClause(AST *master_ast, cypher_astnode_t *node, bool node_is_p ast->skip = NULL; ast->limit = NULL; cypher_astnode_t *pattern; - struct cypher_input_range range = {}; + struct cypher_input_range range = {0}; const cypher_astnode_t *predicate = NULL; uint child_count = (node_is_path) ? 1 : cypher_astnode_nchildren(node); cypher_astnode_t *children[child_count]; diff --git a/src/execution_plan/ops/op_conditional_traverse.c b/src/execution_plan/ops/op_conditional_traverse.c index b5e7c97c20..54d5350bb1 100644 --- a/src/execution_plan/ops/op_conditional_traverse.c +++ b/src/execution_plan/ops/op_conditional_traverse.c @@ -222,7 +222,7 @@ static Record CondTraverseConsume(OpBase *opBase) { /* Get node from current column. */ op->r = op->records[src_id]; - Node destNode = {}; + Node destNode = {0}; Graph_GetNode(op->graph, dest_id, &destNode); Record_AddNode(op->r, op->destNodeIdx, destNode); diff --git a/src/execution_plan/ops/op_create.c b/src/execution_plan/ops/op_create.c index 1c6b15fc5a..65cabb33fb 100644 --- a/src/execution_plan/ops/op_create.c +++ b/src/execution_plan/ops/op_create.c @@ -89,7 +89,7 @@ static void _CreateEdges(OpCreate *op, Record r) { } /* Create the actual edge. */ - Edge newEdge = {}; + Edge newEdge = {0}; if(array_len(e->reltypes) > 0) newEdge.relationship = e->reltypes[0]; Edge_SetSrcNode(&newEdge, src_node); Edge_SetDestNode(&newEdge, dest_node); diff --git a/src/execution_plan/ops/op_merge_create.c b/src/execution_plan/ops/op_merge_create.c index d27b05657c..5f7b38fcd8 100644 --- a/src/execution_plan/ops/op_merge_create.c +++ b/src/execution_plan/ops/op_merge_create.c @@ -134,7 +134,7 @@ static bool _CreateEntities(OpMergeCreate *op, Record r) { } /* Create the actual edge. */ - Edge newEdge = {}; + Edge newEdge = {0}; if(array_len(e->reltypes) > 0) newEdge.relationship = e->reltypes[0]; Edge_SetSrcNode(&newEdge, src_node); Edge_SetDestNode(&newEdge, dest_node); From fc0ed1a9274b44f5ffb33fb7e77d3db47943789b Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Fri, 3 Apr 2020 08:49:28 -0400 Subject: [PATCH 28/32] PR fixes --- src/execution_plan/ops/op_apply.c | 4 +-- .../ops/op_conditional_traverse.c | 1 - src/execution_plan/ops/op_expand_into.c | 12 ++----- src/execution_plan/ops/op_update.c | 31 ++++++++++++++----- src/execution_plan/record.c | 4 +-- 5 files changed, 29 insertions(+), 23 deletions(-) diff --git a/src/execution_plan/ops/op_apply.c b/src/execution_plan/ops/op_apply.c index 15a8fe6cd8..17c581ab62 100644 --- a/src/execution_plan/ops/op_apply.c +++ b/src/execution_plan/ops/op_apply.c @@ -37,7 +37,7 @@ static OpResult ApplyInit(OpBase *opBase) { // Locate branch's Argument op tap. op->op_arg = (Argument *)ExecutionPlan_LocateOp(op->rhs_branch, OPType_ARGUMENT); - assert(op->op_arg && op->op_arg->op.childCount == 0); + assert(op->op_arg); return OP_OK; } @@ -52,7 +52,7 @@ static Record ApplyConsume(OpBase *opBase) { if(!op->r) return NULL; // Bound branch and this op are depleted. // Successfully pulled a new Record, propagate to the top of the RHS branch. - if(op->op_arg) Argument_AddRecord(op->op_arg, OpBase_CloneRecord(op->r)); + Argument_AddRecord(op->op_arg, OpBase_CloneRecord(op->r)); } // Pull a Record from the RHS branch. diff --git a/src/execution_plan/ops/op_conditional_traverse.c b/src/execution_plan/ops/op_conditional_traverse.c index 54d5350bb1..8076692f05 100644 --- a/src/execution_plan/ops/op_conditional_traverse.c +++ b/src/execution_plan/ops/op_conditional_traverse.c @@ -78,7 +78,6 @@ static void _populate_filter_matrix(CondTraverse *op) { /* Update filter matrix F, set row i at position srcId * F[i, srcId] = true. */ Node *n = Record_GetNode(r, op->srcNodeIdx); - assert(n && "failed to resolve source node for traversal"); NodeID srcId = ENTITY_GET_ID(n); GrB_Matrix_setElement_BOOL(op->F, true, i, srcId); } diff --git a/src/execution_plan/ops/op_expand_into.c b/src/execution_plan/ops/op_expand_into.c index 791519be7d..da26f4f2f7 100644 --- a/src/execution_plan/ops/op_expand_into.c +++ b/src/execution_plan/ops/op_expand_into.c @@ -58,7 +58,6 @@ static void _populate_filter_matrix(OpExpandInto *op) { /* Update filter matrix F, set row i at position srcId * F[i, srcId] = true. */ Node *n = Record_GetNode(r, op->srcNodeIdx); - assert(n && "failed to resolve source node for ExpandInto"); NodeID srcId = ENTITY_GET_ID(n); GrB_Matrix_setElement_BOOL(op->F, true, i, srcId); } @@ -159,14 +158,6 @@ static Record _handoff(OpExpandInto *op) { op->r = op->records[op->recordCount]; srcNode = Record_GetNode(op->r, op->srcNodeIdx); destNode = Record_GetNode(op->r, op->destNodeIdx); - if(!srcNode || !destNode) { - // An endpoint may not resolve if an Optional op failed to match, - // in which case we can skip this record. - // Mark as NULL to avoid double free. - op->records[op->recordCount] = NULL; - continue; - - } srcId = ENTITY_GET_ID(srcNode); destId = ENTITY_GET_ID(destNode); bool x; @@ -222,7 +213,8 @@ static Record ExpandIntoConsume(OpBase *opBase) { Record childRecord = OpBase_Consume(child); // Did not managed to get new data, break. if(!childRecord) break; - if(!Record_GetNode(childRecord, op->srcNodeIdx)) { + if(!Record_GetNode(childRecord, op->srcNodeIdx) || + !Record_GetNode(childRecord, op->destNodeIdx)) { /* The child Record may not contain the source node in scenarios like * a failed OPTIONAL MATCH. In this case, delete the Record and try again. */ OpBase_DeleteRecord(childRecord); diff --git a/src/execution_plan/ops/op_update.c b/src/execution_plan/ops/op_update.c index 4c4c1fd3d9..fcd0aae226 100644 --- a/src/execution_plan/ops/op_update.c +++ b/src/execution_plan/ops/op_update.c @@ -53,7 +53,13 @@ static void _UpdateIndex(EntityUpdateCtx *ctx, GraphContext *gc, Schema *s, SIVa Schema_AddNodeToIndices(s, n, true); } -static bool _UpdateNode(OpUpdate *op, EntityUpdateCtx *ctx) { +/* Set a property on a node. For non-NULL values, the property + * will be added or updated if it is already present. + * For NULL values, the property will be deleted if present + * and nothing will be done otherwise. + * Relevant indexes will be updated accordingly. + * Returns 1 if a property was set or deleted. */ +static int _UpdateNode(OpUpdate *op, EntityUpdateCtx *ctx) { /* Retrieve GraphEntity: * Due to Record freeing we can't maintain the original pointer to GraphEntity object, * but only a pointer to an Entity object, @@ -72,7 +78,7 @@ static bool _UpdateNode(OpUpdate *op, EntityUpdateCtx *ctx) { if(old_value == PROPERTY_NOTFOUND) { // Adding a new property; do nothing if its value is NULL. - if(SI_TYPE(ctx->new_value) == T_NULL) return false; + if(SI_TYPE(ctx->new_value) == T_NULL) return 0; GraphEntity_AddProperty((GraphEntity *)node, ctx->attr_id, ctx->new_value); } else { // Update property. @@ -81,10 +87,15 @@ static bool _UpdateNode(OpUpdate *op, EntityUpdateCtx *ctx) { // Update index for node entities. _UpdateIndex(ctx, op->gc, s, old_value, &ctx->new_value); - return true; + return 1; } -static bool _UpdateEdge(OpUpdate *op, EntityUpdateCtx *ctx) { +/* Set a property on an edge. For non-NULL values, the property + * will be added or updated if it is already present. + * For NULL values, the property will be deleted if present + * and nothing will be done otherwise. + * Returns 1 if a property was set or deleted. */ +static int _UpdateEdge(OpUpdate *op, EntityUpdateCtx *ctx) { /* Retrieve GraphEntity: * Due to Record freeing we can't maintain the original pointer to GraphEntity object, * but only a pointer to an Entity object, @@ -99,13 +110,13 @@ static bool _UpdateEdge(OpUpdate *op, EntityUpdateCtx *ctx) { if(old_value == PROPERTY_NOTFOUND) { // Adding a new property; do nothing if its value is NULL. - if(SI_TYPE(ctx->new_value) == T_NULL) return false; + if(SI_TYPE(ctx->new_value) == T_NULL) return 0; GraphEntity_AddProperty((GraphEntity *)edge, ctx->attr_id, ctx->new_value); } else { // Update property. GraphEntity_SetProperty((GraphEntity *)edge, ctx->attr_id, ctx->new_value); } - return true; + return 1; } /* Executes delayed updates. */ @@ -191,7 +202,13 @@ static Record UpdateConsume(OpBase *opBase) { // If the expected entity was not found, make no updates but do not error. if(t == REC_TYPE_UNKNOWN) continue; // Make sure we're updating either a node or an edge. - assert(t == REC_TYPE_NODE || t == REC_TYPE_EDGE); + if(t != REC_TYPE_NODE && t != REC_TYPE_EDGE) { + char *error; + asprintf(&error, "Update error: alias '%s' did not resolve to a graph entity", + update_expression->alias); + QueryCtx_SetError(error); + QueryCtx_RaiseRuntimeException(); + } GraphEntityType type = (t == REC_TYPE_NODE) ? GETYPE_NODE : GETYPE_EDGE; GraphEntity *entity = Record_GetGraphEntity(r, update_expression->record_idx); diff --git a/src/execution_plan/record.c b/src/execution_plan/record.c index b3e909c4d7..3231b562c7 100644 --- a/src/execution_plan/record.c +++ b/src/execution_plan/record.c @@ -139,10 +139,8 @@ GraphEntity *Record_GetGraphEntity(const Record r, int idx) { return (GraphEntity *)Record_GetNode(r, idx); case REC_TYPE_EDGE: return (GraphEntity *)Record_GetEdge(r, idx); - case REC_TYPE_SCALAR: - return (GraphEntity *)(Record_Get(r, idx).ptrval); default: - assert(false); + assert(false && "encountered unexpected type when trying to retrieve graph entity"); } return NULL; } From 2f989776aaa4ae43e48c1abcbd1d457e94d5cf7b Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Fri, 3 Apr 2020 10:38:46 -0400 Subject: [PATCH 29/32] Add demo query for OPTIONAL MATCH --- demo/imdb/imdb_queries.py | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/demo/imdb/imdb_queries.py b/demo/imdb/imdb_queries.py index 002f10050b..c9e7370a9f 100644 --- a/demo/imdb/imdb_queries.py +++ b/demo/imdb/imdb_queries.py @@ -367,6 +367,34 @@ def __init__(self, actors=None, movies=None): ['Tim Blake Nelson']] ) + ################################################################## + ### grand_budapest_hotel_cast_and_their_other_roles + ################################################################## + + self.grand_budapest_hotel_cast_and_their_other_roles = QueryInfo( + + query="""MATCH (a:actor)-[:act]->(h:movie {title: 'The Grand Budapest Hotel'}) + OPTIONAL MATCH (a)-[:act]->(m:movie) WHERE m <> h + RETURN a.name, m.title + ORDER BY a.name, m.title""", + + description='All actors in The Grand Budapest Hotel and their other movies', + reversible=False, + max_run_time_ms=4, + expected_result=[['Adrien Brody', None], + ['Bill Murray', 'The Jungle Book'], + ['F. Murray Abraham', None], + ['Harvey Keitel', 'The Ridiculous 6'], + ['Harvey Keitel', 'Youth'], + ['Jeff Goldblum', 'Independence Day: Resurgence'], + ['Jude Law', 'Spy'], + ['Mathieu Amalric', None], + ['Ralph Fiennes', 'A Bigger Splash'], + ['Ralph Fiennes', 'Spectre'], + ['Willem Dafoe', 'John Wick'], + ['Willem Dafoe', 'The Fault in Our Stars']] + ) + self.queries_info = [ self.number_of_actors_query, self.actors_played_with_nicolas_cage_query, @@ -384,7 +412,8 @@ def __init__(self, actors=None, movies=None): self.eighties_movies_index_scan, self.find_titles_starting_with_american_query, self.same_year_higher_rating_than_huntforthewilderpeople_query, - self.all_actors_named_tim + self.all_actors_named_tim, + self.grand_budapest_hotel_cast_and_their_other_roles ] def queries(self): From a99378454a5fedff900aa17498530f2d5b5715e4 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Fri, 3 Apr 2020 11:05:46 -0400 Subject: [PATCH 30/32] Use standard Python client for automation --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index f1eea21e65..306d99a845 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -27,7 +27,7 @@ jobs: - run: name: Install prerequisite command: >- - pip install git+https://github.com/RedisGraph/redisgraph-py.git@null-graph-entities && + pip install git+https://github.com/RedisGraph/redisgraph-py.git@master && pip install psutil behave && pip install redis-py-cluster && pip install git+https://github.com/RedisLabsModules/RLTest.git@master && From 38ac97645d21ea51150491e3fdc0e8d662d04914 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Mon, 6 Apr 2020 12:57:38 -0400 Subject: [PATCH 31/32] Emit all columns as SIValues in compact formatter --- docs/client_spec.md | 87 ++++++++++--------- .../formatters/resultset_formatter.h | 10 ++- .../formatters/resultset_replycompact.c | 45 ++-------- .../formatters/resultset_replyverbose.c | 1 - tests/flow/test_optional_match.py | 11 +++ tests/flow/test_results.py | 12 +-- tests/flow/test_with_clause.py | 14 ++- 7 files changed, 79 insertions(+), 101 deletions(-) diff --git a/docs/client_spec.md b/docs/client_spec.md index f8cc246278..6a9e275d9f 100644 --- a/docs/client_spec.md +++ b/docs/client_spec.md @@ -25,9 +25,9 @@ Instructions on how to efficiently convert these IDs in the [Procedure Calls](#p Additionally, two enums are exposed: -[ColumnType](https://github.com/RedisGraph/RedisGraph/blob/ff108d7e21061025166a35d29be1a1cb5bac6d55/src/resultset/formatters/resultset_formatter.h#L14-L19) indicates what type of value is held in each column (more formally, that offset into each row of the result set). Each entry in the header row will be a 2-array, with this enum in the first position and the column name string in the second. +[ColumnType](https://github.com/RedisGraph/RedisGraph/blob/ff108d7e21061025166a35d29be1a1cb5bac6d55/src/resultset/formatters/resultset_formatter.h#L14-L19), which as of RedisGraph v2.1.0 will always be `COLUMN_SCALAR`. This enum is retained for backwards compatibility, and may be ignored by the client unless RedisGraph versions older than v2.1.0 must be supported. -[PropertyType](https://github.com/RedisGraph/RedisGraph/blob/ff108d7e21061025166a35d29be1a1cb5bac6d55/src/resultset/formatters/resultset_formatter.h#L21-L28) indicates the data type (such as integer or string) of each returned scalar value. Each scalar values is emitted as a 2-array, with this enum in the first position and the actual value in the second. A column can consist exclusively of scalar values, such as both of the columns created by `RETURN a.value, 'this literal string'`. Each property on a graph entity also has a scalar as its value, so this construction is nested in each value of the properties array when a column contains a node or relationship. +[ValueType](https://github.com/RedisGraph/RedisGraph/blob/ff108d7e21061025166a35d29be1a1cb5bac6d55/src/resultset/formatters/resultset_formatter.h#L21-L28) indicates the data type (such as Node, integer, or string) of each returned value. Each value is emitted as a 2-array, with this enum in the first position and the actual value in the second. Each property on a graph entity also has a scalar as its value, so this construction is nested in each value of the properties array when a column contains a node or relationship. ## Decoding the result set @@ -69,24 +69,26 @@ Verbose (default): Compact: ```sh 127.0.0.1:6379> GRAPH.QUERY demo "MATCH (a)-[e]->(b) RETURN a, e, b.name" --compact -1) 1) 1) (integer) 2 +1) 1) 1) (integer) 1 2) "a" - 2) 1) (integer) 3 + 2) 1) (integer) 1 2) "e" 3) 1) (integer) 1 2) "b.name" -2) 1) 1) 1) (integer) 0 +2) 1) 1) 1) (integer) 8 2) 1) (integer) 0 - 3) 1) 1) (integer) 0 - 2) (integer) 2 - 3) "Tree" - 2) 1) (integer) 0 - 2) (integer) 0 - 3) (integer) 0 - 4) (integer) 1 - 5) 1) 1) (integer) 1 - 2) (integer) 2 - 3) "Autumn" + 2) 1) (integer) 0 + 3) 1) 1) (integer) 0 + 2) (integer) 2 + 3) "Tree" + 2) 1) (integer) 7 + 2) 1) (integer) 0 + 2) (integer) 0 + 3) (integer) 0 + 4) (integer) 1 + 5) 1) 1) (integer) 1 + 2) (integer) 2 + 3) "Autumn" 3) 1) (integer) 2 2) "Apple" 3) 1) "Query internal execution time: 1.085412 milliseconds" @@ -119,15 +121,15 @@ Rather than introspecting on the query being emitted, the client implementation Our sample query `MATCH (a)-[e]->(b) RETURN a, e, b.name` generated the header: ```sh -1) 1) (integer) 2 +1) 1) (integer) 1 2) "a" -2) 1) (integer) 3 - 2) "e" 3) 1) (integer) 1 - 2) "b.name" + 3) "e" +4) 1) (integer) 1 + 3) "b.name" ``` -The 3 array members correspond, in order, to the 3 entities described in the RETURN clause. +The 4 array members correspond, in order, to the 3 entities described in the RETURN clause. Each is emitted as a 2-array: ```sh @@ -135,9 +137,7 @@ Each is emitted as a 2-array: 2) column name (string) ``` -It is the client's responsibility to store [ColumnType enum](https://github.com/RedisGraph/RedisGraph/blob/ff108d7e21061025166a35d29be1a1cb5bac6d55/src/resultset/formatters/resultset_formatter.h#L14-L19). RedisGraph guarantees that this enum may be extended in the future, but the existing values will not be altered. - -In this case, `a` corresponds to a Node column, `e` corresponds to a Relation column, and `b.name` corresponds to a Scalar column. No other column types are currently supported. +The first element is the [ColumnType enum](https://github.com/RedisGraph/RedisGraph/blob/master/src/resultset/formatters/resultset_formatter.h#L14-L19), which as of RedisGraph v2.1.0 will always be `COLUMN_SCALAR`. This element is retained for backwards compatibility, and may be ignored by the client unless RedisGraph versions older than v2.1.0 must be supported. ### Reading result rows @@ -145,37 +145,42 @@ The entity representations in this section will closely resemble those found in Our query produced one row of results with 3 columns (as described by the header): ```sh -1) 1) 1) (integer) 0 +1) 1) 1) (integer) 8 2) 1) (integer) 0 - 3) 1) 1) (integer) 0 - 2) (integer) 2 - 3) "Tree" - 2) 1) (integer) 0 - 2) (integer) 0 - 3) (integer) 0 - 4) (integer) 1 - 5) 1) 1) (integer) 1 - 2) (integer) 2 - 3) "Autumn" + 2) 1) (integer) 0 + 3) 1) 1) (integer) 0 + 2) (integer) 2 + 3) "Tree" + 2) 1) (integer) 7 + 2) 1) (integer) 0 + 2) (integer) 0 + 3) (integer) 0 + 4) (integer) 1 + 5) 1) 1) (integer) 1 + 2) (integer) 2 + 3) "Autumn" 3) 1) (integer) 2 2) "Apple" ``` +Each element is emitted as a 2-array - [`ValueType`, value]. + +It is the client's responsibility to store the [ValueType enum](https://github.com/RedisGraph/RedisGraph/blob/master/src/resultset/formatters/resultset_formatter.h#L21-L28). RedisGraph guarantees that this enum may be extended in the future, but the existing values will not be altered. -We know the first column to contain nodes. The node representation contains 3 top-level elements: +The `ValueType` for the first entry is `VALUE_NODE`. The node representation contains 3 top-level elements: 1. The node's internal ID. 2. An array of all label IDs associated with the node (currently, each node can have either 0 or 1 labels, though this restriction may be lifted in the future). -3. An array of all properties the node contains. Properties are represented as 3-arrays - [property key ID, `PropertyType` enum, value]. +3. An array of all properties the node contains. Properties are represented as 3-arrays - [property key ID, `ValueType`, value]. ```sh [ Node ID (integer), [label ID (integer) X label count] - [[property key ID (integer), PropertyType (enum), value (scalar)] X property count] + [[property key ID (integer), ValueType (enum), value (scalar)] X property count] ] ``` -The second column contains relations. The relation representation differs from the node representation in two respects: +The `ValueType` for the first entry is `VALUE_EDGE`. The edge representation differs from the node representation in two respects: - Each relation has exactly one type, rather than the 0+ labels a node may have. - A relation is emitted with the IDs of its source and destination nodes. @@ -194,13 +199,11 @@ As such, the complete representation is as follows: type ID (integer), source node ID (integer), destination node ID (integer), - [[property key ID (integer), PropertyType (enum), value (scalar)] X property count] + [[property key ID (integer), ValueType (enum), value (scalar)] X property count] ] ``` -The third column contains a scalar. Each scalar is emitted as a 2-array - [`PropertyType` enum, value]. - -As with ColumnType, it is the client's responsibility to store the [PropertyType enum](https://github.com/RedisGraph/RedisGraph/blob/ff108d7e21061025166a35d29be1a1cb5bac6d55/src/resultset/formatters/resultset_formatter.h#L21-L28). RedisGraph guarantees that this enum may be extended in the future, but the existing values will not be altered. +The `ValueType` for the third entry is `VALUE_STRING`, and the other element in the array is the actual value, "Apple". ### Reading statistics diff --git a/src/resultset/formatters/resultset_formatter.h b/src/resultset/formatters/resultset_formatter.h index d6ec2d7ea8..3e5ee28895 100644 --- a/src/resultset/formatters/resultset_formatter.h +++ b/src/resultset/formatters/resultset_formatter.h @@ -14,8 +14,8 @@ typedef enum { COLUMN_UNKNOWN = 0, COLUMN_SCALAR = 1, - COLUMN_NODE = 2, - COLUMN_RELATION = 3, + COLUMN_NODE = 2, // Unused, retained for client compatibility. + COLUMN_RELATION = 3, // Unused, retained for client compatibility. } ColumnType; typedef enum { @@ -32,10 +32,12 @@ typedef enum { } ValueType; // Typedef for header formatters. -typedef void (*EmitHeaderFunc)(RedisModuleCtx *ctx, const char **columns, const Record r, uint *col_rec_map); +typedef void (*EmitHeaderFunc)(RedisModuleCtx *ctx, const char **columns, const Record r, + uint *col_rec_map); // Typedef for record formatters. -typedef void (*EmitRecordFunc)(RedisModuleCtx *ctx, GraphContext *gc, const Record r, uint numcols, uint *col_rec_map); +typedef void (*EmitRecordFunc)(RedisModuleCtx *ctx, GraphContext *gc, const Record r, uint numcols, + uint *col_rec_map); typedef struct { EmitRecordFunc EmitRecord; diff --git a/src/resultset/formatters/resultset_replycompact.c b/src/resultset/formatters/resultset_replycompact.c index 8e89c10880..bfd5f4e27b 100644 --- a/src/resultset/formatters/resultset_replycompact.c +++ b/src/resultset/formatters/resultset_replycompact.c @@ -229,53 +229,22 @@ void ResultSet_EmitCompactRecord(RedisModuleCtx *ctx, GraphContext *gc, const Re for(uint i = 0; i < numcols; i++) { uint idx = col_rec_map[i]; - switch(Record_GetType(r, idx)) { - case REC_TYPE_NODE: - _ResultSet_CompactReplyWithNode(ctx, gc, Record_GetNode(r, idx)); - break; - case REC_TYPE_EDGE: - _ResultSet_CompactReplyWithEdge(ctx, gc, Record_GetEdge(r, idx)); - break; - default: - RedisModule_ReplyWithArray(ctx, 2); // Reply with array with space for type and value - _ResultSet_CompactReplyWithSIValue(ctx, gc, Record_Get(r, idx)); - } + RedisModule_ReplyWithArray(ctx, 2); // Reply with array with space for type and value + _ResultSet_CompactReplyWithSIValue(ctx, gc, Record_Get(r, idx)); } } -// For every column in the header, emit a 2-array that specifies -// the column alias followed by an enum denoting what type -// (scalar, node, or relation) it holds. +// For every column in the header, emit a 2-array containing the ColumnType enum +// followed by the column alias. void ResultSet_ReplyWithCompactHeader(RedisModuleCtx *ctx, const char **columns, const Record r, uint *col_rec_map) { uint columns_len = array_len(columns); RedisModule_ReplyWithArray(ctx, columns_len); for(uint i = 0; i < columns_len; i++) { RedisModule_ReplyWithArray(ctx, 2); - ColumnType t; - // First, emit the column type enum - if(r) { - RecordEntryType entry_type = Record_GetType(r, col_rec_map[i]); - switch(entry_type) { - case REC_TYPE_NODE: - t = COLUMN_NODE; - break; - case REC_TYPE_EDGE: - t = COLUMN_RELATION; - break; - case REC_TYPE_SCALAR: - case REC_TYPE_UNKNOWN: // Treat unknown as scalar. - t = COLUMN_SCALAR; - break; - default: - assert(false); - } - } else { - // If we didn't receive a record, no results will be returned, - // and the column types don't matter. - t = COLUMN_SCALAR; - } - + /* Because the types found in the first Record do not necessarily inform the types + * in subsequent records, we will always set the column type as scalar. */ + ColumnType t = COLUMN_SCALAR; RedisModule_ReplyWithLongLong(ctx, t); // Second, emit the identifier string associated with the column diff --git a/src/resultset/formatters/resultset_replyverbose.c b/src/resultset/formatters/resultset_replyverbose.c index 1ccf811247..715670fd9f 100644 --- a/src/resultset/formatters/resultset_replyverbose.c +++ b/src/resultset/formatters/resultset_replyverbose.c @@ -19,7 +19,6 @@ static void _ResultSet_VerboseReplyWithPath(RedisModuleCtx *ctx, SIValue path); * and NULL values. */ static void _ResultSet_VerboseReplyWithSIValue(RedisModuleCtx *ctx, GraphContext *gc, const SIValue v) { - // Emit the actual value, then the value type (to facilitate client-side parsing) switch(SI_TYPE(v)) { case T_STRING: RedisModule_ReplyWithStringBuffer(ctx, v.stringval, strlen(v.stringval)); diff --git a/tests/flow/test_optional_match.py b/tests/flow/test_optional_match.py index 3a63ffc182..352888b038 100644 --- a/tests/flow/test_optional_match.py +++ b/tests/flow/test_optional_match.py @@ -203,3 +203,14 @@ def test15_optional_named_path(self): [None], [None]] self.env.assertEquals(actual_result.result_set, expected_result) + + # Return a result set with null values in the first record and non-null values in subsequent records. + def test16_optional_null_first_result(self): + global redis_graph + query = """MATCH (a) OPTIONAL MATCH (a)-[e]->(b) RETURN a.v, b.v, TYPE(e) ORDER BY EXISTS(b), a.v, b.v""" + actual_result = redis_graph.query(query) + expected_result = [['v3', None, None], + ['v4', None, None], + ['v1', 'v2', 'E1'], + ['v2', 'v3', 'E2']] + self.env.assertEquals(actual_result.result_set, expected_result) diff --git a/tests/flow/test_results.py b/tests/flow/test_results.py index 2bc11838bd..c9ea9fae38 100644 --- a/tests/flow/test_results.py +++ b/tests/flow/test_results.py @@ -101,15 +101,11 @@ def test05_distinct_full_entities(self): def test06_return_all(self): query = """MATCH (a)-[e]->(b) RETURN *""" result = graph.query(query) - - # These definitions are duplicates of the non-public ResultSetColumnTypes values in redisgraph-py - COLUMN_SCALAR = 1 - COLUMN_NODE = 2 - COLUMN_RELATION = 3 - # Validate the header strings and value types + # Validate the header strings of the 3 columns. # NOTE - currently, RETURN * populates values in alphabetical order, but that is subject to later change. - expected_header = [[COLUMN_NODE, 'a'], [COLUMN_NODE, 'b'], [COLUMN_RELATION, 'e']] - self.env.assertEqual(result.header, expected_header) + self.env.assertEqual(result.header[0][1], 'a') + self.env.assertEqual(result.header[1][1], 'b') + self.env.assertEqual(result.header[2][1], 'e') # Verify that 3 columns are returned self.env.assertEqual(len(result.result_set[0]), 3) diff --git a/tests/flow/test_with_clause.py b/tests/flow/test_with_clause.py index 69490df6c4..ec4b795fc9 100644 --- a/tests/flow/test_with_clause.py +++ b/tests/flow/test_with_clause.py @@ -172,15 +172,13 @@ def test07_projected_graph_entities(self): query = """MATCH (a)-[e]->(b) WITH a, e, b.b_val AS b_val ORDER BY a.a_val LIMIT 2 RETURN *""" actual_result = redis_graph.query(query) - # These definitions are duplicates of the non-public ResultSetColumnTypes values in redisgraph-py - COLUMN_SCALAR = 1 - COLUMN_NODE = 2 - COLUMN_RELATION = 3 - # Validate the header strings and value types + # Validate the header strings of the 3 columns. # NOTE - currently, RETURN * populates values in alphabetical order, but that is subject to later change. - expected_header = [[COLUMN_NODE, 'a'], [COLUMN_SCALAR, 'b_val'], [COLUMN_RELATION, 'e']] - self.env.assertEqual(actual_result.header, expected_header) - # Verify that 2 rows and 3 columns are returned + self.env.assertEqual(actual_result.header[0][1], 'a') + self.env.assertEqual(actual_result.header[1][1], 'b_val') + self.env.assertEqual(actual_result.header[2][1], 'e') + + # Verify that 2 rows and 3 columns are returned. self.env.assertEqual(len(actual_result.result_set), 2) self.env.assertEqual(len(actual_result.result_set[0]), 3) From 732be69f85b9c81640047ef89d956ebcd87cc8f2 Mon Sep 17 00:00:00 2001 From: Jeffrey Lovitz Date: Mon, 6 Apr 2020 13:52:22 -0400 Subject: [PATCH 32/32] Improve flow test for null entities in first result --- tests/flow/test_optional_match.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/tests/flow/test_optional_match.py b/tests/flow/test_optional_match.py index 352888b038..a579d18386 100644 --- a/tests/flow/test_optional_match.py +++ b/tests/flow/test_optional_match.py @@ -5,6 +5,7 @@ from base import FlowTestsBase redis_graph = None +nodes = {} class testOptionalFlow(FlowTestsBase): def __init__(self): @@ -15,20 +16,20 @@ def __init__(self): self.populate_graph() def populate_graph(self): + global nodes # Construct a graph with the form: # (v1)-[:E1]->(v2)-[:E2]->(v3), (v4) node_props = ['v1', 'v2', 'v3', 'v4'] - nodes = [] for idx, v in enumerate(node_props): node = Node(label="L", properties={"v": v}) - nodes.append(node) + nodes[v] = node redis_graph.add_node(node) - edge = Edge(nodes[0], "E1", nodes[1]) + edge = Edge(nodes['v1'], "E1", nodes['v2']) redis_graph.add_edge(edge) - edge = Edge(nodes[1], "E2", nodes[2]) + edge = Edge(nodes['v2'], "E2", nodes['v3']) redis_graph.add_edge(edge) redis_graph.flush() @@ -207,10 +208,10 @@ def test15_optional_named_path(self): # Return a result set with null values in the first record and non-null values in subsequent records. def test16_optional_null_first_result(self): global redis_graph - query = """MATCH (a) OPTIONAL MATCH (a)-[e]->(b) RETURN a.v, b.v, TYPE(e) ORDER BY EXISTS(b), a.v, b.v""" + query = """MATCH (a) OPTIONAL MATCH (a)-[e]->(b) RETURN a, b, TYPE(e) ORDER BY EXISTS(b), a.v, b.v""" actual_result = redis_graph.query(query) - expected_result = [['v3', None, None], - ['v4', None, None], - ['v1', 'v2', 'E1'], - ['v2', 'v3', 'E2']] + expected_result = [[nodes['v3'], None, None], + [nodes['v4'], None, None], + [nodes['v1'], nodes['v2'], 'E1'], + [nodes['v2'], nodes['v3'], 'E2']] self.env.assertEquals(actual_result.result_set, expected_result)