-
Notifications
You must be signed in to change notification settings - Fork 265
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
Use ANTLR SQLite grammar for batch splitting #1729
Conversation
b1fbe7d
to
51c8a45
Compare
Oh, heck, even though we only use the output (and it builds fine), it seems like the ANTLR runtime has a custom build command that is making the e2e tests sad... |
Is it... is it trying to regenerate its test grammars? https://github.com/rrevenantt/antlr4rust/blob/master/build.rs But why, then, would it build in, er, build, but not in e2e?! |
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
51c8a45
to
191f798
Compare
Ah, there's a beta on |
@itowlson this PR scares me - I'd really rather not have to maintain this if possible. Perhaps we're going about this the wrong way, and instead we should be exploring ways we can avoid having to split strings into multiple SQL statements. Perhaps there's a way we can just send the entire string as one thing and let any downstream processes do the splitting if they need to. I realize this might require asking for features in projects we don't control, but perhaps that is a more sure first step? |
We could parse the statements with |
@rylev That would be vastly preferable. The Go libsql client already accepts multiple statements in a single string, in order to be consistent with the Go SQL interface; if we can persuade libsql to have a single-string function in the Rust client too, then this whole problem goes away. |
@rylev To be clear re "ways we can avoid having to split strings into multiple SQL statements"... We are splitting because somewhere at some deep level (I think sqld / hrana, but I didn't keep adequate notes when diving into this, sorry - if you follow the link in the code it should take you to the starting point was the starting point for me) there is an interface (hrana BatchRequest I think) which accepts an array of statements, and rejects strings that contain multiple statements. We don't see this in our own sqld interactions because they're written in Go, and Go (in order to conform to its standard SQL API) disguises this by splitting internally before handing off to the database layer. So as far as I can tell "avoid having to split" means either:
Not sure of any other options but very much open to anything that makes this not our problem! |
This seems to work: #1772 |
Closing in favour of the frankly less alarming #1772. |
This addresses a user issue where SQLite extensions supported by the libsql vendor could not be used in
spin up --sqlite @file
. It aligns our statement splitter with the one used bylibsql-go
, which is probably as official as we're going to get.There are possible maintenance implications if we ever need to update the grammar; I have documented the process but it is very much a manual one.
As noted, the goal here is to align with
libsql-go
. To do so, I have followed the Go implementation as closely as the borrow checker will let me. This means the code is (to my eyes) not very idiomatic Rust, and (to Clippy's eyes) an abomination before heaven. We could make an idiomatic implementation, but then it would be harder to follow if we ever needed to port an update from the Go implementation. Maybe it's not so huge that it would be a concern, but my understanding of the original was not great, so I chose to keep it as similar as I could - open to feedback for sure, but I'd prefer that to be at the level of "to what extent do we want to align" rather than pointing out individual un-Rustacean bits!The huge
sqlite_lexer
file is ANTLR-generated; the source is documented in themod.rs
comments.It would be good to get some samples for tests, since the maintenance of this is now on us.