Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: kysely/helpers/sqlite #588

Merged
merged 13 commits into from
Jul 22, 2023
Merged

Conversation

jlarmstrongiv
Copy link
Contributor

@jlarmstrongiv jlarmstrongiv commented Jul 14, 2023

I have tested the queries in the comments—those SQLite queries work as intended.

Unfortunately, I could not use the PostgreSQL helpers due to Parse error: no such column: agg. It seems that SQLite still expects column names.

I have also modified the kysely functions to match the SQLite queries.

The schema used in the examples is:

CREATE TABLE IF NOT EXISTS "person" ("id" integer primary key, "first_name" text not null, "last_name" text, "gender" text not null, "created_at" text default CURRENT_TIMESTAMP not null);
CREATE TABLE IF NOT EXISTS "pet" ("id" integer primary key, "name" text not null unique, "owner_id" integer not null references "person" ("id") on delete cascade, "species" text not null, is_favorite boolean);
CREATE INDEX "pet_owner_id_index" on "pet" ("owner_id");

with inserts like:

insert into person (id, first_name, last_name, gender, created_at) values (2, 'joe', 'joel', 'j', '2');
insert into person (id, first_name, last_name, gender, created_at) values (3, 'sue', 'suel', 's', '3');
insert into pet (id, name, owner_id, species, is_favorite) values (1, 'jill', 2, 'j', true);
insert into pet (id, name, owner_id, species, is_favorite) values (2, 'mark', 2, 'm', true);
insert into pet (id, name, owner_id, species) values (3, 'cole', 3, 'c');

EDIT: will add tests to the suite https://github.com/kysely-org/kysely/blob/master/test/node/src/json.test.ts thank you @koskimas

@vercel
Copy link

vercel bot commented Jul 14, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
kysely ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 22, 2023 3:31am

@koskimas
Copy link
Member

Nice job, Thank you! Both postgres and mysql helpers are tested here. You can add sqlite tests to the same suite.

@koskimas
Copy link
Member

koskimas commented Jul 14, 2023

You also need to add the new kysely/helpers/sqlite export to package.json. See the section with:

    "./helpers/mysql": {
      "import": "./dist/esm/helpers/mysql.js",
      "require": "./dist/cjs/helpers/mysql.js",
      "default": "./dist/cjs/helpers/mysql.js"
    }

Copy link
Member

@igalklebanov igalklebanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! 💪

Let's not forget to mention how these can be imported in the relations recipe.

@jlarmstrongiv
Copy link
Contributor Author

@igalklebanov how do I concat in sqlite with kysely? I saw in another comment in another test that:

return sql`json_object(${sql.join(
    Object.keys(obj).flatMap((k) => [sql.lit(k), obj[k]])
  )})`

should return the concatenation operator || in postgres (and I assume SQLite), but that doesn’t seem to be the case. I am receiving the error:

SqliteError: no such function: concat

@koskimas
Copy link
Member

koskimas commented Jul 16, 2023

@igalklebanov how do I concat in sqlite with kysely? I saw in another comment in another test that:

return sql`json_object(${sql.join(
    Object.keys(obj).flatMap((k) => [sql.lit(k), obj[k]])
  )})`

should return the concatenation operator || in postgres (and I assume SQLite), but that doesn’t seem to be the case. I am receiving the error:

SqliteError: no such function: concat

The code you provided has nothing to do with concatenation. That SQL snippet should produce

json_object('foo', some_expression, 'bar', some_other_expression, ...)

There's no concatenation anywhere, unless one of the expressions has concatenation in them.

I'm guessing the comment you referred to just meant the || operator doesn't exist on mysql. But it does on postgres and sqlite. Change the test so that || is used on sqlite instead of the concat function.

@rprwhite
Copy link

I'm guessing the comment you referred to just meant the || operator doesn't exist on mysql. But it does on postgres and sqlite. Change the test so that || is used on sqlite instead of the concat function.

Replacing the concat function

full: eb.fn('concat', ['first_name', sql.lit(' '), 'last_name']),

with

  full: sql<string>`first_name || ' ' || last_name`

Throws a type error at build

TypeError: SQLite3 can only bind numbers, strings, bigints, buffers, and null

I assume, because of this WiseLibs/better-sqlite3#907 (comment)

It does work in postgres and mysql (as long as PIPES_AS_CONCAT is set).
What's the best way to approach this for sqlite?

@koskimas
Copy link
Member

That error makes no sense. That sql isn't binding anything. it's a raw SQL snippet. My guess is that your implementation of jsonBuildObject tries to send the whole sql instance as a parameter for some reason.

@koskimas
Copy link
Member

koskimas commented Jul 18, 2023

Ok, so the error comes from the false value in this part of the test:

        // Nest an empty list
        jsonArrayFrom(
          eb
            .selectFrom('pet')
            .select('id')
            .where((eb) => eb.val(false))
        ).as('emptyList'),

better-sqlite3 doesn't support booleans as parameters. You can change

.where((eb) => eb.val(false))

into

where(sql.lit(false))

@koskimas
Copy link
Member

The next problem is that sqlite doesn't parse JSON (since there's no JSON datatype). You need to add this

      if (dialect === 'sqlite') {
        db = db.withPlugin(new ParseJSONResultsPlugin())
      }

in the before block. But it seems that ParseJSONResultsPlugin only parses the top level objects. In the nested relations, the data is doubly, triply etc. encoded json and the plugin would need to do the parsing recursively. @igalklebanov

@igalklebanov
Copy link
Member

igalklebanov commented Jul 18, 2023

@koskimas yeah, this was good enough for the JSON traversal tests. Maybe add an optional flag to recurse?

@koskimas
Copy link
Member

I can fix the plugin. I don't think there's any need for a flag.

@koskimas
Copy link
Member

The plugin is now fixed.

@jlarmstrongiv
Copy link
Contributor Author

Yay 🎉 thank you everyone!! The tests are passing

@jlarmstrongiv jlarmstrongiv changed the title DO NOT MERGE: feat: kysely/helpers/sqlite feat: kysely/helpers/sqlite Jul 19, 2023
src/helpers/sqlite.ts Outdated Show resolved Hide resolved
src/helpers/sqlite.ts Outdated Show resolved Hide resolved
src/helpers/sqlite.ts Outdated Show resolved Hide resolved
@jlarmstrongiv
Copy link
Contributor Author

Good catch, fixed that typo @koskimas 👍

src/helpers/sqlite.ts Outdated Show resolved Hide resolved
@koskimas
Copy link
Member

After the small comment change, I think this is good to go!

@koskimas koskimas merged commit 4d4bf93 into kysely-org:master Jul 22, 2023
4 checks passed
@ncrmro
Copy link

ncrmro commented Aug 19, 2023

Howdy yall @koskimas the sqlite pacakge doesn't appear to be in "kysely": "^0.26.1", ?

@jlarmstrongiv
Copy link
Contributor Author

@ncrmro follow #628 for updates

Gaspero pushed a commit to Gaspero/kysely that referenced this pull request Oct 2, 2023
* feat: kysely/helpers/sqlite

* test: json sqlite functions

* feat: add sqlite json helpers to package.json exports

* docs: typo in code comments

* fix: sqlite json_arrayagg to json_group_array

* fix: sqlite custom join separator

* docs: update recipe to include SQLite

* fix: remove custom join separator

* docs: mention SQLite ParseJSONResultsPlugin

* fix: use ParseJSONResultsPlugin, || for sqlite concat, and sql.lit(false)

* docs: compatible typo

* docs: SqliteDialect typo

* docs: ParseJSONResultsPlugin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request helpers Related to library's helpers sqlite Related to sqlite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants