implement LoadQuery for batch insert statements for sqlite connection

[bwqr]

This is a draft PR requesting for comments. It allows running insert statements with returning clause for sqlite connection. There are a few remaining tasks, which needs a little bit guidance, to consider this PR as ready:

• Find an appropriate Iterator implementation to return from internal_load (instead of currently used boxed iteration).
• Implement LoadQuery for Yes case which requires running multiple insert statements. I can implement this if No case looks good.
• Consider adding tests or examples.
• Consider using a generic connection with backend Sqlite (similar to ExecuteDsl implementation). I tried to do this but it caused conflicting implementation error. It looks like trait bounds resolution is being improved with newer version of Rust compiler. In the future, it might be possible to use a generic connection bounded by Sqlite backend. For now, we can continue using directly SqliteConnection.
• Bump MSRV to 1.84 because of compilation errors caused by conflicting implementation error. Check this and this messages for more information

With this PR, following example should compile successfully:

use diesel::prelude::{ExpressionMethods, Queryable};
use diesel::{Connection, RunQueryDsl, SqliteConnection};

diesel::table! {
    apps (id) {
        id -> Int4,
        #[max_length = 8]
        name -> Varchar,
        #[max_length = 8]
        state -> Varchar,
    }
}

#[derive(Queryable)]
struct App {
    id: i32,
    name: String,
    state: String,
}

fn main() {
    let mut conn = SqliteConnection::establish(":memory:").unwrap();

    let query = diesel::insert_into(apps::table).values(vec![
        (apps::name.eq("Good"), apps::state.eq("Running")),
        (apps::name.eq("Bad"), apps::state.eq("Failed")),
    ]);

    query.get_results::<App>(&mut conn);
}

[weiznich]

Thanks for opening this PR. There seems to be still a failing CI job here: https://github.com/diesel-rs/diesel/actions/runs/15237082999/job/42852408104?pr=4616. I did not look into the details yet, but that's something that needs to be addressed.

Find an appropriate Iterator implementation to return from internal_load (instead of currently used boxed iteration).

I would suggest to return a "private" wrapper type that implements Iterator, that would enable us to change the details later on as users cannot do anything other than iterating with that type. Essentially what we do here, which puts the type in a private module so that users cannot name it in their code. Ideally that wouldn't use a boxed iterator as inner type, but if that's not easily possible that's not that bad. We can change it later with that setup if it turns out to be problematic.

Consider adding tests or examples.

That would be required.

[bwqr]

Thank you for your feedback.

There seems to be still a failing CI job here: https://github.com/diesel-rs/diesel/actions/runs/15237082999/job/42852408104?pr=4616. I did not look into the details yet, but that's something that needs to be addressed.

For some reason, the same code causes conflicting implementation for 'InsertStatement<_, BatchInsert<Vec<insert_statement::ValuesClause<_, _>>, _, _, _>, _>' when it is compiled with rust 1.82 compiler. This error confused me a little bit since other checks pass successfully. I will investigate this by trying to compile the code locally with 1.82.

I would suggest to return a "private" wrapper type that implements Iterator, that would enable us to change the details later on as users cannot do anything other than iterating with that type. Essentially what we do here, which puts the type in a private module so that users cannot name it in their code.

I was hoping to use an already defined iterator instead of defining a new one, such as the LoadIter you have linked. However, since LoadIter is private to its module, I am not able to use it inside the insert_with_default_for_sqlite.rs. Should I define a new iterator similar to LoadIter or should we export LoadIter with pub(crate) to prevent duplication?

I will try to write some tests. Until completing all the tasks, I think converting this PR to draft might be the best.

[weiznich]

For some reason, the same code causes conflicting implementation for 'InsertStatement<, BatchInsert<Vec<insert_statement::ValuesClause<, _>>, _, _, _>, _>' when it is compiled with rust 1.82 compiler. This error confused me a little bit since other checks pass successfully. I will investigate this by trying to compile the code locally with 1.82.

I'm not sure if it's related to 1.82. It might also be due to building with --features "sqlite mysql postgres extras", which is not done by the other CI builds. If its related to the rust version we can just bump the MSRV to something that accepts this, this would be fine.

I was hoping to use an already defined iterator instead of defining a new one, such as the LoadIter you have linked. However, since LoadIter is private to its module, I am not able to use it inside the insert_with_default_for_sqlite.rs. Should I define a new iterator similar to LoadIter or should we export LoadIter with pub(crate) to prevent duplication?

If LoadIter is fitting it would be good to use it. It's no problem to make it pub(crate), public. We just don't want to have it in our public API.

[bwqr]

I'm not sure if it's related to 1.82. It might also be due to building with --features "sqlite mysql postgres extras", which is not done by the other CI builds. If its related to the rust version we can just bump the MSRV to something that accepts this, this would be fine.

I just checked this issue by compiling this PR with a few rust releases with --features "sqlite returning_clauses_for_sqlite_3_35". Here are the results:

• 1.81: Did not compile due to extern block cannot be declared unsafe used in libsqlite3-sys crate. I guess this was stabilized in 1.82.
• 1.82: Causes conflicting implementation ❌
• 1.83: Causes conflicting implementation ❌
• 1.84: Compiles fine ✅
• 1.87: Compiles fine ✅

If LoadIter is fitting it would be good to use it. It's no problem to make it pub(crate), public. We just don't want to have it in our public API.

Nice, I will check whether LoadIter is useful for this implementation.

[weiznich]

I did a quick search which rustc change did introduce that changed behavior just to make sure that this is not by accident and that pointed to rust-lang/rust#130654, which seems to be reasonable. So I suggest to just bump the minimal supported rust version to 1.84. That needs to be done here, here and here.

[bwqr]

I believe this PR is ready for the review :). I would like to thank you for all your guidance and feedback.

[weiznich]

Thanks for the update. This looks already really good in terms of implementation.
I added another bunch of mostly minor comments about some details that need to be adjusted. As soon as those are fixed, this should be fine to be merged.

[weiznich]

Would it be possible to skip the collect here and return the iterator directly?

[weiznich]

We might also want to mention the new feature in the Changelog 😉

[weiznich]

We also want to unwrap the inner result to stop the transaction from being comited in the error test. We likely want to have a test case for this (Just insert a reasonable item + a duplicated item and make sure that none are in the database)

Suggested change

	results.push(result);
	results.push(Ok(result?));

(If we would like to be even more correct: We should roll back the transaction for anything but deserialization errors to perfectly mirror the batch variant)

There is a test for batch inserts without returning here:

diesel/diesel_tests/tests/insert.rs

Line 821 in efcb1e7

fn batch_insert_is_atomic_on_sqlite() {

You likely can just copy and adjust that test for insert + returning and upserts.

Otherwise it sounds like a good idea to quickly go over the tests cfg out for sqlite in that file and check which ones can now be enabled.

[bwqr]

Thank you for pointing this out. I updated code to bubble up errors earlier other than deserialization one as you said. And added a few case to test this. Please do not hesitate to suggest any other improvement for both code and tests (current implementation does a match on the result but there might be a better way to do it).

[weiznich]

Same as above

[weiznich]

Perfect, that's really nice 🎉

[bwqr]

Indeed 😃 , but I found another way to run the query, which eliminated using LoadIter explicitly. I reverted changes related to making LoadIter pub(crate).

[weiznich]

Looks good now, thanks for updating.

(I just pushed a changelog entry, otherwise this is now ready to be merged)