diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c9bd80..1fcaa5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,19 @@ Notable changes to `@tinbase/pg-mem`, the tinbase fork of pg-mem. Released from `main`, which carries the scoped package name. Upstream is tracked through the `upstream` remote (`oguimbal/pg-mem`) rather than a branch; the leftover `master` is vestigial. +## 4.0.2 + +Gaps found by running 80 production projects' migrations and seeds through the agent's validator on pg-mem and on PGlite; all 80 now match. Requires `@tinbase/pgsql-ast-parser` 12.2.1. + +- **Wrong results, fixed:** an aggregate inside a select-list subquery aggregated the *outer* query (`select id, (select count(*) from c where c.p = p.id) from p` returned one row); `agg(x ORDER BY y)` ignored its ORDER BY; `json_agg` / `jsonb_agg` dropped NULL inputs (postgres keeps them as json null). +- `f(unnest(arr))` calls `f` per element (`lower(unnest(emails))` resolved to the range overload); `RETURNS SETOF ` SQL functions. +- A set-returning call in a subquery's FROM reads the outer row (`from jsonb_array_elements(outer.col)`). +- `UPDATE t alias` / `DELETE FROM t alias`: the alias names the table, in the statement and its subqueries. +- plpgsql: a `FOR rec IN ` record's fields are usable in SQL statements inside the loop (`update … set x = rec.col`), including nested loops. +- `json -> key` / `->>` with a column, variable or expression key. +- `ALTER COLUMN … TYPE` and `RENAME COLUMN` keep the column's position. +- `INSERT … VALUES` checks each row against the target columns first ("INSERT has more expressions than target columns"). + ## 4.0.1 - Comparisons across type categories fail with Postgres' own error, `operator does not exist: uuid = text` (was `cannot cast type text to uuid`), types spelled as Postgres spells them. Also for `x IN (select col ...)`. The SQL that is rejected does not change. diff --git a/package.json b/package.json index 16ed6af..ad12880 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@tinbase/pg-mem", - "version": "4.0.1", + "version": "4.0.2", "description": "Fork of pg-mem with extended Postgres conformance (PL/pgSQL, triggers, RLS, correlated subqueries, MERGE, ranges, full-text, partitioning, ...). Tracks oguimbal/pg-mem; pending upstream PR #476.", "main": "index.js", "publishConfig": { @@ -49,7 +49,7 @@ "immutable": "^4.3.4", "lru-cache": "^6.0.0", "object-hash": "^2.0.3", - "pgsql-ast-parser": "npm:@tinbase/pgsql-ast-parser@^12.2.0" + "pgsql-ast-parser": "npm:@tinbase/pgsql-ast-parser@^12.2.1" }, "devDependencies": { "@babel/cli": "^7.10.5", diff --git a/src/column.ts b/src/column.ts index 822727f..1b0a62a 100644 --- a/src/column.ts +++ b/src/column.ts @@ -221,9 +221,8 @@ export class ColRef implements _Column { const nn = newId; this.expression = columnEvaluator(this.table, newId, newType); - // replace in table - this.table.columnMgr.delete(on); - this.table.columnMgr.set(nn, this); + // replace in table, keeping the column's position (postgres never moves a column) + this.table.columnMgr.replace(on, nn, this); } drop(t: _Transaction): void { diff --git a/src/execution/plpgsql.ts b/src/execution/plpgsql.ts index 6e20af9..4f30e9c 100644 --- a/src/execution/plpgsql.ts +++ b/src/execution/plpgsql.ts @@ -541,6 +541,10 @@ interface Helpers { prepareSql(sql: string): (ctx: RunCtx) => any; /** run a dynamic (runtime-built) SQL string */ runDynamic(ctx: RunCtx, sqlText: string): any; + /** the same toolbox with extra variables in scope for embedded SQL */ + withVars(extra: { name: string; value: IValue }[]): Helpers; + /** rewrites embedded SQL for the enclosing FOR-loop records (rec.col -> variable) */ + rewriteSql?: (sql: string) => string; } /** does this block (recursively) use RETURN NEXT / RETURN QUERY? => set-returning */ @@ -610,6 +614,7 @@ function makeHelpers(schema: _ISchema, params: Parameter[], returns: _IType | ni }; const outColumns = ((returns as any)?.of?.columns as { name: string }[] | undefined) ?? null; return { + withVars: (extra) => makeHelpers(schema, [...params, ...extra.map((x, i) => ({ index: params.length + i, value: x.value }))], returns, setof), returns, setof, compileExpr, @@ -864,6 +869,16 @@ function expandRowTypes(toks: string[], schema: _ISchema): string[] { return out; } +/** `rec.col` -> fieldVar(col) in SQL text, outside string literals and quoted identifiers, for known columns */ +function rewriteRecordFields(sql: string, rec: string, known: Set, fieldVar: (col: string) => string): string { + const re = new RegExp(`(^|[^\\w$."])${rec.replace(/[^\w]/g, '')}\\s*\\.\\s*([a-zA-Z_][\\w$]*)`, 'gi'); + // split on '…' and "…" so literals and quoted identifiers are left alone + return sql.split(/('(?:[^']|'')*'|"(?:[^"]|"")*")/).map((part, i) => i % 2 + ? part + : part.replace(re, (m, pre, col) => known.has(col.toLowerCase()) ? pre + fieldVar(col.toLowerCase()) : m) + ).join(''); +} + function parseTypeDef(typeSrc: string): any { const e = parseExpr(`null::${typeSrc}`); if (e.type !== 'cast') { @@ -1007,7 +1022,27 @@ function compileBody(stmts: GStmt[], h: Helpers): GCompiled[] { // (aliased) selection; the current row is fed to the body's expressions const sel = h.querySelection(s.sql).setAlias(s.varName); const holder: { row: any } = { row: DUMMY_ROW }; - const body = compileBody(s.body, { ...h, compileExpr: h.mkCompiler(sel, () => holder.row) }); + // embedded SQL in the body (`update … set x = rec.col`) sees `rec.col` as a table-qualified + // column, not a variable: expose each field as a variable and rewrite the references to it + const rec = s.varName.toLowerCase(); + const fields = sel.columns.filter(c => !!c.id); + const fieldVar = (col: string) => `__rec_${rec}__${col}`; + const withFields = h.withVars(fields.map(c => ({ + name: fieldVar(c.id!), + value: new Evaluator(c.type, fieldVar(c.id!), `plpgsql_rec_${rec}_${c.id}`, [], + (_raw, t) => c.get(holder.row, t), { forceNotConstant: true }), + }))); + const known = new Set(fields.map(c => c.id!.toLowerCase())); + // enclosing loops' records first, so a nested loop still sees them + const rewrite = (sql: string) => rewriteRecordFields(h.rewriteSql ? h.rewriteSql(sql) : sql, rec, known, fieldVar); + const body = compileBody(s.body, { + ...withFields, + compileExpr: h.mkCompiler(sel, () => holder.row), + mkCompiler: withFields.mkCompiler, + prepareSql: (sql) => withFields.prepareSql(rewrite(sql)), + querySelection: (sql) => withFields.querySelection(rewrite(sql)), + rewriteSql: rewrite, + }); return { run(ctx) { for (const row of sel.enumerate(ctx.t)) { diff --git a/src/execution/records-mutations/deletion.ts b/src/execution/records-mutations/deletion.ts index 45c05a6..b5fb5d0 100644 --- a/src/execution/records-mutations/deletion.ts +++ b/src/execution/records-mutations/deletion.ts @@ -12,7 +12,7 @@ export class Deletion extends MutationDataSourceBase { const { schema } = buildCtx(); const table = asTable(schema.getObject(ast.from)); // row-level security: DELETE can only affect rows visible via DELETE policies - const mutatedSel = applyReadRls(table, table.selection, 'delete', statementReadsColumns(ast as any)) + const mutatedSel = applyReadRls(table, ast.from.alias ? table.selection.setAlias(ast.from.alias) : table.selection, 'delete', statementReadsColumns(ast as any)) .filter(ast.where); super(table, mutatedSel, ast); diff --git a/src/execution/records-mutations/insert.ts b/src/execution/records-mutations/insert.ts index 7bc9611..abddf32 100644 --- a/src/execution/records-mutations/insert.ts +++ b/src/execution/records-mutations/insert.ts @@ -30,6 +30,21 @@ export class Insert extends MutationDataSourceBase { // init super super(table, selection, ast); + // postgres checks each VALUES row against the target columns before the rows against + // each other, so a seed row with one value too many says so (not "VALUES lists must + // all be the same length") + if (ast.insert.type === 'values') { + const target = ast.columns?.length ?? table.selection.columns.length; + for (const row of ast.insert.values) { + if (row.length > target) { + throw new QueryError('INSERT has more expressions than target columns', '42601'); + } + if (ast.columns && row.length < target) { + throw new QueryError('INSERT has more target columns than expressions', '42601'); + } + } + } + // get data to insert this.valueRawSource = ast.insert.type === 'values' ? buildValues(ast.insert, true) diff --git a/src/execution/records-mutations/update.ts b/src/execution/records-mutations/update.ts index 6394a60..bf37250 100644 --- a/src/execution/records-mutations/update.ts +++ b/src/execution/records-mutations/update.ts @@ -63,7 +63,9 @@ export class Update extends MutationDataSourceBase { // => REGULAR UPDATE // row-level security: UPDATE can only affect rows visible via UPDATE policies - mutatedSel = applyReadRls(into, into.selection, 'update', statementReadsColumns(ast as any)) + // `update t alias set ... where alias.col ...`: the alias names the table, as in FROM + const base = ast.table.alias ? into.selection.setAlias(ast.table.alias) : into.selection; + mutatedSel = applyReadRls(into, base, 'update', statementReadsColumns(ast as any)) .filter(ast.where); } diff --git a/src/execution/schema-amends/create-function.ts b/src/execution/schema-amends/create-function.ts index b66a4a3..d158b9b 100644 --- a/src/execution/schema-amends/create-function.ts +++ b/src/execution/schema-amends/create-function.ts @@ -94,6 +94,7 @@ export class CreateFunction extends ExecHelper implements _IStatementExecutor { args, code: fn.code, returns, + setof: !!fn.setof, functionName: fn.name.name, schema: schema, }); diff --git a/src/execution/select.ts b/src/execution/select.ts index a3c8ef8..67028c2 100644 --- a/src/execution/select.ts +++ b/src/execution/select.ts @@ -160,7 +160,10 @@ function buildRawSelectSubject(p: SelectFromStatement): _ISelection | nil { const fnName = from.alias?.name ?? from.function?.name; // built against the left FROM items: function calls referencing them // are implicitly lateral in postgres - const fromValue: IValue = sel ? withSelection(sel, () => buildValue(from)) : buildValue(from); + // a first FROM item reads no row of its own: build it against the empty selection, + // so `from f(outer.col)` in a subquery resolves outer.col as a correlated reference + // rather than as a column of whatever selection happens to be current + const fromValue: IValue = withSelection(sel ?? buildCtx().schema.dualTable.selection, () => buildValue(from)); if ((from.lateral || !fromValue.isConstant) && sel) { if (!ArrayType.matches(fromValue.type) || RecordType.matches(fromValue.type.of)) { throw new NotSupported('lateral function calls returning records'); diff --git a/src/interfaces.ts b/src/interfaces.ts index f8d0a62..400b350 100644 --- a/src/interfaces.ts +++ b/src/interfaces.ts @@ -195,6 +195,8 @@ export interface ToCompile { args: ArgDefDetails[]; /** Expected return type (if any) */ returns?: IType | nil; + /** RETURNS SETOF : `returns` is then that type's array, one element per row */ + setof?: boolean; } export class AdvancedResult { diff --git a/src/parser/function-call.ts b/src/parser/function-call.ts index 28b709f..cf31e7c 100644 --- a/src/parser/function-call.ts +++ b/src/parser/function-call.ts @@ -7,10 +7,17 @@ import hash from 'object-hash'; import { parseArrayLiteral, QName } from 'pgsql-ast-parser'; import { asSingleQName, nullIsh, qnameToStr } from '../utils'; import { buildCtx } from './context'; -import { markSetReturning } from '../transforms/expand-srf'; +import { markSetReturning, isSetReturning } from '../transforms/expand-srf'; export function buildCall(name: string | QName, args: IValue[]): IValue { + // f(unnest(arr)): postgres calls f once per element and the result is itself set-returning + // (`select lower(unnest(emails))`). Applying f to the whole array instead resolved + // lower(text[]) to the range overload. + const srfAt = args.findIndex(a => isSetReturning(a) && a.type instanceof ArrayType); + if (srfAt >= 0) { + return buildCallPerElement(name, args, srfAt); + } let type: _IType | nil = null; let get: (...args: any[]) => any; @@ -312,6 +319,30 @@ export function buildCall(name: string | QName, args: IValue[]): IValue { } +function buildCallPerElement(name: string | QName, args: IValue[], srfAt: number): IValue { + const srf = args[srfAt]; + let current: any; + // stands for "the element being processed"; impure so it is never folded into a constant + const element = new Evaluator((srf.type as ArrayType).of, null, hash({ srfElement: srf.hash }), srf, () => current, { unpure: true }); + const perElement = buildCall(name, args.map((a, i) => i === srfAt ? element : a)); + const ret = new Evaluator( + perElement.type.asArray() + , null + , hash({ call: name, perElement: perElement.hash }) + , args + , (raw, t) => { + const arr = srf.get(raw, t); + if (nullIsh(arr)) { + return null; + } + return (arr as any[]).map(x => { + current = x; + return perElement.get(raw, t); + }); + }, { unpure: true }); + return markSetReturning(ret); +} + function expectArgs(name: string | QName, args: IValue[], count: number | [number, number]) { const [min, max] = typeof count === 'number' ? [count, count] : count; if (args.length < min || args.length > max) { diff --git a/src/schema/function-call-table.ts b/src/schema/function-call-table.ts index 98703e8..0281090 100644 --- a/src/schema/function-call-table.ts +++ b/src/schema/function-call-table.ts @@ -25,7 +25,12 @@ export class FunctionCallTable extends DataSourceBase { } enumerate(t: _Transaction): Iterable { - const results = this.evaluator.get(null, t); + // nothing precedes this FROM item, so the arguments read no row: they are constants, or + // outer-query references in a correlated subquery (`from jsonb_array_elements(outer.col)`), + // which are bound per outer row before this runs - evaluated now, not as constants + const results = this.evaluator.isConstant + ? this.evaluator.get() + : this.evaluator.get({}, t); for (const result of results ?? []) { result[this.symbol] = true; } @@ -50,10 +55,14 @@ export class FunctionCallTable extends DataSourceBase { explain(e: _Explainer): _SelectExplanation { - throw new Error('Method not implemented.'); + return { + id: e.idFor(this), + _: 'table', + table: 'function call', + } as any; } stats(t: _Transaction): Stats | null { - throw new Error('Method not implemented.'); + return null; } } diff --git a/src/schema/pg-catalog/binary-operators.ts b/src/schema/pg-catalog/binary-operators.ts index a7c4f5b..c3f1ba2 100644 --- a/src/schema/pg-catalog/binary-operators.ts +++ b/src/schema/pg-catalog/binary-operators.ts @@ -258,6 +258,33 @@ function jsonbHasKey(doc: any, key: string): boolean { } function registerJsonOperators(schema: _ISchema) { + // ======= "json -> key" / "json ->> key" with a non-literal key (a column, a variable, + // an expression); a literal key is compiled as a member access instead, same semantics + for (const j of [Types.json, Types.jsonb]) { + for (const [key, pick] of [ + [Types.text(), (doc: any, k: string) => doc && typeof doc === 'object' && !Array.isArray(doc) ? doc[k] : undefined], + [Types.integer, (doc: any, i: number) => Array.isArray(doc) ? doc[i < 0 ? doc.length + i : i] : undefined], + ] as const) { + schema.registerOperator({ + operator: '->', + left: j, + right: key, + returns: j, + implementation: (doc: any, k: any) => (pick as any)(doc, k) ?? null, + }); + schema.registerOperator({ + operator: '->>', + left: j, + right: key, + returns: Types.text(), + implementation: (doc: any, k: any) => { + const v = (pick as any)(doc, k); + return v === undefined || v === null ? null : typeof v === 'string' ? v : JSON.stringify(v); + }, + }); + } + } + // ======= "jsonb ? text" / "?|" (any) / "?&" (all) key existence schema.registerOperator({ operator: '?', diff --git a/src/schema/pg-catalog/sql-function-language.ts b/src/schema/pg-catalog/sql-function-language.ts index 4f03532..317cfa9 100644 --- a/src/schema/pg-catalog/sql-function-language.ts +++ b/src/schema/pg-catalog/sql-function-language.ts @@ -13,7 +13,7 @@ import { RecordType } from '../../datatypes/t-record'; let execId = 0; export function registerSqlFunctionLanguage(db: _IDb) { - db.registerLanguage('sql', ({ code, schema: _schema, args, returns: _returns }) => { + db.registerLanguage('sql', ({ code, schema: _schema, args, returns: _returns, setof }) => { const schema = _schema as _ISchema; const returns = _returns as _IType; // parse SQL @@ -69,6 +69,14 @@ export function registerSqlFunctionLanguage(db: _IDb) { transformResult = (v, t, eid) => v?.map(x => { return transformItem(x, t, eid); }); + } else if (setof && ArrayType.matches(returns)) { + // RETURNS SETOF : one value per row + const cols = executor.selection.columns; + if (cols.length !== 1 || !cols[0].type.canConvertImplicit(returns.of)) { + throw new QueryError(`return type mismatch in function declared to return ${returns.of.name}`, '42P13'); + } + const col = cols[0].cast(returns.of); + transformResult = (v, t) => (v ?? []).map(x => col.get(x, t)); } else { // returns a single value const cols = executor.selection.columns; diff --git a/src/table.ts b/src/table.ts index f5efcb2..fadc3ca 100644 --- a/src/table.ts +++ b/src/table.ts @@ -61,6 +61,20 @@ class ColumnManager { this.invalidateColumns(); return this.map.delete(name); } + + /** Puts `colDef` under `newName` where `oldName` was, keeping the column's position (rename, retype) */ + replace(oldName: string, newName: string, colDef: ColRef) { + this.invalidateColumns(); + const entries = Array.from(this.map.entries()); + this.map.clear(); + for (const [k, v] of entries) { + if (k === oldName) { + this.map.set(newName, colDef); + } else { + this.map.set(k, v); + } + } + } } export class MemoryTable extends DataSourceBase implements IMemoryTable, _ITable { diff --git a/src/tests/corpus-parity.spec.ts b/src/tests/corpus-parity.spec.ts index b24dcb5..2c49158 100644 --- a/src/tests/corpus-parity.spec.ts +++ b/src/tests/corpus-parity.spec.ts @@ -548,6 +548,126 @@ describe('corpus parity', () => { }); }); + describe('set-returning calls inside other calls', () => { + // from a production migration: a lookup by emails, `email = any (select lower(unnest(_emails)))` + it('applies the outer function to each element', () => { + expect(many(`select lower(unnest(array['A', 'b'])) as v`)).toEqual([{ v: 'a' }, { v: 'b' }]); + expect(many(`select upper(lower(unnest(array['Ab', 'cD']))) as v`)).toEqual([{ v: 'AB' }, { v: 'CD' }]); + expect(many(`select length(unnest(array['ab', 'cde'])) as n`)).toEqual([{ n: 2 }, { n: 3 }]); + expect(many(`select lower(unnest(null::text[])) as v`)).toEqual([]); + }); + + it('works in ANY / IN subqueries and SQL functions', () => { + none(`create table u (email text); insert into u values ('a@x.io'), ('B@X.IO'), ('c@x.io'); + create function n(_e text[]) returns int language sql stable as + $$ select count(*)::int from u where lower(email) = any (select lower(unnest(_e))) $$`); + expect(many(`select n(array['b@x.io', 'C@x.io', 'zz']) as n`)).toEqual([{ n: 2 }]); + expect(many(`select count(*)::int as n from u where lower(email) in (select lower(unnest(array['A@X.IO'])))`)).toEqual([{ n: 1 }]); + }); + + it('RETURNS SETOF returns one value per row, and checks the column type', () => { + none(`create function f(_e text[]) returns setof text language sql as $$ select lower(unnest(_e)) $$`); + expect(many(`select * from f(array['A', 'b'])`)).toEqual([{ f: 'a' }, { f: 'b' }]); + expectQueryError(() => none(`create function g() returns setof int language sql as $$ select 'x'::text $$`), /return type mismatch/); + }); + }); + + describe('ALTER COLUMN keeps the column where it is', () => { + // a production migration retyped user_id text -> uuid; generated types then listed it last + it('on retype and rename', () => { + none(`create table v (id int, user_id text, created_at timestamptz default now(), note text); + alter table v alter column user_id type uuid using user_id::uuid; + alter table v rename column note to body`); + expect(many(`select column_name from information_schema.columns where table_name = 'v' order by ordinal_position`).map(r => r.column_name)) + .toEqual(['id', 'user_id', 'created_at', 'body']); + expect(db.public.query(`select * from v`).fields.map(f => f.name)).toEqual(['id', 'user_id', 'created_at', 'body']); + }); + }); + + describe('plpgsql FOR-loop records in embedded SQL', () => { + // from a production data-fix migration: `for m in select … from (values …) as t(old_value, new_value) + // loop update … set x = replace(x, m.old_value, m.new_value) …` + beforeEach(() => none(`create table c (id int, p text, note text); insert into c values (1, '/a.jpg', 'x'), (2, '/b.jpg', 'y')`)); + it('an UPDATE in the loop reads the record\'s fields', () => { + none(`do $$ declare m record; begin + for m in select * from (values ('/a.jpg', '/A.jpg')) as t(old_value, new_value) loop + update c set p = replace(p, m.old_value, m.new_value) where p like '%' || m.old_value || '%'; + end loop; end $$`); + expect(many(`select p from c order by id`)).toEqual([{ p: '/A.jpg' }, { p: '/b.jpg' }]); + }); + it('leaves string literals alone, and nested loops see the outer record', () => { + none(`do $$ declare a record; b record; begin + for a in select id from c loop + for b in select a.id * 10 as big loop + update c set note = 'a.id=' || b.big::text where id = a.id; + end loop; + end loop; end $$`); + expect(many(`select note from c order by id`)).toEqual([{ note: 'a.id=10' }, { note: 'a.id=20' }]); + }); + }); + + describe('aggregates with ORDER BY, and json_agg NULLs', () => { + beforeEach(() => none(`create table t (g int, x int, s text); insert into t values (1, 2, 'b'), (1, 1, 'a'), (1, null, null), (2, 5, 'e'), (2, 4, 'd')`)); + it('feeds rows in the aggregate\'s ORDER BY', () => { + expect(many(`select jsonb_agg(x order by x desc nulls last) as v from t where g = 1`)).toEqual([{ v: [2, 1, null] }]); + expect(many(`select g, string_agg(s, ',' order by s desc) as v from t group by g order by g`)).toEqual([{ g: 1, v: 'b,a' }, { g: 2, v: 'e,d' }]); + expect(many(`select array_agg(x order by s) as v from t where g = 2`)).toEqual([{ v: [4, 5] }]); + expect(many(`select jsonb_agg(x order by x) filter (where x > 1) as v from t`)).toEqual([{ v: [2, 4, 5] }]); + }); + it('json_agg / jsonb_agg keep NULL inputs as json null', () => { + expect(many(`select jsonb_agg(x order by x nulls first) as v from t where g = 1`)).toEqual([{ v: [null, 1, 2] }]); + expect(many(`select json_agg(x) as v from t where x is null`)).toEqual([{ v: [null] }]); + expect(many(`select jsonb_agg(x) as v from t where false`)).toEqual([{ v: null }]); + }); + }); + + describe('json -> / ->> with a non-literal key', () => { + // from a production migration: jsonb_array_elements(tc.credits -> grp), grp a plpgsql variable + it('takes a column, an expression or a variable as the key', () => { + none(`create table d (j jsonb, k text, i int); insert into d values ('{"cast": [{"name": "A"}, {"name": "B"}]}', 'cast', 1), ('[10, 20, 30]', 'x', -1)`); + expect(many(`select j -> k -> 0 ->> 'name' as v from d`)).toEqual([{ v: 'A' }, { v: null }]); + expect(many(`select j -> i as v from d`)).toEqual([{ v: null }, { v: 30 }]); + expect(many(`select jsonb_array_length(j -> (k)) as n from d where jsonb_typeof(j -> k) = 'array'`)).toEqual([{ n: 2 }]); + none(`do $$ declare grp text; begin foreach grp in array array['cast'] loop + update d set k = (select string_agg(e ->> 'name', ',' order by ord) from jsonb_array_elements(d.j -> grp) with ordinality as a(e, ord)) where jsonb_typeof(d.j -> grp) = 'array'; + end loop; end $$`); + expect(many(`select k from d where i = 1`)).toEqual([{ k: 'A,B' }]); + }); + }); + + describe('correlated set-returning calls in a subquery FROM, and UPDATE/DELETE aliases', () => { + // from a production migration: update public.titles_cache tc set credits = jsonb_set(tc.credits, …, + // (select … from jsonb_array_elements(tc.credits -> grp) with ordinality as a(e, ord))) + beforeEach(() => none(`create table d (id int, j jsonb, k text); insert into d values (1, '{"c": [{"n": "A"}, {"n": "B"}]}', null), (2, '{"c": [{"n": "C"}]}', null)`)); + it('reads the outer row', () => { + expect(many(`select id, (select string_agg(e ->> 'n', ',') from jsonb_array_elements(d.j -> 'c') as e) as n from d order by id`)) + .toEqual([{ id: 1, n: 'A,B' }, { id: 2, n: 'C' }]); + expect(many(`select id from d where exists (select 1 from jsonb_array_elements(d.j -> 'c') as e where e ->> 'n' = 'C')`)).toEqual([{ id: 2 }]); + }); + it('UPDATE and DELETE accept a table alias, in the statement and its subqueries', () => { + none(`update d tc set k = (select string_agg(e ->> 'n', ',' order by ord) from jsonb_array_elements(tc.j -> 'c') with ordinality as a(e, ord)) where tc.id = 1`); + expect(many(`select k from d order by id`)).toEqual([{ k: 'A,B' }, { k: null }]); + none(`delete from d x where x.k is null`); + expect(many(`select id from d`)).toEqual([{ id: 1 }]); + }); + it('an aggregate in a select-list subquery does not aggregate the outer query', () => { + none(`create table c (p int); insert into c values (1), (1), (2)`); + expect(many(`select id, (select count(*) from c where c.p = d.id) as n from d order by id`)).toEqual([{ id: 1, n: 2 }, { id: 2, n: 1 }]); + expect(many(`select count(*) as n, (select count(*) from c) as m from d`)).toEqual([{ n: 2, m: 3 }]); + }); + }); + + describe('INSERT … VALUES row length', () => { + // a production seed had a row with one value too many; postgres names that, per row + it('checks each row against the target columns first', () => { + none(`create table t (a int, b int, c int default 3)`); + expectQueryError(() => none(`insert into t (a, b) values (1, 2), (1, 2, 3)`), /INSERT has more expressions than target columns/); + expectQueryError(() => none(`insert into t (a, b) values (1, 2), (1)`), /INSERT has more target columns than expressions/); + expectQueryError(() => none(`insert into t values (1), (1, 2)`), /VALUES lists must all be the same length/); + none(`insert into t values (1, 2)`); + }); + }); + describe('CREATE OR REPLACE TRIGGER', () => { it('replaces an existing trigger', () => { none(`create table o (id int, n int); diff --git a/src/transforms/aggregation.ts b/src/transforms/aggregation.ts index 786b307..671c880 100644 --- a/src/transforms/aggregation.ts +++ b/src/transforms/aggregation.ts @@ -6,6 +6,7 @@ import { Types } from '../datatypes'; import { nil, NotSupported } from '../interfaces'; import hash from 'object-hash'; import { Evaluator } from '../evaluator'; +import { buildSortKeys, compareRows, SortKey } from './order-by'; import { buildCount } from './aggregations/count'; import { buildMinMax } from './aggregations/max-min'; import { buildSum } from './aggregations/sum'; @@ -75,6 +76,8 @@ interface AggregationInstance { distinct: IValue[] | nil; /** optional `FILTER (WHERE ...)` predicate: rows where it is not true are skipped */ filter: IValue | nil; + /** `agg(x ORDER BY ...)`: rows are fed in this order */ + orderBy: SortKey[] | nil; } function isIntegralType(value: any): boolean { @@ -229,6 +232,8 @@ export class Aggregation extends TransformBase implements _ISelection, _IAggrega computer: AggregationGroupComputer; distinctHash: Set; instance: AggregationInstance, + /** rows held back for an aggregate with ORDER BY */ + ordered?: Row[]; }[]; }>(); @@ -278,9 +283,25 @@ export class Aggregation extends TransformBase implements _ISelection, _IAggrega } g.distinctHash.add(valuesHash); } + if (g.instance.orderBy) { + // fed once the group is complete, in ORDER BY order + (g.ordered ??= []).push(item); + continue; + } g.computer.feedItem(item); } } + for (const group of groups.values()) { + for (const g of group.aggs) { + if (g.ordered && g.computer) { + g.ordered.sort(compareRows(g.instance.orderBy!, t)); + for (const item of g.ordered) { + g.computer.feedItem(item); + } + g.ordered = undefined; + } + } + } // if this.base is empty, and this is not a group by... // 👉 Must return a result. @@ -372,6 +393,8 @@ export class Aggregation extends TransformBase implements _ISelection, _IAggrega const filter = call.filter ? buildValue(call.filter).cast(Types.bool) : null; + // jsonb_agg(x ORDER BY y), string_agg(s, ',' ORDER BY s), ... + const orderBy = call.orderBy?.length ? buildSortKeys(call.orderBy) : null; this.aggregations.set(hashed, { id, @@ -379,6 +402,7 @@ export class Aggregation extends TransformBase implements _ISelection, _IAggrega computer: got, distinct, filter, + orderBy, }); return getter; } diff --git a/src/transforms/aggregations/json_aggs.ts b/src/transforms/aggregations/json_aggs.ts index 5211375..2631004 100644 --- a/src/transforms/aggregations/json_aggs.ts +++ b/src/transforms/aggregations/json_aggs.ts @@ -17,12 +17,10 @@ class JsonAggExpr implements AggregationComputer { return { feedItem: (item) => { const value = this.exp.get(item, t); - if (!nullIsh(value)) { - // Same conversion row_to_json does: the aggregated body must - // carry json numbers for numeric/bigint, not the strings - // pg-mem holds them as internally. - full.push(toJsonValue(value, this.exp.type)); - } + // a NULL input is a json null in the result, as in postgres ([1, null, 2]). + // Same conversion row_to_json does: the aggregated body must carry json + // numbers for numeric/bigint, not the strings pg-mem holds them as internally. + full.push(nullIsh(value) ? null : toJsonValue(value, this.exp.type)); }, finish: () => full.length === 0 ? null : full, } diff --git a/src/transforms/order-by.ts b/src/transforms/order-by.ts index b340983..e6b38b8 100644 --- a/src/transforms/order-by.ts +++ b/src/transforms/order-by.ts @@ -9,12 +9,53 @@ export function buildOrderBy(on: _ISelection, order: OrderByStatement[]) { return new OrderBy(on, order); } +export interface SortKey { + by: IValue; + order: 'ASC' | 'DESC'; + nullsLast: boolean; +} + +/** ORDER BY items as sort keys (build them within the selection they read) */ +export function buildSortKeys(order: OrderByStatement[]): SortKey[] { + return order.map(x => { + const dir = x.order ?? 'ASC'; + return { + by: buildValue(x.by), + order: dir, + // postgres: NULLS LAST by default ascending, NULLS FIRST descending + nullsLast: dir === 'ASC' ? x.nulls !== 'FIRST' : x.nulls === 'LAST', + }; + }); +} + +/** a row comparator for Array.sort, following postgres' ORDER BY rules */ +export function compareRows(keys: SortKey[], t: _Transaction): (a: Row, b: Row) => number { + return (a, b) => { + for (const o of keys) { + const aval = o.by.get(a, t); + const bval = o.by.get(b, t); + const na = nullIsh(aval); + const nb = nullIsh(bval); + if (na && nb) { + continue; + } + if (na || nb) { + return nb === o.nullsLast ? -1 : 1; + } + if (o.by.type.equals(aval, bval)) { + continue; + } + if (o.by.type.gt(aval, bval)) { + return o.order === 'ASC' ? 1 : -1; + } + return o.order === 'ASC' ? -1 : 1; + } + return 0; + }; +} + class OrderBy extends FilterBase implements _IAggregation { - order: { - by: IValue; - order: 'ASC' | 'DESC'; - nullsLast: boolean; - }[]; + order: SortKey[]; get index() { return null; @@ -52,15 +93,7 @@ class OrderBy extends FilterBase implements _IAggregation { constructor(private selection: _ISelection, order: OrderByStatement[]) { super(selection); - this.order = withSelection(selection, - () => order.map(x => { - const order = x.order ?? 'ASC'; - return ({ - by: buildValue(x.by), - order, - nullsLast: order === 'ASC' ? x.nulls !== 'FIRST' : x.nulls === 'LAST', - }); - })); + this.order = withSelection(selection, () => buildSortKeys(order)); } @@ -76,28 +109,7 @@ class OrderBy extends FilterBase implements _IAggregation { enumerate(t: _Transaction): Iterable { const all = [...this.base.enumerate(t)]; - all.sort((a, b) => { - for (const o of this.order) { - const aval = o.by.get(a, t); - const bval = o.by.get(b, t); - const na = nullIsh(aval); - const nb = nullIsh(bval); - if (na && nb) { - continue; - } - if (na || nb) { - return nb === o.nullsLast ? -1 : 1; - } - if (o.by.type.equals(aval, bval)) { - continue; - } - if (o.by.type.gt(aval, bval)) { - return o.order === 'ASC' ? 1 : -1; - } - return o.order === 'ASC' ? -1 : 1; - } - return 0; - }); + all.sort(compareRows(this.order, t)); return all; } diff --git a/src/transforms/selection.ts b/src/transforms/selection.ts index 4b11df6..810c53d 100644 --- a/src/transforms/selection.ts +++ b/src/transforms/selection.ts @@ -49,7 +49,14 @@ function hasAggreg(e: Expr) { return; } visitor.super().call(expr); - } + }, + // an aggregate inside a subquery aggregates the subquery, not this query: + // `select id, (select count(*) from c where c.p = p.id) from p` is one row per p + select: x => x, + union: x => x, + with: x => x, + withRecursive: x => x, + values: x => x, })).expr(e); return has }