diff --git a/src/cypher/cypher.c b/src/cypher/cypher.c index 5724f3ac6..957860f3d 100644 --- a/src/cypher/cypher.c +++ b/src/cypher/cypher.c @@ -4156,17 +4156,34 @@ static void execute_with_clause(cbm_query_t *q, binding_t **bindings_ptr, int *b /* Project RETURN * — all bound variable properties */ /* Collect all variable names from query patterns */ +/* Has this variable already been collected? A query may name the same variable + * in more than one pattern, and RETURN * must give it one set of columns. */ +static bool star_var_seen(const char **vars, int vc, const char *name) { + for (int i = 0; i < vc; i++) { + if (strcmp(vars[i], name) == 0) { + return true; + } + } + return false; +} + +/* Collect the variables a RETURN * projects, in the order the query names them + * and with no repeats. Without the repeat check, `MATCH (f) OPTIONAL MATCH + * (f)-[:CALLS]->(g)` names f in two patterns and f gets its four columns + * twice. */ static int collect_pattern_vars(cbm_query_t *q, const char **vars, int max_vars) { int vc = 0; for (int pi = 0; pi < q->pattern_count; pi++) { for (int ni = 0; ni < q->patterns[pi].node_count && vc < max_vars; ni++) { - if (q->patterns[pi].nodes[ni].variable) { - vars[vc++] = q->patterns[pi].nodes[ni].variable; + const char *var = q->patterns[pi].nodes[ni].variable; + if (var && !star_var_seen(vars, vc, var)) { + vars[vc++] = var; } } for (int ri = 0; ri < q->patterns[pi].rel_count && vc < max_vars; ri++) { - if (q->patterns[pi].rels[ri].variable) { - vars[vc++] = q->patterns[pi].rels[ri].variable; + const char *var = q->patterns[pi].rels[ri].variable; + if (var && !star_var_seen(vars, vc, var)) { + vars[vc++] = var; } } } @@ -4218,8 +4235,41 @@ static void project_star_row(binding_t *b, const char **vars, int vc, const char } } +/* RETURN * after a WITH. + * + * The pattern's variables are out of scope by this point — the WITH replaced + * them with the names it made. Each of those names holds one value, not a + * node, so each is ONE column rather than the four a node variable gets. + * + * Reading the pattern here instead is the fault this function exists to avoid: + * it named variables the bindings no longer hold, found nothing for every one + * of them, and answered a full result of empty strings with no error. */ +static void execute_return_star_after_with(cbm_query_t *q, binding_t *bindings, int bind_count, + int max_rows, result_builder_t *rb) { + cbm_return_clause_t *wc = q->with_clause; + char name_bufs[CYP_MAX_VARS][CBM_SZ_128]; + const char *cols[CYP_MAX_VARS]; + int col_n = wc->count < CYP_MAX_VARS ? wc->count : CYP_MAX_VARS; + for (int i = 0; i < col_n; i++) { + cols[i] = resolve_item_alias(&wc->items[i], name_bufs[i], sizeof(name_bufs[i])); + } + rb_set_columns(rb, cols, col_n); + for (int bi = 0; bi < bind_count && rb->row_count < max_rows; bi++) { + const char *vals[CYP_MAX_VARS]; + for (int i = 0; i < col_n; i++) { + cbm_node_t *vn = binding_get(&bindings[bi], cols[i]); + vals[i] = vn && vn->name ? vn->name : ""; + } + rb_add_row(rb, vals); + } +} + static void execute_return_star(cbm_query_t *q, binding_t *bindings, int bind_count, int max_rows, result_builder_t *rb) { + if (q->with_clause) { + execute_return_star_after_with(q, bindings, bind_count, max_rows, rb); + return; + } const char *vars[CBM_SZ_32]; int vc = collect_pattern_vars(q, vars, CBM_SZ_32); build_star_columns(rb, vars, vc); diff --git a/tests/test_cypher.c b/tests/test_cypher.c index c37b2b7d0..d7081c600 100644 --- a/tests/test_cypher.c +++ b/tests/test_cypher.c @@ -2859,6 +2859,50 @@ TEST(cypher_exec_return_star) { PASS(); } +TEST(cypher_return_star_dedups_repeated_pattern_var) { + /* RETURN * collected its column variables from every pattern in turn and + * never deduped, so a variable named in two patterns got its four columns + * twice. Here f is named in the MATCH and again in the OPTIONAL MATCH, so + * eight columns is right and twelve is the fault. */ + cbm_store_t *s = setup_cypher_store(); + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute(s, "MATCH (f:Function) OPTIONAL MATCH (f)-[:CALLS]->(g) RETURN *", + "test", 0, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.col_count, 8); + ASSERT_STR_EQ(r.columns[0], "f.name"); + ASSERT_STR_EQ(r.columns[4], "g.name"); + cbm_cypher_result_free(&r); + cbm_store_close(s); + PASS(); +} + +TEST(cypher_return_star_after_with_names_aliases) { + /* RETURN * built its columns from the query pattern, never from the + * bindings it was about to project. After a WITH the live scope is the + * aliases the WITH made, so the old code asked for f and g, found neither, + * and answered every value empty with no error. */ + cbm_store_t *s = setup_cypher_store(); + cbm_cypher_result_t r = {0}; + int rc = cbm_cypher_execute(s, + "MATCH (f:Function)-[:CALLS]->(g) " + "WITH f.name AS caller, g.name AS callee RETURN *", + "test", 0, &r); + ASSERT_EQ(rc, 0); + ASSERT_EQ(r.col_count, 2); + ASSERT_STR_EQ(r.columns[0], "caller"); + ASSERT_STR_EQ(r.columns[1], "callee"); + /* Three CALLS edges in the fixture. */ + ASSERT_EQ(r.row_count, 3); + for (int i = 0; i < r.row_count; i++) { + ASSERT_TRUE(r.rows[i][0][0] != '\0'); + ASSERT_TRUE(r.rows[i][1][0] != '\0'); + } + cbm_cypher_result_free(&r); + cbm_store_close(s); + PASS(); +} + TEST(cypher_parse_neq) { cbm_query_t *q = NULL; char *err = NULL; @@ -4161,6 +4205,8 @@ SUITE(cypher) { RUN_TEST(cypher_exec_where_is_null); RUN_TEST(cypher_exec_where_is_not_null); RUN_TEST(cypher_exec_return_star); + RUN_TEST(cypher_return_star_dedups_repeated_pattern_var); + RUN_TEST(cypher_return_star_after_with_names_aliases); RUN_TEST(cypher_parse_neq); RUN_TEST(cypher_parse_in); RUN_TEST(cypher_parse_is_null);