Adding resultsFormat to d1-api.ts to fix .raw() bug

[geelen]

This PR does a couple of things:

• Massively improves the realism of the D1 test suite. I've also got d1-api.ts and d1-api-test.js being imported into D1's internal test suite to verify these are correct, but the new d1-mock now much more closely mirrors the real system.
• Adds a resultsFormat flag to better control what D1 returns behind the scenes, with a default to ROWS_AND_COLUMNS, which should be more efficient (see below).
• Fixes .run() to no longer return .results property, as per the docs.
• Makes .raw() effectively the default and derives .all()/.first() from that, rather than the other way around.

In particular, this change fixes a long-standing bug with D1 where joining tables with the same column names was losing data, e.g.

await DB.exec(`
  CREATE TABLE abc (a INT, b INT, c INT);
  CREATE TABLE cde (c INT, d INT, e INT);
  INSERT INTO abc VALUES (1,2,3),(4,5,6);
  INSERT INTO cde VALUES (7,8,9),(1,2,3);
`),

const { results } = await DB.prepare(`SELECT * FROM abc, cde;`).all()
//>  results: [
//>     { a: 1, b: 2, c: 7, d: 8, e: 9 },
//>     { a: 1, b: 2, c: 1, d: 2, e: 3 },
//>     { a: 4, b: 5, c: 7, d: 8, e: 9 },
//>     { a: 4, b: 5, c: 1, d: 2, e: 3 },
//>   ]

Note that cde.c is overriding abc.c in each of the rows as it's a simple object map and last-one-wins.

The intended solution was to be able to use .raw() to skip the step of building JS objects from each row:

await DB.prepare(`SELECT * FROM abc, cde;`).raw()
//>  [
//>    [1, 2, 3, 7, 8, 9],
//>    [1, 2, 3, 1, 2, 3],
//>    [4, 5, 6, 7, 8, 9],
//>    [4, 5, 6, 1, 2, 3],
//>  ]

But previously that wasn't working as the D1 service was building the objects before sending them back, so by the time your worker got a response the data was already missing. In fact, .raw() previously just did a Object.values() on each row of the response, which is some real alpha code (sorry everyone).

That's now fixed, if you pass ?resultsFormat=ROWS_AND_COLUMNS you get { results: { columns: [...], rows: [[...],[...], ...]} instead of { results: [{ ... }, { ... }, ...]} and you can reconstruct the object form for .all() just fine. But since this code is going to roll out independently of the backend changes, this PR was slightly complicated by having to support both old and new response types.

One outstanding question: should we find a way to return .columns to the user at all? As it stands, .raw() returns a simple array: [ row1_values, row2_values, ... ], which doesn't really leave space for a .columns property (even if that would be technically possible to jam in there). Perhaps we should make .raw(true) return a { rows, columns } object?

Leaving this as draft PR for now as I'm not 100% that this API doesn't need further changes, but would appreciate any 👀 from workerd folks.

[geelen]

Update on this, we're going to add .raw({ columnNames: true }) to return column names:

const [columns, ...rows] = await DB.prepare(`SELECT * FROM abc, cde;`).raw({ columnNames: true })
//>  columns: ["a","b","c","c","d","e"]
//>  rows: [
//>    [1, 2, 3, 7, 8, 9],
//>    [1, 2, 3, 1, 2, 3],
//>    [4, 5, 6, 7, 8, 9],
//>    [4, 5, 6, 1, 2, 3],
//>  ]

I definitely think we can do better in future but for now this at least makes it possible to return the column names, even if it's slightly unergonomic.

[mrbbot]

Not for this PR, but this file is kinda similar to Miniflare's d1/database.worker.ts. I wonder if there's some way we could avoid duplicating these implementations? Should Miniflare's workers maybe live inside workerd?

[geelen]

Oh yeah this is absolutely doing the same thing, which means miniflare needs to be updated as well. 🤔

Would be great to combine somehow, I'll start a new convo.

[geelen]

See cloudflare/workers-sdk#4890

[mrbbot]

Looks good, with one comment: do any of these changes need to be gated behind a compatibility flag? Specifically thinking about raw() now potentially returning an array of arrays with more values/different indexes for existing values.

[rozenmd]

IMO dump() is safe to remove today, since it's a no-op for SRS-backed dbs

[geelen]

Yeah I know but it's still a breaking change unless we've already shut down all alpha DBs. So I was going to leave it for now and fix it when we're forced to introduce a compat date.

[geelen]

Looks good, with one comment: do any of these changes need to be gated behind a compatibility flag? Specifically thinking about raw() now potentially returning an array of arrays with more values/different indexes for existing values.

Yeah I can see arguments either way, but the existing behaviour is definitely a bug (that very few people have actually hit) so I wanted to just roll it out silently. My main issue with a compat_date change is that we have to leave the old API in there alongside the fixed version, or make d1-api aware of the change and able to... reproduce the buggy result for older compat dates?