Thoughts on a new AsFilter trait/derive macro?

[demhadais]

Backend

PostgreSQL, MySQL, SQLite

What do you want to add to diesel?

Recently, the fantastic HasQuery was added to diesel, which improves query ergonomics a lot. I've been thinking about a "sibling" trait (potentially called AsFilter) that could be used like so:

use diesel::prelude::*;

diesel::table! {
    persons(id) {
        id -> Integer,
        name -> Text,
        age -> Integer
    }
}

#[derive(HasQuery)]
struct Person {
    id: i32,
    name: String,
    age: i32,
}

#[derive(AsFilter)]
struct PersonFilter {
    #[diesel(persons::name.eq)]
    name: Option<String>,
    #[diesel(persons::age.eq)]
    age: Option<i32>,
}

fn fetch_people(filter: PersonFilter, db_conn: &mut diesel::pg::PgConnection) -> Vec<Person> {
    let mut query = Person::query().filter(filter.as_filter());

    query.load(db_conn).unwrap()
}

Implementation notes

Here's something I already use in my own code, which could probably be turned into a derive macro with some work (above code omitted):

type BoxedFilter<'a, QS> = Box<dyn BoxableExpression<QS, Pg, SqlType = Bool> + 'a>;

trait AsFilter<'a, QS> {
    fn as_filter(&'a self) -> Option<BoxedFilter<'a, QS>>;
}

impl<'a, QS: 'a> AsFilter<'a, QS> for PersonFilter
where
    persons::name: SelectableExpression<QS>,
    persons::age: SelectableExpression<QS>,
{
    fn as_filter(&'a self) -> Option<BoxedFilter<'a, QS>> {
        let mut filter: Option<BoxedFilter<QS>> = None;

        if let Some(name) = &self.name {
            filter = Some(Box::new(persons::name.eq(name)));
        }

        filter = match (filter, self.age) {
            (Some(f), Some(age)) => Some(Box::new(f.and(persons::age.eq(age)))),
            (None, Some(age)) => Some(Box::new(persons::age.eq(age))),
            (f, None) => f,
        };

        filter
    }
}

Additional details

My only concern is performance. I tried to dig into how diesel handles boxed queries to see if it's reboxing the query the way my AsFilter trait does, but I couldn't really tell. If the current way (query.into_boxed) is the same in number of heap allocations as my AsFilter, then it should be fine. There's also a possibility for optimization if the filter structure has no optional fields - then, only one box is necessary (or we split this into two traits - AsFilter and AsBoxedFilter).

Anyways, if anyone has thoughts as to whether it's a worthwhile pursuit, I'm happy to hear them!

Checklist

• I have already looked over the issue tracker and the discussion forum for similar possible closed issues.

[weiznich]

Thanks for writing this proposal. I can see why such a derive would look like a good extension, but I fear that it's not as easy as that. Different users will have widely different requirements on how exactly that derive is supposed to work. You want to be able to specify the operator as part of the attribute, while others likely want to have the operator flexible as well. Technically that's likely still all solvable but comes with a lot of complexity. Given that I would prefer to see some out of tree experimentation in a third party crate first. If that crate is reasonable popular we can still talk about merging it back to diesel itself.

---

My only concern is performance. I tried to dig into how diesel handles boxed queries to see if it's reboxing the query the way my AsFilter trait does, but I couldn't really tell. If the current way (query.into_boxed) is the same in number of heap allocations as my AsFilter, then it should be fine. There's also a possibility for optimization if the filter structure has no optional fields - then, only one box is necessary (or we split this into two traits - AsFilter and AsBoxedFilter).

I don't think that's something you would need to care that much about. Allocations are an order of magnitude cheaper than network IO, so this will be all only noise compared to the actual query runtime due to the need to perform network IO for most of the backends. The only exception there is the SQLite backend. There allocations can matter, but only in hot inner loops, so even there this is still likely irrelevant.

[demhadais]

Thanks for your reply. That makes sense to me! If I find time, I'll begin working on a separate crate. Also, what you said about allocations definitely makes sense. I don't think I phrased my concern correctly, it was more about user expectations. If I was using this derive, I'd expect that these two functions have roughly the same number of allocations (regardless of the number of fields I'm filtering on):

fn fetch_people(filter: PersonFilter) {
    let query = Person::query().into_boxed();

    if let Some(name) = filter.name {
        query.filter(persons::name.eq(name));
    }
}

fn fetch_people_derive(filter: PersonFilter) {
    let query = Person::query().filter(filter.as_filter());
}

Realistically, even if they don't have the same number of allocations, that's something I could easily document. I think I'd just want to know how many reallocations the first function is likely to produce so I could make a somewhat precise comparison.

Anyways, thanks for your help!