Skip to content

fix(cypher): classify aggregation columns by is_aggregate_func, not a bare func check #1221

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AmirF194 wants to merge 1 commit into
base: main
Choose a base branch
from
+55 −9
Open

fix(cypher): classify aggregation columns by is_aggregate_func, not a bare func check #1221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions src/cypher/cypher.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3720,10 +3720,11 @@ typedef struct {
static int with_agg_build_key(cbm_return_clause_t *wc, binding_t *b, char *key, size_t key_sz) {
int kl = 0;
for (int ci = 0; ci < wc->count; ci++) {
if (wc->items[ci].func) {
if (is_aggregate_func(wc->items[ci].func)) {
continue;
}
const char *v = binding_get_virtual(b, wc->items[ci].variable, wc->items[ci].property);
char vbuf[CBM_SZ_512];
const char *v = project_item(b, &wc->items[ci], vbuf, sizeof(vbuf));
kl += snprintf(key + kl, key_sz - (size_t)kl, "%s|", v);
if (kl >= (int)key_sz) {
kl = (int)key_sz - SKIP_ONE;
Expand Down Expand Up @@ -3759,11 +3760,12 @@ static int with_agg_find_or_create(with_agg_t **aggs, int *agg_cnt, int *agg_cap
(*aggs)[found].maxs[ci] = -CYP_DBL_MAX;
}
for (int ci = 0; ci < wc->count; ci++) {
if (wc->items[ci].func) {
if (is_aggregate_func(wc->items[ci].func)) {
(*aggs)[found].group_vals[ci] = heap_strdup("0");
continue;
}
const char *v = binding_get_virtual(b, wc->items[ci].variable, wc->items[ci].property);
char vbuf[CBM_SZ_512];
const char *v = project_item(b, &wc->items[ci], vbuf, sizeof(vbuf));
(*aggs)[found].group_vals[ci] = heap_strdup(v);
/* If this group item is a bare node variable, remember its id so the
* carried virtual var can re-fetch any property (group_vals holds only
Expand All @@ -3781,7 +3783,7 @@ static int with_agg_find_or_create(with_agg_t **aggs, int *agg_cnt, int *agg_cap
/* Accumulate aggregation values for a binding */
static void with_agg_accumulate(with_agg_t *agg, cbm_return_clause_t *wc, binding_t *b) {
for (int ci = 0; ci < wc->count; ci++) {
if (!wc->items[ci].func) {
if (!is_aggregate_func(wc->items[ci].func)) {
continue;
}
agg->counts[ci]++;
Expand Down Expand Up @@ -3876,7 +3878,7 @@ static void execute_with_aggregate(cbm_return_clause_t *wc, binding_t *bindings,
for (int ci = 0; ci < wc->count; ci++) {
char name_buf[CBM_SZ_256];
const char *alias = resolve_item_alias(&wc->items[ci], name_buf, sizeof(name_buf));
if (wc->items[ci].func) {
if (is_aggregate_func(wc->items[ci].func)) {
char vbuf[CBM_SZ_64];
if (wc->items[ci].distinct && strcmp(wc->items[ci].func, "COUNT") == 0) {
snprintf(vbuf, sizeof(vbuf), "%d", aggs[a].distinct_n[ci]); /* #239 */
Expand Down Expand Up @@ -4175,7 +4177,7 @@ static void ret_agg_init_group(ret_agg_entry_t *entry, const char *key, int item
/* Accumulate a binding into RETURN aggregation */
static void ret_agg_accumulate(ret_agg_entry_t *entry, cbm_return_clause_t *ret, binding_t *b) {
for (int ci = 0; ci < ret->count; ci++) {
if (!ret->items[ci].func) {
if (!is_aggregate_func(ret->items[ci].func)) {
continue;
}
entry->counts[ci]++;
Expand Down Expand Up @@ -4227,7 +4229,7 @@ static void ret_agg_build_key(cbm_return_clause_t *ret, binding_t *b, char *key,
const char **vals, char valbufs[][CBM_SZ_512]) {
int klen = 0;
for (int ci = 0; ci < ret->count; ci++) {
if (ret->items[ci].func) {
if (is_aggregate_func(ret->items[ci].func)) {
vals[ci] = "0";
continue;
}
Expand All @@ -4251,7 +4253,7 @@ static void ret_agg_emit_row(cbm_return_clause_t *ret, ret_agg_entry_t *agg, res
const char *row[CBM_SZ_32];
char bufs[CBM_SZ_32][CBM_SZ_64];
for (int ci = 0; ci < ret->count; ci++) {
if (!ret->items[ci].func) {
if (!is_aggregate_func(ret->items[ci].func)) {
row[ci] = agg->group_vals[ci];
continue;
}
Expand Down
44 changes: 44 additions & 0 deletions tests/test_cypher.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2469,6 +2469,28 @@ TEST(cypher_exec_count_star) {
PASS();
}

/* #1111: type(r) grouped with count(*) must return the actual relationship type,
* not the row count. ret_agg_build_key/ret_agg_emit_row classified aggregate vs.
* scalar columns with a bare `item->func` truthy check, so type(r) (a non-aggregate
* function, func != NULL) was misrouted into the aggregate-value branch and
* formatted via format_agg_value's default case, silently substituting the row
* count for the relationship type. */
TEST(cypher_issue1111_return_type_count_group) {
cbm_store_t *s = setup_cypher_store();
cbm_cypher_result_t r = {0};
int rc = cbm_cypher_execute(
s, "MATCH (a)-[r]->(b) RETURN type(r) AS t, count(*) AS n ORDER BY n DESC", "test", 0, &r);
ASSERT_EQ(rc, 0);
ASSERT_EQ(r.row_count, 2);
ASSERT_STR_EQ(r.rows[0][0], "CALLS");
ASSERT_STR_EQ(r.rows[0][1], "3");
ASSERT_STR_EQ(r.rows[1][0], "DEFINES");
ASSERT_STR_EQ(r.rows[1][1], "1");
cbm_cypher_result_free(&r);
cbm_store_close(s);
PASS();
}

TEST(cypher_parse_skip) {
cbm_query_t *q = NULL;
char *err = NULL;
Expand Down Expand Up @@ -2653,6 +2675,26 @@ TEST(cypher_exec_with_node_groupvar_prop) {
PASS();
}

/* #1111, WITH variant: the same misrouting in with_agg_build_key/with_agg_accumulate/
* execute_with_aggregate's per-column func check. */
TEST(cypher_issue1111_with_type_count_group) {
cbm_store_t *s = setup_cypher_store();
cbm_cypher_result_t r = {0};
int rc = cbm_cypher_execute(
s,
"MATCH (a)-[r]->(b) WITH type(r) AS t, count(*) AS n RETURN t, n ORDER BY n DESC",
"test", 0, &r);
ASSERT_EQ(rc, 0);
ASSERT_EQ(r.row_count, 2);
ASSERT_STR_EQ(r.rows[0][0], "CALLS");
ASSERT_STR_EQ(r.rows[0][1], "3");
ASSERT_STR_EQ(r.rows[1][0], "DEFINES");
ASSERT_STR_EQ(r.rows[1][1], "1");
cbm_cypher_result_free(&r);
cbm_store_close(s);
PASS();
}

TEST(cypher_exec_with_where) {
cbm_store_t *s = setup_cypher_store();
cbm_cypher_result_t r = {0};
Expand Down Expand Up @@ -3301,6 +3343,7 @@ SUITE(cypher) {
RUN_TEST(cypher_exec_max);
RUN_TEST(cypher_exec_collect);
RUN_TEST(cypher_exec_count_star);
RUN_TEST(cypher_issue1111_return_type_count_group);
RUN_TEST(cypher_parse_skip);
RUN_TEST(cypher_parse_sum_avg);
RUN_TEST(cypher_parse_collect);
Expand All @@ -3314,6 +3357,7 @@ SUITE(cypher) {
/* Phase 6: WITH clause */
RUN_TEST(cypher_exec_with_rename);
RUN_TEST(cypher_exec_with_count);
RUN_TEST(cypher_issue1111_with_type_count_group);
RUN_TEST(cypher_exec_with_node_groupvar_prop);
RUN_TEST(cypher_exec_with_where);
RUN_TEST(cypher_exec_with_orderby_limit);
Expand Down
Loading