SQL Client: allow DuckDB's Dot Commands to work without adding trailing semicolon

[sheluchin]

DuckDB has a bunch of "Dot Commands" which don't work with a trailing semicolon. Conjure currently inserts trailing semicolons when evaluating in a SQL file.

A few ideas for improving support:

• add some command detection for cases when the ; shouldn't be added:
  • DuckDB's leading . and the usual leading \ would cover most case
• allow this feature to be toggled on/off
• allow configuring the pattern beyond leading . / \ in case other SQL clients do it differently

[russtoku]

When I created the SQL client, I didn't automatically add a semicolon (;) statement terminator to the end of statements sent to the REPL because I wanted to be able to send psql metacommands (equivalent of DuckDB Dot Commands).

A subsequent commit added the feature to automatically append the semicolon to whatever is sent to the REPL.

Might I suggest that this feature be made an option, g:conjure#client#sql#stdio#append_semicolon, that defaults to "on" (true) like the HUD (heads-up display). For example, it's my preference to turn off the HUD in favor of seeing more of what's coming back from the REPL. Thus, my Neovim config sets g:conjure#log#hud#enabled to false.

The feature can then be toggled off when needing to send the REPL a command that shouldn't have a semicolon at the end. And back on when sending SQL statements. Toggling can easily be added for individual user's tastes by adding a key mapping.

The logic of trying to determine what is a REPL command for various REPLs vs SQL statement would require a bunch of code and testing. So, advantage feature toggle for less code overall.

[Olical]

So I added the pesky auto-semicolon, I can't remember exactly why now, it was either because of an issue where people were complaining about the REPL getting stuck or because I thought it would make the behaviour more "Do What I Meant".

Oh! I think I remember, it was because the tree-sitter access was returning the expressions without semi-colons! I think!

So rather than try to get clever with it, I just started appending semi-colons everywhere to make it always send a finished expression on every evaluation. I could've edited my first paragraph after I remembered but I like to leave my workings out to show how forgetful I am 😛

I'll have a quick tinker and see if I can get it to hunt one character / node further for the semi colon and include it if found. If it's not there, I'll let you send without it, so we respect the user and what they're trying to do. Because maybe you want to send an expression across multiple evaluations?

I still worry we'll let the user get into a "more input required" state which feels VERY odd from within Conjure but is kind of easier to grok in the terminal prompt. Maybe we can indicate "more input required" somehow in Conjure too?

[Olical]

Interestingly when I evaluate \watch the node type is ERROR, when I evaluate .tables the node type is also .tables. Kinda hard to work with that and have confidence, but it's something.

I can't seem to get the ; from a statement though, the tree sitter API just doesn't return it. If I go up one level I get the whole document. SO! That means I need to check if the node is a statement, if it is then I append a semi-colon.

Then the user can use visual selection to evaluate special implementation specific commands. The ",ee" etc mappings won't work with them just yet because the tree sitter parser doesn't like them, I'll see if I can improve that but that might require changes to the parser itself and might be out of scope for a hacky workaround here.

If we can at least get statements working 100% of the time and non-statements working through range / visual eval, that'll be a start!

[Olical]

Please check out the main branch and let me know how it works for you! There are issues but they are with the tree sitter grammar and I'm not sure what I can do about them.

Basically if you have two "meta commands" (is that a good name?) on two lines next to each other, we pick up both right now and send them both at the same time. We don't spot the new line as the boundary. Maybe there's a way I can fudge this too, are they always on a single line? If so, maybe I can work with that somehow but it might get messy.

For now, I only append ; to true statements according to tree-sitter. Everything else just gets a new line on the end which I think is fine.

[Olical]

Yeah this might be tricky to get any better, I'm open to ideas.

.tables foo
.tables bar

When you ask tree sitter "what node is under my cursor" on either of these lines, it returns both lines. So I'd have to start saying "oh we got an ERROR node, which line is the current cursor on and take that whole line" or something like that. Gets pretty fiddly. It's doable within the client API technically, I just worry I'll write something clever that still fails in a bunch of edge cases.

Really it'd be nice if the tree sitter grammar picked up meta commands from various SQLs.

[Olical]

Got it working! So I assume if you try to run a "meta command" as a form now you want to take the whole line. Used one of the newer more powerful client APIs to get this working, added it for Julia and it works great for this issue too. Yay! Multiple client support paying off again!

