Fix offset needing a limit clause (on sqlite)

[weiznich]

Just opening this one to get some feedback on the chosen approach

This should ~work but has one downside: Every code that is generic over connection types/backends now requires bounds like SelectStatement<…>: QueryFragment<DB> (For concrete examples see the change in diesel_migrations). That's quite unfortunate in my opinion, but I've found no better way to solve this without falling back to unstable features like specialization

[weiznich]

That needs to be move to somewhere else

[weiznich]

I've just want to get the normal SelectStatement working before I will fix that.

[sgrif]

Ugh I was hoping LIMIT -1 was specified in ISO SQL so we wouldn't need to special case this...

[sgrif]

Only other approach I can think of would be this:

impl<DB: Backend + SupportsLonelyOffset> QueryFragment<DB> for NoLimitClause {
    fn walk_ast(...) -> ... {
         Ok(())
    }
}

impl QueryFragment<Sqlite> for NoLimitClause {
    fn walk_ast(...) -> ... {
        out.push_sql("LIMIT -1");
        Ok(())
    }
}

Honestly though, having LimitOffsetClause makes sense. I'm wondering if the right move here is to remove SupportsLonelyOffset, and instead require all backends to provide an explicit impl. This seems like the place that backends diverge the most (can offset be used w/o limit? Do they do limit/offset, or ISO standard offset/fetch first?) Maybe something like this?

pub struct LimitOffsetClause<LimitClause, OffsetClause> {
    pub limit_clause: LimitClause,
    pub offset_clause: OffsetClause,
}

impl<Lim, Off> QueryFragment<Pg> for LimitOffsetClause<Lim, Off>
where
    Lim: QueryFragment<Pg>,
    Off: QueryFragment<Pg>,
{
    fn walk_ast(...) -> ... {
        self.limit_clause.walk_ast(out.reborrow())?;
        self.offset_clause.walk_ast(out.reborrow())?;
        Ok(())
    }
}

// Identical impl for MySQL

impl<Off> QueryFragment<Sqlite> for LimitOffsetClause<NoLimitClause, OffsetClause<Off>>
where
    OffsetClause<Off>: QueryFragment<Sqlite>,
{
    fn walk_ast(...) -> ... {
        out.push_sql(" LIMIT -1 ");
        self.offset_clause.walk_ast(out.reborrow())?;
        Ok(())
    }
}

// explicit impls for `LimitClause<Lim>, AnyOffsetClause` and `NoLimitClause, NoOffsetClause`

[weiznich]

Turns out that mysql does also not support offset without a preceding limit clause. Their "workaround" is even more horrible:

To retrieve all rows from a certain offset up to the end of the result set, you can use some large number for the second parameter.

[sgrif]

Uh... I guess we can just not support this on MySQL?

…

On Tue, Apr 9, 2019, 11:43 AM Georg Semmler ***@***.***> wrote: Turns out that mysql does also not support offset without a preceding limit clause <https://dev.mysql.com/doc/refman/8.0/en/select.html>. Their "workaround" is even more horrible: To retrieve all rows from a certain offset up to the end of the result set, you can use some large number for the second parameter. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#2029 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdWK2ZvuRK5hktzJ0sRdvfsoM85OEI6ks5vfMNDgaJpZM4cdwiW> .

[weiznich]

That would be a solution.
Otherwise maybe just add a #[deprecated (note = "MySql is broken, use postgres instead")] to the mysql backend 😋

[weiznich]

@diesel-rs/contributors This could need a final review.

[weiznich]

So I've tried to port the diesel-oci backend to the latest diesel master branch (including this PR). It turns out that some changes are required to make this work: Basically third party backends need to see things and need to be able to implement certain traits.

[weiznich]

We may want to use this trait for the other conversations as well in a future change, but not sure about that