diff --git a/hypaware-core/plugins-workspace/ai-gateway/src/dataset.js b/hypaware-core/plugins-workspace/ai-gateway/src/dataset.js index 532ae9c9..e0611694 100644 --- a/hypaware-core/plugins-workspace/ai-gateway/src/dataset.js +++ b/hypaware-core/plugins-workspace/ai-gateway/src/dataset.js @@ -200,8 +200,12 @@ function withSchemaColumns(source) { // engine's streaming fast path. A partition that physically lacks the // requested column (the additive schema-drift case this wrapper exists // for) surfaces its values as `undefined` holes in the chunk; normalize - // them to null, the same "this partition predates the column" value the - // row path reads, so accumulators see one representation either way. + // them to null so every partition's chunk reads the same way and an + // accumulator sees one representation across the merged stream. This is + // NOT the value the row path reads: `scan` above pads an absent cell with + // `undefined` (LLP 0241 §alignment), and 0241 deliberately left the + // null/undefined split between the two paths unsettled, so nothing may + // branch on which one it got. // // A `where` naming a DECLARED-but-physically-absent column can't be // handed to the source: this wrapper is the only layer that knows the diff --git a/llp/0015-query-and-datasets.spec.md b/llp/0015-query-and-datasets.spec.md index 17a88ce9..739ea6d9 100644 --- a/llp/0015-query-and-datasets.spec.md +++ b/llp/0015-query-and-datasets.spec.md @@ -74,41 +74,45 @@ column a given partition physically lacks, and a parquet-backed source throws partition can't satisfy the predicate the union drops `where` for it and lets the engine filter. `columns` is always forwarded, which adds no failure the merged stream did not already have, but an absent column does **not** read as -null. A bare identifier projection (`SELECT extra FROM t`, with or without an -alias, a `LIMIT`, or a predicate on a column every partition has) leaves the -drifted cell **unresolved**: `executeProject` finds no matching cell, so it emits -a lazy `evaluateExpr` thunk that would throw, and writes no entry into the row's -`resolved` map. `collect()` reads the pre-materialized `resolved` map for every -advertised column and never invokes that thunk, so the key is present with the -value `undefined`, is not `null`, and `JSON.stringify` drops it. This holds only -under three conditions, all of them in `executeProject` and `collect()` rather -than in the union: the consumer goes through `collect()` (as `executeQuerySql` -does); every row reaching `collect()` carries a `resolved` map, which holds here -because every in-repo partition's rows come from squirreling's `asyncRow`, the -only thing that pre-materializes one; **and every output column of that -projection is a star or a bare identifier**. `executeProject` gates on the last -one up front (`resolveable`) and emits no `resolved` map at all when it fails, so -a single non-identifier sibling column (an expression, a function, or even a -literal) collapses the fast path for the whole result and the drifted column's -thunk is invoked and throws: `SELECT extra, 1 AS n FROM t` throws even though -nothing evaluates `extra`. A row that carries no `resolved` map at all does the -same. Anything -that *evaluates* the absent column throws squirreling's `ColumnNotFoundError` at -the first row from a partition without it: a `WHERE` on it, an expression or -function over it, `ORDER BY`, `GROUP BY`, `DISTINCT`, or an aggregate on it. -`SELECT *` is unaffected: each partition's rows keep their own shape, so the key -is simply absent. The column is addressable at all only because the union -advertises the superset of partition columns; when no partition has it, planning -fails with the same error unless a wrapper advertises the declared schema on top -of the union (LLP 0032's `withSchemaColumns`), which keeps such a column -addressable; the exact value a read of it then yields depends on the read path -and is not settled here. Pinned by +null: it reads as `undefined`. The union pads every row out to the column list +the scan advertised +([LLP 0241 §alignment](./0241-scan-rows-carry-advertised-columns.decision.md#alignment)), +so a column a partition physically lacks is still a real cell on the row, and +that cell resolves to `undefined`. The value is outside `SqlPrimitive` and +`JSON.stringify` drops it, so a padded column renders as an absent key even +though the row object owns it, and `Object.keys(row).length` over a star counts +the **advertised** columns rather than the physical ones. Every **row**-path +read agrees on that value: reading the row's pre-materialized `resolved` map +(`collect()`'s fast path), invoking the cell directly, and evaluating the column +above the scan in a `WHERE`, an expression or function over it, `ORDER BY`, +`GROUP BY`, `DISTINCT`, or an aggregate. The `scanColumn` column-stream path is +**not** part of that agreement and is not what padding changed: the union +forwards each partition's chunks unchanged, and a wrapper above it normalizes +the holes if it wants them uniform (LLP 0032's `withSchemaColumns` maps them to +`null`). LLP 0241 deliberately left that `null`/`undefined` split between the +two paths unsettled. `SELECT *` renders identically to the unpadded star, +because the key it now owns is one `JSON.stringify` drops. The column is +addressable at all only because the union advertises the superset of partition +columns; when no partition has it, planning fails unless a wrapper advertises +the declared schema on top of the union (LLP 0032's `withSchemaColumns`), which +keeps such a column addressable; the exact value a read of it then yields +depends on the read path and is not settled here. Pinned by [`test/core/union-source.test.js`](../test/core/union-source.test.js). > **Corrected (#731, PR #740).** This section previously stated that projecting > an absent column "reads as null, never throws". That was never true of the -> code; the paragraph above records the measured contract. No runtime behaviour -> changed. +> code when it was written; the paragraph above records the measured contract. +> No runtime behaviour changed. + +> **Corrected again (#820).** PR #740 was cut before, and merged after, +> [LLP 0241](./0241-scan-rows-carry-advertised-columns.decision.md), so the text +> it landed described a tree that no longer existed: an unresolved drifted cell, +> `undefined` only via `collect()`'s pre-materialized fast path, and a +> `ColumnNotFoundError` from anything that evaluated the column or merely sat +> beside it as a non-identifier sibling. 0241's padding removed that seam, and +> the semantic conflict turned `master` red. The paragraph above records the +> contract as measured on the current tree. No runtime behaviour changed in this +> correction either. > **Extended-by: [LLP 0241 §alignment](./0241-scan-rows-carry-advertised-columns.decision.md#alignment).** > The hints above say what a union may forward; LLP 0241 adds what shape the diff --git a/src/core/query/union-source.js b/src/core/query/union-source.js index f9f09abd..8cf86692 100644 --- a/src/core/query/union-source.js +++ b/src/core/query/union-source.js @@ -102,29 +102,26 @@ export async function* alignRows(rows, columns) { * reading the column as null. When a partition can't satisfy the predicate we * drop `where` for it and let the engine filter the concatenated stream (it * already owns the filter via `appliedWhere: false`). `columns` is always - * forwarded, which adds no failure the merged stream did not already have, but - * an absent column does NOT read as null. A bare identifier projection leaves - * the drifted cell unresolved: squirreling's `executeProject` finds no matching - * cell, so it emits a lazy `evaluateExpr` thunk that would throw and writes no - * entry into the row's `resolved` map. `collect()` reads the pre-materialized - * `resolved` map for every advertised column and never invokes that thunk, so - * the key is present with the value `undefined` (not `null`, dropped by - * `JSON.stringify`). That holds only under three conditions, all of them in - * `executeProject` and `collect()` rather than in the union: the consumer goes - * through `collect()`; every row reaching `collect()` carries a `resolved` map, - * which holds here because every in-repo partition's rows come from - * squirreling's `asyncRow`, the only thing that pre-materializes one; and every - * output column of that projection is a star or a bare identifier. - * `executeProject` gates on the last one up front (`resolveable`) and emits no - * `resolved` map at all when it fails, so a single non-identifier sibling column - * (an expression, a function, or even a literal) collapses the fast path for the - * whole result and the drifted column's thunk is invoked and throws: - * `SELECT extra, 1 AS n FROM t` throws even though nothing evaluates `extra`. A - * row that carries no `resolved` map at all does the same. - * Anything that evaluates the column (a `WHERE` on it, an expression over it, - * `ORDER BY`/`GROUP BY`/`DISTINCT`, an aggregate) throws `ColumnNotFoundError` - * at the first such row. `SELECT *` keeps each partition's own row shape, so the - * key is simply absent. `test/core/union-source.test.js` pins both halves. + * forwarded, which adds no failure the merged stream did not already have, and + * an absent column reads as `undefined` rather than `null` or a throw: every + * row is padded out to the scan's advertised column list by `alignRows` below, + * so a column a partition physically lacks is still a real cell that resolves + * to `undefined` (LLP 0241 §alignment). One consequence is worth stating, + * because it is the thing a maintainer gets wrong: `undefined` is outside + * `SqlPrimitive` and `JSON.stringify` drops it, so a padded column renders as + * an absent key even though the row object owns it. `Object.keys(row).length` + * over a star therefore counts the advertised columns, not the physical ones, + * and is not a way to discover what a partition holds. What no longer varies is + * which ROW path the caller took: reading `resolved`, invoking the cell, and + * evaluating the column in a `WHERE`, `ORDER BY`, `GROUP BY`, `DISTINCT` or an + * aggregate all agree, where a short row made the last group throw + * `ColumnNotFoundError` on the first partition lacking the column. The + * `scanColumn` hook below is a DIFFERENT path and padding does not touch it: it + * forwards each partition's chunks unchanged, so an absent column's value there + * is whatever the partition streams, and a wrapper above the union normalizes + * those holes if it wants them uniform (ai-gateway's `withSchemaColumns` maps + * them to `null`). LLP 0241 left that `null`/`undefined` split between the two + * paths unsettled on purpose. `test/core/union-source.test.js` pins these. * * Because a sub-source now emits exactly the columns it is asked for (see * `parquet-source.js`), forwarding `columns` also determines what the engine diff --git a/test/core/union-source.test.js b/test/core/union-source.test.js index 1814f119..2e0d09e0 100644 --- a/test/core/union-source.test.js +++ b/test/core/union-source.test.js @@ -483,7 +483,7 @@ function hasExtraKey(rows, key = 'extra') { return rows.map((row) => Object.prototype.hasOwnProperty.call(row, key)) } -// @ref LLP 0015#multi-partition-union [tests]: forwarding `columns` does not null-pad; the drifted cell is `undefined`, which is why the doc no longer promises null +// @ref LLP 0015#multi-partition-union [tests]: the union pads a drifted cell with `undefined`, never `null`, which is why the doc no longer promises null test('a projected column one partition lacks reads as undefined, never null', async () => { // Every bare-projection shape the LLP names, so "Pinned by" covers the alias // and `LIMIT` variants it claims and not just the plain projection. @@ -492,8 +492,9 @@ test('a projected column one partition lacks reads as undefined, never null', as { query: 'SELECT extra FROM t WHERE score > 1', key: 'extra', values: [undefined, undefined, 'x'], json: '[{},{},{"extra":"x"}]' }, { query: 'SELECT extra AS e FROM t', key: 'e', values: [undefined, undefined, 'x'], json: '[{},{},{"e":"x"}]' }, // A bare-identifier SIBLING keeps `executeProject`'s `resolveable` gate - // open, so the fast path survives. The throwing test below pins what a - // non-identifier sibling does to the very same query. + // open, so `collect()` stays on its pre-materialized fast path. The test + // below pins the same answer coming back off the slow path, where a + // non-identifier sibling closes that gate and the cell is really invoked. { query: 'SELECT score, extra FROM t', key: 'extra', values: [undefined, undefined, 'x'], json: '[{"score":1.5},{"score":2.5},{"score":3.5,"extra":"x"}]' }, { query: 'SELECT extra FROM t LIMIT 1', key: 'extra', values: [undefined], json: '[{}]' }, { query: 'SELECT extra FROM t LIMIT 0', key: 'extra', values: [], json: '[]' }, @@ -503,35 +504,43 @@ test('a projected column one partition lacks reads as undefined, never null', as assert.equal(rows.length, values.length, query) assert.deepEqual(hasExtraKey(rows, key), values.map(() => true), 'the projection puts the key on every row') assert.deepEqual(rows.map((row) => row[key]), values, query) - assert.equal(rows[0]?.[key] === null, false, 'undefined, not null: the union never pads the drifted partition') + assert.equal(rows[0]?.[key] === null, false, 'undefined, not null: the union pads the drifted cell, it does not null it') assert.equal(JSON.stringify(rows), json, 'JSON.stringify drops the undefined cells') } }) -// @ref LLP 0015#multi-partition-union [tests]: the undefined read is `collect()`'s pre-materialized fast path, not a copied value; the cell itself throws -test('the drifted cell is unresolved and throws; only collect() turns it into undefined', async () => { +// @ref LLP 0241#alignment [tests]: the drifted cell is padded by the union, so it is a real resolvable cell rather than a throwing thunk +test('the drifted cell is padded, so it resolves to undefined instead of throwing', async () => { const results = executeSql({ tables: { t: await driftedUnion() }, query: 'SELECT extra FROM t' }) - /** @type {{ resolvedHasKey: boolean, cell: string }[]} */ + /** @type {{ resolvedHasKey: boolean, cell: string, value: SqlPrimitive | undefined }[]} */ const seen = [] for await (const row of results.rows()) { const resolved = row.resolved let cell = 'ok' + /** @type {SqlPrimitive | undefined} */ + let value try { - await row.cells.extra() + value = await row.cells.extra() } catch (err) { cell = err instanceof Error ? err.constructor.name : 'unknown' } - seen.push({ resolvedHasKey: !!resolved && Object.prototype.hasOwnProperty.call(resolved, 'extra'), cell }) + seen.push({ resolvedHasKey: !!resolved && Object.prototype.hasOwnProperty.call(resolved, 'extra'), cell, value }) } + // The union pads each row out to the column list the scan advertised, so + // `executeProject` finds `extra` in `row.cells` on every row and takes its + // copy path. The cell is real, it resolves to `undefined`, and `resolved` + // carries the key. Nothing here depends on `collect()`'s fast path: the + // caller reading the cell directly and the caller reading `resolved` now + // agree, which is what stops the same query answering two different ways. assert.deepEqual(seen, [ - { resolvedHasKey: false, cell: 'ColumnNotFoundError' }, - { resolvedHasKey: false, cell: 'ColumnNotFoundError' }, - { resolvedHasKey: true, cell: 'ok' }, - ], 'a drifted cell is a throwing thunk with no `resolved` entry; `collect()` reads `resolved` and never calls it') + { resolvedHasKey: true, cell: 'ok', value: undefined }, + { resolvedHasKey: true, cell: 'ok', value: undefined }, + { resolvedHasKey: true, cell: 'ok', value: 'x' }, + ], 'the padded cell resolves to undefined and `resolved` carries the key') }) -// @ref LLP 0015#multi-partition-union [tests]: the undefined read is load-bearing on every partition pre-materializing `resolved` -test('a partition whose rows carry no resolved map makes a bare projection throw', async () => { +// @ref LLP 0241#alignment [tests]: padding happens below `executeProject`, so a partition that pre-materializes no `resolved` map reads the same as one that does +test('a partition whose rows carry no resolved map reads the same, because the union pads below it', async () => { /** * A legal `AsyncDataSource` that hand-rolls its rows instead of going * through squirreling's `asyncRow`, so nothing pre-materializes `resolved`. @@ -568,45 +577,73 @@ test('a partition whose rows carry no resolved map makes a bare projection throw unresolvedSource(['id', 'score'], [{ id: 1, score: 1.5 }]), unresolvedSource(['id', 'score', 'extra'], [{ id: 3, score: 3.5, extra: 'x' }]), ]) - for (const query of ['SELECT extra FROM t', 'SELECT extra AS e FROM t', 'SELECT extra FROM t LIMIT 1']) { - await assert.rejects( - () => collect(executeSql({ tables: { t: union }, query })), - /Column "extra" not found/, - `${query} throws without collect()'s pre-materialized fast path` - ) - } -}) - -// @ref LLP 0015#multi-partition-union [tests]: evaluating an absent column throws rather than reading as null, and so does a non-identifier sibling column that never touches it -test('evaluating a column one partition lacks throws, and so does a non-identifier sibling', async () => { - const evaluating = [ - 'SELECT extra FROM t WHERE extra IS NOT NULL', - "SELECT id FROM t WHERE extra = 'x'", - "SELECT coalesce(extra, 'none') AS e FROM t", - 'SELECT id, extra FROM t ORDER BY extra', - 'SELECT max(extra) AS m FROM t', + // The union pads before `executeProject` ever sees a row, so where the + // source got its cells no longer decides the answer. A hand-rolled source + // that never calls squirreling's `asyncRow` reads exactly like the parquet + // partitions above. + /** @type {[string, string, (SqlPrimitive | undefined)[]][]} */ + const cases = [ + ['SELECT extra FROM t', 'extra', [undefined, 'x']], + ['SELECT extra AS e FROM t', 'e', [undefined, 'x']], + ['SELECT extra FROM t LIMIT 1', 'extra', [undefined]], + // A non-identifier sibling closes `executeProject`'s `resolveable` gate, + // so `collect()` falls off its pre-materialized fast path and invokes the + // cell. The padded cell resolves, so this shape answers too. + ['SELECT extra, 1 AS n FROM t', 'extra', [undefined, 'x']], ] - for (const query of evaluating) { - await assert.rejects(() => runDrifted(query), /Column "extra" not found/, query) + for (const [query, key, values] of cases) { + const rows = await collect(executeSql({ tables: { t: union }, query })) + assert.deepEqual(hasExtraKey(rows, key), values.map(() => true), `${query} puts the key on every row`) + assert.deepEqual(rows.map((row) => row[key]), values, query) } +}) - // Nothing below evaluates `extra`. `executeProject` computes `resolveable` - // over the WHOLE column list and emits no `resolved` map when any output - // column is neither a star nor a bare identifier, so one literal or one - // expression sibling drops `collect()`'s fast path for the entire result, - // the drifted thunk is invoked, and it throws. This is the shape that - // surprises people: `SELECT score, extra FROM t` above reads `undefined`, - // and adding `, 1 AS n` to it throws. - const siblingCollapsesTheFastPath = [ - 'SELECT extra, 1 AS n FROM t', - 'SELECT extra, score * 2 AS d FROM t', - ] - for (const query of siblingCollapsesTheFastPath) { - await assert.rejects(() => runDrifted(query), /Column "extra" not found/, query) +// @ref LLP 0241#alignment [tests]: a clause the engine evaluates above the scan reads the padded cell as undefined and answers, where a short row made it throw +test('evaluating a column one partition lacks answers with undefined, and so does a non-identifier sibling', async () => { + // Every shape here read the absent column off `row.cells` above the scan and + // raised `ColumnNotFoundError` on the short row. On a padded row the lookup + // hits, resolves to `undefined`, and the query answers. The answers are the + // ones the hinted form of each query always gave, so this is the union + // agreeing with itself rather than a new result. + assert.deepEqual(await runDrifted('SELECT extra FROM t WHERE extra IS NOT NULL'), [{ extra: 'x' }]) + assert.deepEqual((await runDrifted("SELECT id FROM t WHERE extra = 'x'")).map((row) => Number(row.id)), [3]) + assert.deepEqual(await runDrifted("SELECT coalesce(extra, 'none') AS e FROM t"), [{ e: 'none' }, { e: 'none' }, { e: 'x' }]) + assert.deepEqual(await runDrifted('SELECT max(extra) AS m FROM t'), [{ m: 'x' }]) + + const ordered = await runDrifted('SELECT id, extra FROM t ORDER BY extra') + assert.deepEqual(ordered.map((row) => Number(row.id)), [1, 2, 3]) + assert.deepEqual(ordered.map((row) => row.extra), [undefined, undefined, 'x']) + + // Nothing below evaluates `extra`, but a literal or expression sibling + // closes `executeProject`'s `resolveable` gate, so `collect()` leaves its + // pre-materialized fast path and invokes the cell for real. The padded cell + // resolves, so the sibling no longer decides whether the query answers. + for (const [query, key, values] of /** @type {[string, string, (SqlPrimitive | undefined)[]][]} */ ([ + ['SELECT extra, 1 AS n FROM t', 'n', [1, 1, 1]], + ['SELECT extra, score * 2 AS d FROM t', 'd', [3, 5, 7]], + ])) { + const rows = await runDrifted(query) + assert.deepEqual(rows.map((row) => row.extra), [undefined, undefined, 'x'], query) + assert.deepEqual(rows.map((row) => row[key]), values, query) } }) -test('SELECT * keeps each partition row shape, so a drifted key is absent rather than undefined', async () => { +// @ref LLP 0241#alignment [tests]: a star pads each partition row out to the advertised list, so the key count matches the schema `QueryResults.columns` already promised +test('SELECT * pads each partition row to the union column list, so a drifted key is present and undefined', async () => { const rows = await runDrifted('SELECT * FROM t') - assert.deepEqual(hasExtraKey(rows), [false, false, true], 'a star projection copies only the columns a row has') + assert.deepEqual(hasExtraKey(rows), [true, true, true], 'every row carries every advertised column') + assert.deepEqual(rows.map((row) => Object.keys(row)), [ + ['id', 'score', 'extra'], + ['id', 'score', 'extra'], + ['id', 'score', 'extra'], + ], 'the key order is the advertised order, on the narrow partition too') + assert.deepEqual(rows.map((row) => row.extra), [undefined, undefined, 'x']) + // The padded cell is `undefined`, not `null`, so the rendering is byte for + // byte what the unpadded star produced: `JSON.stringify` drops the key it + // used to drop by absence. + assert.equal( + JSON.stringify(rows, (_key, value) => (typeof value === 'bigint' ? Number(value) : value)), + '[{"id":1,"score":1.5},{"id":2,"score":2.5},{"id":3,"score":3.5,"extra":"x"}]', + 'padding an absent cell with undefined renders identically to omitting the key' + ) })