diff --git a/docs/testing/semantic-coverage-matrix.md b/docs/testing/semantic-coverage-matrix.md index aad5250c..2d249e9b 100644 --- a/docs/testing/semantic-coverage-matrix.md +++ b/docs/testing/semantic-coverage-matrix.md @@ -656,3 +656,29 @@ zero regressions; unit 944/944; functional clean): evaluates), though the Quantifier1-4 [8]/[9] scenarios additionally need varlen `relationships(p)`/`nodes(p)` through an aggregating WITH + GROUP-BY-on-list (deeper, deferred). + +## Coverage update (2026-06-02) — ORDER BY in WITH flows to downstream aggregation; overflow-safe temporal ordering (+10) + +`transform_with.c`, `runtime/udf_helpers.c`. Verified via the TCK harness (rigorous +full pass-set diff: **zero regressions, +10**, 3776→3786; unit 944/944; functional +clean). Closes the entire `WithOrderBy1` [45] cluster ("Sort order should be +consistent with comparisons where comparisons are defined", all 10 type examples). + +- **`WITH … ORDER BY WITH collect(x)` now sees rows in sorted order.** + An ORDER BY on a non-aggregating WITH was only pushed into the WITH's CTE body + when it referenced a *dropped* input variable (WithOrderBy2 [21]-[24]); for an + ORDER BY over a *kept* input variable it stayed on the outer SELECT, which a + subsequent aggregating WITH (`collect`) never observed — so `collect` aggregated + in UNWIND order. Now, when every ORDER BY identifier is a live input variable and + the WITH has no LIMIT/SKIP, the ORDER BY is *also* emitted inside the CTE body + (`order_expr_all_live_input` gate) so the downstream aggregate's input is ordered + (SQLite preserves an ordered subquery's row order through `json_group_array`). + The outer ORDER BY is still emitted for final-output ordering. +- **Temporal `<`/`>` ordering no longer overflows int64 for far-future years.** + `gql_order_cmp_func`'s temporal path compared `parse_temporal_ns()` values, where + `epoch_seconds * 1e9` overflows int64 around year ~2262 and wraps negative — so + `localdatetime/datetime({year: 9999})` compared as the *smallest* value, making + `<` disagree with the ORDER BY key (which uses the zero-padded ISO string). New + `cmp_temporal_strings()` parses each operand to `(epoch_seconds, sub_second_ns)` + and compares componentwise (no overflow across the full Cypher year range), + falling back to lexical compare on parse failure. diff --git a/src/backend/runtime/udf_helpers.c b/src/backend/runtime/udf_helpers.c index 6bd3cc74..caff93b2 100644 --- a/src/backend/runtime/udf_helpers.c +++ b/src/backend/runtime/udf_helpers.c @@ -383,6 +383,11 @@ static int gql_cmp_json_vals(sqlite3 *db, * Called by transform_binary_operation's LT/GT/LTE/GTE path — * each operand is transformed exactly once. */ static int64_t parse_temporal_ns(const char *s); /* defined below */ +/* Overflow-safe temporal ordering: compares two ISO temporal strings by their + * UTC instant using (epoch_seconds, sub_second_ns) components, so far-future + * years whose epoch-nanoseconds exceed int64 don't wrap. Returns -1/0/1, or + * -2 if either operand fails to parse. (WithOrderBy1 [45].) */ +static int cmp_temporal_strings(const char *a, const char *b); /* defined below */ /* Heuristic: does the text look like a Cypher temporal value (time 'HH:MM…' or * date/datetime 'YYYY-MM-DD…')? Used so ordered comparison of two temporals @@ -475,9 +480,11 @@ void gql_order_cmp_func( if (!a || !b) { sqlite3_result_null(context); return; } if (looks_temporal(a) && looks_temporal(b)) { /* Compare two temporal values by UTC instant, not lexically, so a - * tz offset is honored (Temporal7 [3]: 10:00+01:00 < 09:35+00:00). */ - int64_t na = parse_temporal_ns(a), nb = parse_temporal_ns(b); - cmp = (na < nb) ? -1 : (na > nb) ? 1 : 0; + * tz offset is honored (Temporal7 [3]: 10:00+01:00 < 09:35+00:00). + * Use the overflow-safe (seconds, ns) comparison so far-future + * years (9999) don't wrap int64 epoch-ns (WithOrderBy1 [45]). */ + cmp = cmp_temporal_strings(a, b); + if (cmp == -2) cmp = strcmp(a, b); /* unparseable → lexical */ } else { cmp = strcmp(a, b); } @@ -1554,6 +1561,70 @@ static int64_t parse_temporal_ns(const char *s) { return (int64_t)epoch * 1000000000LL + ns; } +/* Parse an ISO temporal string into UTC epoch SECONDS plus sub-second nanos, + * without the int64 overflow that (epoch * 1e9) suffers for far-future years. + * Mirrors parse_temporal_ns's field extraction. Returns false on parse error. */ +static bool parse_temporal_secs_ns(const char *s, int64_t *out_secs, int64_t *out_ns) { + if (!s) return false; + int y = 1970, mo = 1, d = 1, h = 0, mi = 0, sec = 0; + int64_t ns = 0; + int tz_offset_min = 0; + const char *time_start = NULL; + if (strlen(s) >= 10 && s[4] == '-') { + if (sscanf(s, "%d-%d-%d", &y, &mo, &d) < 3) return false; + if (s[10] == 'T' || s[10] == ' ') time_start = s + 11; + } else if (strlen(s) >= 5 && s[2] == ':') { + time_start = s; + } else { + return false; + } + if (time_start) { + sscanf(time_start, "%d:%d:%d", &h, &mi, &sec); + const char *dot = strchr(time_start, '.'); + if (dot) { + char buf[10] = { '0','0','0','0','0','0','0','0','0', 0 }; + int i; + for (i = 0; i < 9 && dot[i + 1] >= '0' && dot[i + 1] <= '9'; i++) + buf[i] = dot[i + 1]; + ns = atoll(buf); + } + const char *tz_from = dot ? dot + 1 : time_start; + const char *tz = NULL; + for (const char *q = tz_from; *q; q++) { + if (*q == 'Z' || *q == '+' || (*q == '-' && q > time_start + 2)) { tz = q; break; } + } + if (tz) { + if (*tz == 'Z') tz_offset_min = 0; + else { + int sign = (*tz == '+') ? 1 : -1; + int oh = 0, om = 0; + if (sscanf(tz + 1, "%d:%d", &oh, &om) >= 1 || + sscanf(tz + 1, "%2d%2d", &oh, &om) >= 1) + tz_offset_min = sign * (oh * 60 + om); + } + } + } + struct tm t; + memset(&t, 0, sizeof(t)); + t.tm_year = y - 1900; t.tm_mon = mo - 1; t.tm_mday = d; + t.tm_hour = h; t.tm_min = mi; t.tm_sec = sec; + time_t epoch = timegm(&t); + epoch -= tz_offset_min * 60; + *out_secs = (int64_t)epoch; + *out_ns = ns; + return true; +} + +static int cmp_temporal_strings(const char *a, const char *b) { + int64_t sa, na, sb, nb; + if (!parse_temporal_secs_ns(a, &sa, &na) || + !parse_temporal_secs_ns(b, &sb, &nb)) + return -2; + if (sa != sb) return (sa < sb) ? -1 : 1; + if (na != nb) return (na < nb) ? -1 : 1; + return 0; +} + /* Build openCypher Duration JSON object from a signed total-nanoseconds value. * Returns: {"_iso8601": "...", "months": 0, "days": D, "seconds": S, "nanosecondsOfSecond": N} * diff --git a/src/backend/transform/transform_with.c b/src/backend/transform/transform_with.c index 1f2090ac..db172e5a 100644 --- a/src/backend/transform/transform_with.c +++ b/src/backend/transform/transform_with.c @@ -224,6 +224,60 @@ static bool expr_refs_dropped_input_var(cypher_transform_context *ctx, ast_node } } +/* True if every identifier referenced by `expr` is live in the CURRENT + * (input) scope — i.e. the expression can be evaluated inside the WITH's CTE + * body (where the input tables are in FROM). Used to decide whether a + * non-aggregating WITH's ORDER BY can be pushed into the CTE body so a + * DOWNSTREAM aggregating WITH (e.g. `... ORDER BY value WITH collect(x)`) + * sees rows in sorted order. Mirrors validate_identifiers_in_scope_ex's walk; + * projected-only aliases (not yet input vars) make this return false, so they + * stay on the outer path. (WithOrderBy1 [45].) */ +static bool order_expr_all_live_input(cypher_transform_context *ctx, ast_node *expr) +{ + if (!expr) return true; + switch (expr->type) { + case AST_NODE_IDENTIFIER: { + cypher_identifier *id = (cypher_identifier*)expr; + if (!id->name) return true; + return transform_var_lookup(ctx->var_ctx, id->name) != NULL; + } + case AST_NODE_PROPERTY: + return order_expr_all_live_input(ctx, ((cypher_property*)expr)->expr); + case AST_NODE_BINARY_OP: { + cypher_binary_op *b = (cypher_binary_op*)expr; + return order_expr_all_live_input(ctx, b->left) && + order_expr_all_live_input(ctx, b->right); + } + case AST_NODE_NOT_EXPR: + return order_expr_all_live_input(ctx, ((cypher_not_expr*)expr)->expr); + case AST_NODE_NULL_CHECK: + return order_expr_all_live_input(ctx, ((cypher_null_check*)expr)->expr); + case AST_NODE_FUNCTION_CALL: { + cypher_function_call *fc = (cypher_function_call*)expr; + if (fc->args) + for (int i = 0; i < fc->args->count; i++) + if (!order_expr_all_live_input(ctx, fc->args->items[i])) return false; + return true; + } + case AST_NODE_LIST: { + cypher_list *l = (cypher_list*)expr; + if (l->items) + for (int i = 0; i < l->items->count; i++) + if (!order_expr_all_live_input(ctx, l->items->items[i])) return false; + return true; + } + case AST_NODE_SUBSCRIPT: { + cypher_subscript *s = (cypher_subscript*)expr; + return order_expr_all_live_input(ctx, s->expr) && + order_expr_all_live_input(ctx, s->index) && + order_expr_all_live_input(ctx, s->slice_start) && + order_expr_all_live_input(ctx, s->slice_end); + } + default: + return true; /* literals, parameters, etc. */ + } +} + /* * Transform an expression to a dynamically allocated string. * Uses a temporary buffer to capture output, then returns the result. @@ -670,16 +724,30 @@ int transform_with_clause(cypher_transform_context *ctx, cypher_with *with) nm = ((cypher_property*)it->expr)->property_name; if (nm) proj_names[proj_n++] = (char*)nm; } - bool needs = false, has_agg_order = false; + bool needs = false, has_agg_order = false, all_live_input = true; for (int i = 0; i < with->order_by->count; i++) { cypher_order_by_item *oi = (cypher_order_by_item*)with->order_by->items[i]; if (find_aggregating_call(oi->expr)) has_agg_order = true; if (expr_refs_dropped_input_var(ctx, oi->expr, proj_names, proj_n)) needs = true; + if (!order_expr_all_live_input(ctx, oi->expr)) all_live_input = false; } /* Aggregating ORDER BY exprs (e.g. ORDER BY count(x) + 1) interact with * the WITH's GROUP BY and must stay on the outer path. (WithOrderBy4 [16].) */ if (has_agg_order) needs = false; - if (needs) { + + /* Push the ORDER BY into the CTE body when either: + * (a) `needs` — it references an input variable the WITH drops, which + * the outer SELECT over the CTE cannot resolve (WithOrderBy2 + * [21]-[24]); this REPLACES the outer ORDER BY; or + * (b) every ORDER BY identifier is a live input variable and the WITH + * has no LIMIT/SKIP — ordering the CTE rows so a DOWNSTREAM + * aggregating WITH (`... ORDER BY value WITH collect(x)`) sees + * sorted input. SQLite preserves an ordered subquery's row order + * through json_group_array. The outer ORDER BY is still emitted + * for final-output ordering. (WithOrderBy1 [45].) */ + bool no_limit = !with->limit && !with->skip; + bool dup_for_downstream = !needs && !has_agg_order && all_live_input && no_limit; + if (needs || dup_for_downstream) { dbuf_append(&cte_body, " ORDER BY "); for (int i = 0; i < with->order_by->count; i++) { cypher_order_by_item *oi = (cypher_order_by_item*)with->order_by->items[i]; @@ -693,7 +761,9 @@ int transform_with_clause(cypher_transform_context *ctx, cypher_with *with) oexpr, odir, oexpr, odir); free(oe); } - order_pushed_to_cte = true; + /* Only the dropped-var case suppresses the outer ORDER BY; the + * downstream-ordering duplicate keeps it for final output. */ + if (needs) order_pushed_to_cte = true; } }