From f7d424c899e4e9ae649dfe0e243a071e2460c9f1 Mon Sep 17 00:00:00 2001 From: Makisuo Date: Fri, 23 Oct 2026 01:54:05 +0200 Subject: [PATCH] fix: parenthesise an arithmetic expression passed as an argument MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `arith` wrote `${lhs} ${op} ${rhs}` with no parentheses. For a chain that is documented: it follows SQL precedence. But a compound argument lost its boundary too — `$.a.sub($.b.add(1))` compiled to `a - b + 1` — and no call order can express `a - (b + 1)` then. A right-hand argument built by `arith` is now parenthesised; chained SQL is unchanged. The fuzz model groups arguments the same way, and FUZZ_STRICT accepts a list of known-issue ids to re-enable just those. Co-Authored-By: Claude Opus 5.5 --- .changeset/arith-argument-parens.md | 5 +++++ docs/expressions.md | 6 ++++++ docs/testing.md | 4 ++-- src/ch/expr.ts | 20 +++++++++++++++++--- src/docs-examples.test.ts | 11 +++++++++++ tests/fuzz/compile.fuzz.test.ts | 14 +++++--------- tests/fuzz/harness.ts | 7 ++++--- tests/fuzz/known-issues.ts | 25 ++++++++----------------- tests/fuzz/literals.fuzz.test.ts | 8 +++----- tests/fuzz/model.ts | 13 +++++++------ tests/fuzz/queries.fuzz.test.ts | 8 +++----- 11 files changed, 71 insertions(+), 50 deletions(-) create mode 100644 .changeset/arith-argument-parens.md diff --git a/.changeset/arith-argument-parens.md b/.changeset/arith-argument-parens.md new file mode 100644 index 0000000..76e0e01 --- /dev/null +++ b/.changeset/arith-argument-parens.md @@ -0,0 +1,5 @@ +--- +"@maple-dev/effect-orm": patch +--- + +An arithmetic expression passed as an argument keeps its parentheses: `$.a.sub($.b.add(1))` compiles to `a - (b + 1)`, not `a - b + 1`. Chains still follow SQL precedence as documented (`$.a.sub(1).div(2)` is `a - 1 / 2`); passing the group as an argument is how to group. diff --git a/docs/expressions.md b/docs/expressions.md index 25a7fb2..c859b4c 100644 --- a/docs/expressions.md +++ b/docs/expressions.md @@ -164,6 +164,12 @@ decodes that `null` as `NaN`. `sum`, `sumIf`, and `toFloat64OrZero` behave the s _(Backed by `docs/expressions.md > Arithmetic does not parenthesise`.)_ +An expression passed as an *argument* keeps its boundary, so that is how to group: +`$.DurationMs.sub($.Offset.add(1))` is `DurationMs - (Offset + 1)`, and +`CH.lit(0.5).mul($.DurationMs.sub(1))` is `0.5 * (DurationMs - 1)`. + +_(Backed by `docs/expressions.md > An arithmetic argument keeps its parentheses`.)_ + ### Division can produce a NULL `.div()` and `.mod()` decode nullably, whatever their operands are. ClickHouse renders `1 / 0` as diff --git a/docs/testing.md b/docs/testing.md index eb74994..708b12e 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -124,8 +124,8 @@ A failure prints the shrunk input, the SQL it compiled to, the database's reply, Bugs found but not yet fixed live in `tests/fuzz/known-issues.ts`. Each entry recognises the inputs that reach its bug and gives a reason, so the suite stays green while the rest of -the input space keeps being fuzzed. After fixing a bug, delete its entry and run with -`FUZZ_STRICT=1`, which ignores every entry, to confirm nothing else matched it. Differences +the input space keeps being fuzzed. `FUZZ_STRICT=` re-enables one entry's inputs, to check a +fix before deleting the entry; `FUZZ_STRICT=1` re-enables them all. Differences between the databases that are not bugs are encoded in the model instead, each with the difference it represents. diff --git a/src/ch/expr.ts b/src/ch/expr.ts index 8420b42..59a1fb5 100644 --- a/src/ch/expr.ts +++ b/src/ch/expr.ts @@ -391,8 +391,16 @@ const inCond = ( const acceptsNull = (schema: Schema.Codec | undefined): boolean => schema !== undefined && Result.isSuccess(Schema.decodeUnknownResult(schema)(null)) +/** Expressions `arith` built: an infix operator a caller may pass as an argument. */ +const infix = new WeakSet() + /** Numeric promotion and SQL NULL propagation share one runtime codec. The - * result type is the caller's claim — see {@link Quotient} for `/` and `%`. */ + * result type is the caller's claim — see {@link Quotient} for `/` and `%`. + * + * The receiver is not parenthesised: a chain follows SQL precedence, as + * docs/expressions.md says. An argument is, when it is itself arithmetic — + * `$.a.sub($.b.add(1))` means `a - (b + 1)`, and no call order could say so + * if the argument's boundary were dropped. */ const arith = ( lhs: SqlFragment, op: string, @@ -411,10 +419,16 @@ const arith = ( acceptsNull(rhsSchema) // `+`, `-`, `*` can overflow a Float64 to `inf`, sent as JSON null: NaN. const overflows = op === "+" || op === "-" || op === "*" - return makeExpr( - known(() => `${compile(lhs)} ${op} ${compile(toFragment(rhs))}`), + const grouped = typeof rhs === "object" && rhs !== null && infix.has(rhs) + const expr = makeExpr( + known(() => { + const right = compile(toFragment(rhs)) + return `${compile(lhs)} ${op} ${grouped ? `(${right})` : right}` + }), (nullable ? Schema.NullOr(CHNumber) : overflows ? CHFloatResult : CHNumber) as Schema.Codec, ) as Expr + infix.add(expr) + return expr } /** diff --git a/src/docs-examples.test.ts b/src/docs-examples.test.ts index 29760b0..ef673be 100644 --- a/src/docs-examples.test.ts +++ b/src/docs-examples.test.ts @@ -302,6 +302,17 @@ describe("docs/expressions.md", () => { expect(oneLine(compileCHUnsafe(query, {}).sql)).toContain("DurationMs - 1 / 2 AS ratio") }) + it("An arithmetic argument keeps its parentheses", () => { + const query = CH.from(Events).select(($) => ({ + left: $.DurationMs.sub($.DurationMs.add(1)), + half: CH.lit(0.5).mul($.DurationMs.sub(1)), + })) + + const sql = oneLine(compileCHUnsafe(query, {}).sql) + expect(sql).toContain("events.DurationMs - (events.DurationMs + 1) AS left") + expect(sql).toContain("0.5 * (events.DurationMs - 1) AS half") + }) + it("Combining conditions with and/or", () => { const query = CH.from(Events) .select(($) => ({ name: $.Name })) diff --git a/tests/fuzz/compile.fuzz.test.ts b/tests/fuzz/compile.fuzz.test.ts index a033c79..c4edeed 100644 --- a/tests/fuzz/compile.fuzz.test.ts +++ b/tests/fuzz/compile.fuzz.test.ts @@ -16,7 +16,7 @@ import * as PG from "@maple-dev/effect-orm/postgres" import type { CoreContext } from "../core-cases" import { scopeContexts as contexts } from "./contexts" import { compilePlan } from "./build" -import { config, FuzzFailure, property, skipKnown } from "./harness" +import { FuzzFailure, property, skipKnown } from "./harness" import { knownHostileIssue, knownPlanIssue } from "./known-issues" import { nastyOptions, ORDER_COLUMNS, plan, type OrderColumn, type Plan } from "./plan" @@ -26,10 +26,8 @@ const stringsIn = (value: unknown): Array => const serialize = (value: unknown) => JSON.stringify(value, (_, v) => (typeof v === "bigint" ? `${v}n` : v instanceof Date ? `Date(${v.getTime()})` : v)) const compiles = (ctx: CoreContext, input: Plan) => { - if (!config.strict) { - const known = knownPlanIssue({ target: ctx.dialect, plan: input }) - if (known !== undefined) skipKnown(known) - } + const known = knownPlanIssue({ target: ctx.dialect, plan: input }) + if (known !== undefined) skipKnown(known) const first = compilePlan(ctx, input) if (first._tag !== "Compiled") throw new FuzzFailure(`a valid plan was not compiled (${first._tag})`, { error: first.error }) const { compiled } = first @@ -113,10 +111,8 @@ const hostileInput = (dialect: "clickhouse" | "postgres"): fc.Arbitrary { - if (!config.strict) { - const known = knownHostileIssue(input) - if (known !== undefined) skipKnown(known) - } + const known = knownHostileIssue(input) + if (known !== undefined) skipKnown(known) let exit: Exit.Exit try { const { query, params } = hostileQuery(ctx, input) diff --git a/tests/fuzz/harness.ts b/tests/fuzz/harness.ts index 5ea3b7e..d213753 100644 --- a/tests/fuzz/harness.ts +++ b/tests/fuzz/harness.ts @@ -9,8 +9,8 @@ // command that replays it (`FUZZ_SEED`, `FUZZ_PATH`); // - inputs pinned in `regressions.ts` are replayed before any random one, so // a counterexample stays a test after the bug is fixed; -// - `known` issues are skipped by name (see `known-issues.ts`), and -// `FUZZ_STRICT=1` turns them back on to check whether one is fixed. +// - `known` issues are skipped by name (see `known-issues.ts`); +// `FUZZ_STRICT=1` turns them all back on, `FUZZ_STRICT=,` just those. import fc from "fast-check" import { pinnedFor } from "./regressions" @@ -25,7 +25,8 @@ export const config = { runs: int("FUZZ_RUNS"), seed: int("FUZZ_SEED"), path: process.env.FUZZ_PATH || undefined, - strict: process.env.FUZZ_STRICT === "1", + /** `1`: ignore every known issue. A comma list of ids: ignore just those (to verify one fix). */ + strict: new Set((process.env.FUZZ_STRICT ?? "").split(",").filter(Boolean)), verbose: process.env.FUZZ_VERBOSE === "1", } diff --git a/tests/fuzz/known-issues.ts b/tests/fuzz/known-issues.ts index ec5a21b..d969158 100644 --- a/tests/fuzz/known-issues.ts +++ b/tests/fuzz/known-issues.ts @@ -4,6 +4,7 @@ // while it is open and the rest of the input space keeps being fuzzed. This file // is the backlog: fix the bug, delete its entry, and run with `FUZZ_STRICT=1` // to confirm nothing else matched it. An entry needs a reason a person can act on. +import { config } from "./harness" import type { LiteralType } from "./values" export interface KnownIssue { @@ -12,6 +13,10 @@ export interface KnownIssue { readonly matches: (input: A) => boolean } +/** The first open issue `input` reaches, unless FUZZ_STRICT re-enabled it. */ +const firstMatch = (issues: ReadonlyArray>, input: A): string | undefined => + issues.find((issue) => !config.strict.has("1") && !config.strict.has(issue.id) && issue.matches(input))?.id + export type LiteralPath = "compare" | "select" export interface LiteralInput { readonly literal: LiteralType @@ -47,7 +52,7 @@ const literalIssues: ReadonlyArray> = [ /** The id of the open issue this input would hit, if any. */ export const knownLiteralIssue = (input: LiteralInput): string | undefined => - literalIssues.find((issue) => issue.matches(input))?.id + firstMatch(literalIssues, input) // Hostile values (compile.fuzz.test.ts) @@ -101,7 +106,7 @@ const hostileIssues: ReadonlyArray> = [ ] export const knownHostileIssue = (input: HostileInput): string | undefined => - hostileIssues.find((issue) => issue.matches(input))?.id + firstMatch(hostileIssues, input) // Query plans (queries.fuzz.test.ts) @@ -121,8 +126,6 @@ const nodes = function* (value: unknown): Generator> { for (const item of Object.values(value)) yield* nodes(item) } } -const precedence = (op: unknown) => (op === "add" || op === "sub" ? 1 : 2) -const isArith = (e: unknown): e is { op: string; l: unknown; r: unknown } => (e as any)?.k === "arith" /** The conjuncts a WHERE list ANDs together, flattening nested ANDs. */ const conjuncts = (conds: ReadonlyArray): Array => @@ -160,18 +163,6 @@ const planIssues: ReadonlyArray> = [ target === "postgres" && [...nodes(plan)].some((node) => node.k === "value" && typeof node.v === "number" && !Number.isInteger(node.v)), }, - { - id: "arith-argument-not-parenthesised", - reason: - "A compound expression passed as an arithmetic argument loses its boundary: `$.a.sub($.b.add(1))` compiles to " + - "`a - b + 1`, `$.a.div($.b.mul(2))` to `a / b * 2`. docs/expressions.md documents that *chains* follow SQL " + - "precedence (the model applies that), but an argument is not a chain, and no call order expresses " + - "`a - (b + 1)`. Parenthesising a compound right operand would fix it without changing chained SQL.", - matches: ({ plan }) => - [...nodes(plan)].some( - (node) => isArith(node) && isArith(node.r) && precedence(node.r.op) <= precedence(node.op), - ), - }, { id: "fromquery-select-all", reason: @@ -181,4 +172,4 @@ const planIssues: ReadonlyArray> = [ }, ] -export const knownPlanIssue = (input: PlanInput): string | undefined => planIssues.find((issue) => issue.matches(input))?.id +export const knownPlanIssue = (input: PlanInput): string | undefined => firstMatch(planIssues, input) diff --git a/tests/fuzz/literals.fuzz.test.ts b/tests/fuzz/literals.fuzz.test.ts index 74ece2a..791a44f 100644 --- a/tests/fuzz/literals.fuzz.test.ts +++ b/tests/fuzz/literals.fuzz.test.ts @@ -18,7 +18,7 @@ import { Effect, Exit } from "effect" import * as CH from "@maple-dev/effect-orm/clickhouse" import * as PG from "@maple-dev/effect-orm/postgres" import { clickhouseEndpoint, closePostgres, run } from "./db" -import { config, FuzzFailure, property, skip, skipKnown, withContext } from "./harness" +import { FuzzFailure, property, skip, skipKnown, withContext } from "./harness" import { knownLiteralIssue, type LiteralPath } from "./known-issues" import { literalTypes, normalize, referenceJson, wellFormed, type LiteralType } from "./values" @@ -35,10 +35,8 @@ const build = (literal: LiteralType, value: unknown, path: LiteralPath) => { const roundTrip = async (literal: LiteralType, value: unknown, path: LiteralPath): Promise => { // The reference row goes through JSON, which cannot carry a lone surrogate the way SQL text does. if (!wellFormed(value)) skip("lone surrogate") - if (!config.strict) { - const known = knownLiteralIssue({ literal, value, path }) - if (known !== undefined) skipKnown(known) - } + const known = knownLiteralIssue({ literal, value, path }) + if (known !== undefined) skipKnown(known) const M = literal.dialect === "clickhouse" ? CH : PG const exit = Effect.runSyncExit((M.compile as any)(build(literal, value, path), { p: value })) if (Exit.isFailure(exit)) throw new FuzzFailure("the builder refused a valid value", { value, cause: String(exit.cause) }) diff --git a/tests/fuzz/model.ts b/tests/fuzz/model.ts index 2d11825..2f7f305 100644 --- a/tests/fuzz/model.ts +++ b/tests/fuzz/model.ts @@ -90,15 +90,16 @@ const chKind = (e: Expr | number): ChKind => { // Expressions /** - * Arithmetic as the SQL text parses it. The builder writes `${lhs} ${op} ${rhs}` - * without parentheses — documented in docs/expressions.md ("Arithmetic does not - * parenthesise"): a chain follows SQL precedence, not call order. So the model - * flattens a tree to the token sequence the SQL contains and re-parses it with - * `*` `/` `%` binding tighter than `+` `-`, both left-associative. + * Arithmetic as the SQL text parses it. A chain is not parenthesised — + * documented in docs/expressions.md ("Arithmetic does not parenthesise"), it + * follows SQL precedence, not call order — while an argument keeps its own + * parentheses. So the model flattens the receiver chain to the token sequence + * the SQL contains, keeps each argument whole, and re-parses with `*` `/` `%` + * binding tighter than `+` `-`, both left-associative. */ const parsed = new WeakSet() const flatten = (e: Expr): Array => - e.k === "arith" ? [...flatten(e.l), e.op, ...(typeof e.r === "number" ? [{ k: "num", v: e.r } as Expr] : flatten(e.r))] : [e] + e.k === "arith" ? [...flatten(e.l), e.op, typeof e.r === "number" ? ({ k: "num", v: e.r } as Expr) : e.r] : [e] const asParsed = (e: Expr & { k: "arith" }): Expr => { const tokens = flatten(e) const node = (op: ArithOp, l: Expr, r: Expr): Expr => { diff --git a/tests/fuzz/queries.fuzz.test.ts b/tests/fuzz/queries.fuzz.test.ts index 41a5ca5..eee2907 100644 --- a/tests/fuzz/queries.fuzz.test.ts +++ b/tests/fuzz/queries.fuzz.test.ts @@ -12,7 +12,7 @@ import type { CoreContext } from "../core-cases" import { contexts, scopeContexts } from "./contexts" import { compilePlan } from "./build" import { clickhouseEndpoint, closePostgres, run, type Target } from "./db" -import { config, FuzzFailure, property, skipKnown, withContext } from "./harness" +import { FuzzFailure, property, skipKnown, withContext } from "./harness" import { knownPlanIssue } from "./known-issues" import { evaluate } from "./model" import { nastyOptions, plainOptions, plan, type Plan } from "./plan" @@ -30,10 +30,8 @@ const canonical = (rows: ReadonlyArray): Array => const sorted = (rows: Array) => rows.map((r) => JSON.stringify(r)).sort() const matchesModel = async (ctx: CoreContext, target: Target, input: Plan) => { - if (!config.strict) { - const known = knownPlanIssue({ target, plan: input }) - if (known !== undefined) skipKnown(known) - } + const known = knownPlanIssue({ target, plan: input }) + if (known !== undefined) skipKnown(known) const expected = evaluate(input, target) const result = compilePlan(ctx, input) if (result._tag !== "Compiled") throw new FuzzFailure(`a valid plan was not compiled (${result._tag})`, { error: result.error })