So now you can eval statements and meta commands as if they were real forms understood by tree sitter (even though they're not yet 😢 ) with the caveat that a meta command sends the whole line.

I hope this works great for you! Let me know if you hit any edge cases or if this is exactly what you want.

[sheluchin]

@Olical thanks for looking at this already!

Here's the snippet I'm testing on:

.tables

.mode lines

SELECT
    date,
    type,
    description,
    city
FROM crime_scene_report;

It looks almost totally right, but I think something is amiss in 75f475e. Perhaps you left some debugging code in there? I'm getting messages like this in the lower left message position:

-- eval (current-form): .tables
<node ERROR> ERROR
Press ENTER or type command to continue

-- eval (current-form): SELECT date, type, description, city FROM crime_scene_re...
<node statement> statement ;
-- eval (current-form): SELECT date, type, description, city FROM crime_scene_re...

but it it does actually do the thing.

The only other thing I notice so far is that putting your cursor anywhere in the first two meta commands and doing a root eval will run both:

-- eval (root-form): .tables .mode lines
crime_scene_report      get_fit_now_check_in    interview             
drivers_license         get_fit_now_member      person                
facebook_event_checkin  income                  solution              

[Olical]

I did indeed leave a print in!

[Olical]

Removed 😄

The root form one is a little harder though... will have to think about how I can kind of "step in" and interrupt the normal behaviour in order to cut the buffer up and provide one line.

tree-sitter is sort of messing with us here and I have to add special cases where I know tree-sitter will be wrong. This is actually kind of useful to have in general, so might be worth the extra effort.

[sheluchin]

Found one more edge case. Looks like inline comments cause problems:

-- eval (current-form): .tables -- noqa
-- --------------------------------------------------------------------------------
-- eval (current-form): .tables
crime_scene_report      get_fit_now_check_in    interview             
drivers_license         get_fit_now_member      person                
facebook_event_checkin  income                  solution              

In this case I'm using SQLFluff via nvim-lint and trying to suppress an error using a noqa comment.

[Olical]

What's the problem here specifically? The output looks correct to me?

…

On Fri, 4 Oct 2024, 19:28 Alex Sheluchin, ***@***.***> wrote: Found one more edge case. Looks like inline comments cause problems: -- eval (current-form): .tables -- noqa -- -------------------------------------------------------------------------------- -- eval (current-form): .tables crime_scene_report get_fit_now_check_in interview drivers_license get_fit_now_member person facebook_event_checkin income solution In this case I'm using SQLFluff via nvim-lint and trying to suppress an error using a noqa comment <https://docs.sqlfluff.com/en/stable/configuration/ignoring_configuration.html#ignoring-individual-lines> . — Reply to this email directly, view it on GitHub <#611 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACM6XLBIABHGRUF4HG3R7TZZ3M5RAVCNFSM6AAAAABPKJIT6GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJUGMZTOMZVGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[sheluchin]

@Olical this might be getting a little too far into the weeds, but I wanted to document it anyhow.

In the snippet you will see that I eval'd two forms. The first one .tables -- noqa is a metacmd followed by a comment to keep my linter happy. It did not produce any evaluation result, but the behaviour is consistent with what you'd see using the duckdb CLI (ie you can't include a comment with a metacmd). The second eval omits the noqa comment and works as expected.

I don't know what would make for the best UX around this. On the one hand, you would probably never keep a metacmd in a SQL file so the linter should complain. On the other hand, it's reasonable to temporarily put these metacmds in SQL files as part of the REPL workflow, and in this case it would be nice to avoid getting linter warnings for stuff you've done intentionally. I think trying to mirror the behaviour of the duckdb CLI here might not be ideal. Maybe it should be possible to eval a metacmd form with a comment, and have the comment internally stripped out? Like I said, getting a bit far into the weeds with this one.

[russtoku]

For metacommands, I put them in comments in a SQL file. Then select the part that I want to send to the REPL. Would that work for @sheluchin?

[sheluchin]

@russtoku yep, that's what I've arrived at after a bit of tinkering. Seems like a reasonable compromise which lets users work with the metacmds but doesn't require adding too much complexity to the Conjure code base. Thank you!

[russtoku]

I've tested these changes with PostgreSQL and duckdb using dev/sql/sandbox.sql. Things are working as expected.

There is a quirk with duckdb:
Given these commands in a SQL buffer:

show;
.tables

,ee doesn't work on both commands but selecting a command and using ,E works on both.

But if the order is reversed:

.tables
show;

,ee works on both.

show; is a short-hand for show all tables; so either work.

FYI: It doesn't matter what g:conjure#client#sql#stdio#prompt_pattern is set to, the SQL client works. It can be ignored.

[Olical]

Pushed a change to main that strips trailing comments from meta commands. So that should solve @sheluchin's issue although moving them to their own file and selecting them is also good.

I think I'll just have to pass on the issue @russtoku pointed out though. That one I think will require much more work to solve, so far I've gone for 70% value for 10% effort which I think worked quite well. That one I think will require the SQL tree-sitter module to properly handle and tag meta commands. If it ever does support them properly (maybe that's impossible?) then Conjure should just keep working but improve and we can remove these... hacks?

So far I'm pretty happy with the added robustness. It's not perfect, but it's a lot better than it was.