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

sqlite: Add split module #1772

Merged
merged 2 commits into from
Sep 19, 2023
Merged

sqlite: Add split module #1772

merged 2 commits into from
Sep 19, 2023

Conversation

lann
Copy link
Collaborator

@lann lann commented Sep 15, 2023

let stmts = split::split_sql("SELECT 1;SELECT * FROM my_table;").collect::<Result<Vec<_>>>()?;
assert_eq!(stmts, ["SELECT 1;", "SELECT * FROM my_table;"]);

@lann lann requested review from rylev and itowlson September 15, 2023 14:04
@lann lann marked this pull request as ready for review September 15, 2023 14:19
@lann lann force-pushed the sqlite-split branch 2 times, most recently from dab566b to 1ac9aa4 Compare September 15, 2023 14:40
@lann lann requested a review from dicej September 15, 2023 15:18
@itowlson
Copy link
Collaborator

I thought it was only used by the libsql implementation, so would vote for having it in the libsql crate as a private implementation detail, but that's a nit, and easy to move after merge.

This isn't currently hooked up to the libsql code that requires splitting; is that because this was primarily about socialising and agreeing the approach rather than plumbing it in? (It's fine if it is, I or others can do the plumbing - just checking.)

@lann
Copy link
Collaborator Author

lann commented Sep 17, 2023

Yeah it was more of a proof of concept that got out of hand.

@itowlson
Copy link
Collaborator

I did a quick test with this to make sure it approved Karthik's vexing SQL CREATE virtual TABLE vss_blog_posts3 USING vss0(embedding(384)); (which was the motivation for all this in the first place) - I wanted to check in case libsql and rusqlite disagreed on parsing. The good news is it worked!

The less good news is that this seemed to struggle with parsing files where one statement depended on a previous one. It happened that my test file for Karthik's SQL included some of my old database CREATE/INSERT setup - and that failed. Minimal repro:

CREATE TABLE fiteme23 (num INT);
INSERT INTO fiteme23(num) VALUES(55);

failed with "no such table: fiteme23". Could the embedded connection be doing more than parsing and be actually validating each statement in the context of the connection itself? Although if that were the case I don't get why it would allow the virtual table SQL which rusqlite presumably wouldn't be able to process without extensions.

(for the unit tests, the happy entry is ("CREATE TABLE fiteme23 (num INT); INSERT INTO fiteme23(num) VALUES(55);", &["CREATE TABLE fiteme23 (num INT);", "INSERT INTO fiteme23(num) VALUES(55);"]))

@lann
Copy link
Collaborator Author

lann commented Sep 17, 2023

Ugh I meant to test that but lost track of it in my unit testing fervour. I have another way to tackle this that I can try tomorrow.

@lann
Copy link
Collaborator Author

lann commented Sep 18, 2023

OK, this version doesn't validate statements quite so vigorously.

Used to split statements for execute_batch.

Signed-off-by: Lann Martin <lann.martin@fermyon.com>
Copy link
Collaborator

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

A couple of test cases I'd like to throw in (they pass, just to have them locked in), but otherwise LGTM. Thanks!

Signed-off-by: Lann Martin <lann.martin@fermyon.com>
@lann lann merged commit 631e32f into spinframework:main Sep 19, 2023
@lann lann deleted the sqlite-split branch September 19, 2023 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants