Try to hack around a breaking change in diesel 2.1

[weiznich]

cc @omid

That change is not complete yet. It misses:

• A regression test for the now broken variant to verify that it actually works and to prevent breaking it again
• Figuring out to support the DISTINCT ON (name, id) … ORDER BY name ASC, id DESC cases.

[omid]

I don't have access to the branch to apply the tests, anyway, here are the tests, for the distinct.rs file. It fails in the first and third cases in this test, but it's not a big deal, we can mention this as a limitation and remove them from the test:

#[cfg(feature = "postgres")]
#[test]
fn distinct_on_sql_literal() {
    use crate::schema::users::dsl::*;

    let connection = &mut connection();
    diesel::sql_query(
        "INSERT INTO users (name, hair_color) VALUES ('Sean', 'black'), ('Sean', NULL), ('Tess', NULL), ('Tess', NULL)",
    ).execute(connection)
        .unwrap();

    let expected_data = vec![
        NewUser::new("Sean", Some("black")),
        NewUser::new("Tess", None),
    ];

    // order by with sql literal
    let data: Vec<_> = users
        .select(NewUser::as_select())
        .order(dsl::sql::<sql_types::Bool>("name"))
        .distinct_on(name)
        .load(connection)
        .unwrap();
    assert_eq!(expected_data, data);

    // order by and distinct on with sql literal
    let data: Vec<_> = users
        .select(NewUser::as_select())
        .order(dsl::sql::<sql_types::Bool>("name"))
        .distinct_on(dsl::sql::<sql_types::Bool>("name"))
        .load(connection)
        .unwrap();
    assert_eq!(expected_data, data);

    // distinct on with sql literal
    let data: Vec<_> = users
        .select(NewUser::as_select())
        .order(name)
        .distinct_on(dsl::sql::<sql_types::Bool>("name"))
        .load(connection).unwrap();
    assert_eq!(expected_data, data);
}

I also added the failed test case to distinct_of_multiple_columns:

    // same amount of order by and distinct on
    // including asc and desc
    let data = posts::table
        .inner_join(users::table)
        .order((users::id.asc(), posts::body.desc()))
        .distinct_on((users::id, posts::body))
        .load(&mut connection);
    let expected = vec![
        (posts[1].clone(), sean.clone()),
        (posts[0].clone(), sean),
        (posts[5].clone(), tess.clone()),
        (posts[4].clone(), tess),
    ];

    assert_eq!(Ok(expected), data);

[weiznich]

@omid Thanks for providing these test cases. I've checked them and only the following is accepted on diesel 2.0.4:

  // order by and distinct on with sql literal
    let data: Vec<_> = users
        .select(NewUser::as_select())
        .order(dsl::sql::<sql_types::Bool>("name"))
        .distinct_on(dsl::sql::<sql_types::Bool>("name"))
        .load(connection)
        .unwrap();
    assert_eq!(expected_data, data);

The other two cases were not accepted before. I will try and see if I find a good solution to add support for that case back.

[dsp]

What's left here to do? I am happy to give it a shot, to see that we can get closer to a patch release for diesel out

[weiznich]

What's left here to do? I am happy to give it a shot, to see that we can get closer to a patch release for diesel out

The open work here is figuring out how to get all the compile tests passing. That requires changing these impls to cover the .order_by((column_a.desc(), column_b.acs()) case as well without running into an overlapping trait impl issue. Help here is welcome, but I guess that this is a pretty hard issue for someone that is not familiar with diesels internals.

[dsp]

Okay, I got to the point where i understand the problem and the current implementation as well as the issue with overlapping implementations. I'll see if i can find the time to dig into this, since this seem rather non-trivial.

[weiznich]

I think I've found a solution for the overlapping impl problem: We need to generate more target implementations. Essentially permutations like these:

impl<T1, T2> ValidOrderingForDistinct<DistinctOnClause<(T1, T2)>> for OrderClause<(crate::expression::operators::Asc<T1>, crate::expression::operators::Asc<T2>)> {}
impl<T1, T2> ValidOrderingForDistinct<DistinctOnClause<(T1, T2)>> for OrderClause<(crate::expression::operators::Asc<T1>, crate::expression::operators::Desc<T2>)> {}
impl<T1, T2> ValidOrderingForDistinct<DistinctOnClause<(T1, T2)>> for OrderClause<(crate::expression::operators::Desc<T1>, crate::expression::operators::Asc<T2>)> {}
impl<T1, T2> ValidOrderingForDistinct<DistinctOnClause<(T1, T2)>> for OrderClause<(crate::expression::operators::Desc<T1>, crate::expression::operators::Desc<T2>)> {}
impl<T1, T2> ValidOrderingForDistinct<DistinctOnClause<(T1, T2)>> for OrderClause<(T1, crate::expression::operators::Desc<T2>)> {}
impl<T1, T2> ValidOrderingForDistinct<DistinctOnClause<(T1, T2)>> for OrderClause<(T1, crate::expression::operators::Asc<T2>)> {}
impl<T1, T2> ValidOrderingForDistinct<DistinctOnClause<(T1, T2)>> for OrderClause<(crate::expression::operators::Desc<T1>, T2)> {}
impl<T1, T2> ValidOrderingForDistinct<DistinctOnClause<(T1, T2)>> for OrderClause<(crate::expression::operators::Asc<T1>, T2)> {}

need to be generate for all supported tuple sizes. I will try to integrate that into the relevant macro next time I have some spare time. If someone else want's to try that please go on and let me know the results.