-
Notifications
You must be signed in to change notification settings - Fork 164
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 history backend #131
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had difficulty fitting it into the existing history traits since they seem to want a file backed history and the database trait wants to be async.
Is there any particular reason for database trait being async? I mean would the performance gains be worth the mental overhead to think async
?
we'd use the current history trait so that when you hit up arrow/traverse the history, it does some type of select from the database and returns a HistoryItem or Vec
Any examples of what all fields we will use in engine from this HistoryItem
structure? I mean from a usage standpoint I can only speculate what all you have in mind. If you can share some thoughts on what features you had in mind around this, that would be great.
Some of the use cases that I can envision include
- Being able to run history queries like
<command> b/w -7d to -14d
(where the user is like I am sure this is in the history somewhere around last week or something. Obviously, this is mere speculation so please don't really focus on the syntactic aspect - which can be improved) jump
/z
/etc like autocompletion based on all the available history data.- based on previous error codes we highlight the command as
red
(?) - etc. (I would like to know what all you were planning on)
We can always modify the current implementation of FileBackHistory to use a common trait/same struct. Once we agree on something we can modify it or we can simply go with a trait that currently provides a string command and use that and then postpone this thinking.
use std::str::FromStr; | ||
|
||
#[async_trait] | ||
pub trait Database { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like to have queries all start with get. It just plays nicer with autocomplete etc.
load
-> get_by_id
range
-> get_in_range
first
-> get_first
etc....
The usage will look like this
let command_repo = CommandRepo::new();
let command = command_repo.get_by_id(...)?;
or something similar. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya, sure. sounds good to me. i'd love to standardize things. i'm not sure if this is a @jntrnr call or not. 1/2 of me thinks JT wouldn't care, the other 1/2 of me thinks JT may be particular about function names. Either way, I'm cool with whatever.
&self, | ||
limit: Option<i64>, | ||
search_mode: SearchMode, | ||
query: &str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use some help understanding what this query is. Other info is pretty clear from the type info but I am not so sure about query
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mean to be a broken record here but all of the database code is best understood by looking at the database sample app I wrote called hiztery mentioned above. Here's how it's implemented in that code base.
Some(HizteryCmd::Search {
search_mode,
limit,
query,
}) => {
// cargo run -- search -m "p" -q "code"
debug!(
"Searching with phrase: {}, limit: {:?}, mode: {}",
&query, limit, &search_mode
);
let s_mode = match search_mode.as_ref() {
"p" => SearchMode::Prefix,
"f" => SearchMode::FullText,
"z" => SearchMode::Fuzzy,
_ => SearchMode::FullText,
};
let result = sqlite.search(limit, s_mode, &query).await;
match result {
Ok(r) => {
debug!("Found {} hits", r.len());
for (idx, hit) in r.iter().enumerate() {
debug!("Hit # [{}] History: [{}]", idx + 1, hit.command);
}
}
_ => debug!("No hits found for phrase: {}", &query),
}
}
Ok(Self { pool, pid }) | ||
} | ||
|
||
async fn setup_db(pool: &SqlitePool) -> Result<(), sqlx::Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have pool
as an instance variable for this struct. Right? So why do we need to inject this in the function too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever you think is best. Just a lot of copy-n-pasting.
Ok(()) | ||
} | ||
|
||
fn convert_time(h: &HistoryItem) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain what this function is for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function isn't used yet but I mocked it up so that it could be used to translate from timestamp_nanos to print out human readable times. This is how save stores datetime.
async fn save_raw(
tx: &mut sqlx::Transaction<'_, sqlx::Sqlite>,
h: &HistoryItem,
) -> Result<(), sqlx::Error> {
// We don't need the history_id here because it's an auto number field
// so it should be ever increasing
sqlx::query(
"insert or ignore into history_items(timestamp, duration, exit_status, command, cwd, session_id)
values(?1, ?2, ?3, ?4, ?5, ?6)",
)
.bind(h.timestamp.timestamp_nanos())
.bind(h.duration)
.bind(h.exit_status)
.bind(h.command.as_str())
.bind(h.cwd.as_str())
.bind(h.session_id)
.execute(tx)
.await?;
Ok(())
}
search_mode: SearchMode, | ||
query: &str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we merge these together? Won't enums allow for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, sure. Feel free to rework it if you like. I'm up for anything really. I just want to keep the power of sqlite in here.
because the database.rs file was ripped off from atuin and i just left it. :)
the comments above in the
i think that would be good but it would be nice if it were maybe Generics so people can implement it how they want. My sticking point was the current one is so |
wow! thank for all these great questions! this is awesome! I'm really just trying to plant the seed here so reedline can do awesome things with a sqlite backed history. i encourage you to work on this, up to and including taking ownership of it and make any changes that seem appropriate. i'm down with whatever the team wants to do. i don't have the rust skillz that ya'll do. |
Some quick thoughts - I really like the idea of using sqlite as our history backend. There seems to be a ton of potential here not just for showing the history, but for filtering it and using it in fun ways like atuin does. I wanted to give it a couple days on the On a related note, with this new functionality, feels like we should definitely start brainstorming how we want to add UI elements in reedline. The nice thing about using crossterm is that we, in theory, have a pretty flexible terminal interface to work with. So if we wanted we should be able to draw popups and whatnot. There are also TUI libraries that work with crossterm (this one comes to mind but I think there are others, too) |
If anyone is still watching this dead PR, I created a new demo app here that uses the same database trait and sqlite struct. It should be enough to review this PR or create a new one. |
Since i almost didn't find this code I just want to link this related issue in nushell: nushell/nushell#3364 as well as |
unfortunately, skim isn't cross platform due to the use of termion. but still the screenshots are cool. i really like the idea of "session or directory or host or everywhere" type of searching history. and being the nerd that i am, i love the stats on the right. thanks for adding this. i think we're getting closer to starting on this. there are people who want to use and developers who want to write it. :) |
Ah, I didn't know about that. The relevant issue is here, I guess: skim-rs/skim#293
I'm taking a bit of a stab at it right now, let me know if there's anything specific I should consider. I'm doing it with rusqlite (to avoid async) and my plan is to use a db structure similar to histdb. |
This is a first stab at a MVP that uses a sqlite backend for history. This code compiles, however I have not run it even once. I had difficulty fitting it into the existing history traits since they seem to want a file backed history and the database trait wants to be async. Good work to the team so far on making history work so well. So, I shoe-horned this code into a pseudo filebacked/sqlbacked history just to get it to compile. It's not how I'd want it to run and I don't like it in its current state but it gets some points across about how awesome we could make the history.
Ideally, we'd use the current history trait so that when you hit up arrow/traverse the history, it does some type of select from the database and returns a HistoryItem or Vec. Right now, it's just creating a database, loading the commands into a vec and ignoring the rest of the HistoryItem.
Another thought I had is, that it would be nice to have so caching perhaps. So, ever navigation doesn't require a select. I'm not sure that's viable, but it's worth thinking about.
If you want to play around with the sqlite prototype app, feel free to clone it from here https://github.com/fdncred/hiztery. All the functions of the Database trait are implemented in this test app.
I'm hoping for people to jump in and make this work as an alternate history backend because there is a lot of potential.
Here's an attempt at some rudimentary documentation.
Each row in the database follow this struct. There are some todo comments about potentially adding other columns that may come in handy later.
All the database interaction happens in the database.rs file. Here's the db trait.
it also supports these types of searches. see the hiztery repo for how these search modes work.
credit goes to atuin where most of the db code came from.
Just thinking outloud here, it would be nice if the History traits used generics so we don't always have to return a String for the command. we could return a HistoryItem or whatever 3rd parties want to return. Same for some of the input parameters. On some of the History traits I'd like to pass in a HistoryItem.