search

prisma / quaint

SQL Query AST and Visitor for Rust
Apache License 2.0
582 stars 62 forks source link

sqlite: open the database as main #126

Closed cryslith closed 4 years ago

cryslith commented 4 years ago

This allows referring to possibly-nonexistent tables with their unqualified names in raw commands, and is more consistent with the behavior of the Postgres and MySQL backends.

In particular, it enables creating tables using the Queryable interface. For instance, one can write db.cmd_raw("CREATE TABLE my_table (x INTEGER);") to create a table, instead of db.cmd_raw("CREATE TABLE db_name.my_table (x INTEGER);") (where db_name is the value of the db_name parameter in the database URL). This is useful because without it, one would have to re-parse the original database URL in order to get the value of db_name, and then thread that value through all code that needs to create a table.

For backwards compatibility, the database file is still attached as db_name too, so it's mapped as both main and db_name.

tomhoule commented 4 years ago

We have a fairly large codebase relying on quaint at https://github.com/prisma/prisma-engines

I think the rationale for the change is solid. We could test prisma with this change and merge if we don't find any issue, wdyt @pimeys ?

pimeys commented 4 years ago

Yeah. We basically always have the database indicator in all our queries. Model is User and the Schema is Blog, so all the queries will be User.Blog, and user.db is attached as User.

Might be a trickier change.

tomhoule commented 4 years ago

Sorry for neglecting this PR for so long. So I did try this patch on the prisma repo, and it seems like the queries on the ATTACHed database are now read-only, which is a problem since we do add the attached name everywhere. I think the way forward would be for us to no longer do that, and we can merge this PR. What do you think @pimeys ?

pimeys commented 4 years ago

Yes.

tomhoule commented 4 years ago

@cryslith sorry it got delayed so long. We did some testing and it turned out to be more complicated than expected on our end, but I think we should merge your PR now. It just needs to be rebased first.

@pimeys Here's the related PR: https://github.com/prisma/prisma-engines/pull/1156 — there is some cleanup work left to do, but it demonstrates that we can make it work.

cryslith commented 4 years ago

Closing since #192 was merged