diesel icon indicating copy to clipboard operation
diesel copied to clipboard

Aliases with group_by and select

Open mlesin opened this issue 2 years ago • 3 comments

Setup

Versions

  • Rust: rustc 1.63.0 (4b91a6ea7 2022-08-08)
  • Diesel: 2.0.0
  • Database: PostgreSQL 13
  • Operating System Linux (Ubuntu 22.04)

Feature Flags

  • diesel: postgres, uuid, chrono

Problem Description

I'm trying to add second alias to query with left_join and failing. I have the following code which compiles fine:

let leafs = diesel::alias!(chunks as leafs);
let mut query = chunks::table
    .left_join(
        leafs.on(chunks::path
            .contains(leafs.field(chunks::path))
            .and(leafs.field(chunks::nchildren).eq(0))),
    )
    .group_by(chunks::id)
    .select((
        chunks::all_columns,
        sum(leafs.field(chunks::estimated).nullable()),
    ))
    .into_boxed();

But if I'm converting it to this, it fails to compile:

let (c, leafs) = diesel::alias!(chunks as c, chunks as leafs);
let mut query = c
    .left_join(
        leafs.on(c
            .field(chunks::path)
            .contains(leafs.field(chunks::path))
            .and(leafs.field(chunks::nchildren).eq(0))),
    )
    .group_by(c.field(chunks::id))
    .select((
        c.fields(chunks::all_columns),
        sum(leafs.field(chunks::estimated).nullable()),
    ))
    .into_boxed();

What are you trying to accomplish?

I'm trying to add alias for main self-referencing table to make query more complex later

What is the expected output?

It should compile :)

What is the actual output?

60 |             .select((
   |              ^^^^^^ the trait `ValidGrouping<AliasedField<c, chunks::columns::id>>` is not implemented for `AliasedField<c, chunks::columns::parent_id>`
60 |             .select((
   |              ^^^^^^ the trait `ValidGrouping<AliasedField<c, chunks::columns::id>>` is not implemented for `AliasedField<c, chunks::columns::path>`
60 |             .select((
   |              ^^^^^^ the trait `ValidGrouping<AliasedField<c, chunks::columns::id>>` is not implemented for `AliasedField<c, chunks::columns::label>`
... and so on for each field in chunk model

Are you seeing any additional errors?

Steps to reproduce

corresponding model is the following:

diesel::table! {
    use diesel::sql_types::*;
    use diesel_ltree::sql_types::*;

    chunks (id) {
        id -> Int8,
        parent_id -> Nullable<Int8>,
        path -> Ltree,
        label -> Text,
        estimated -> Int4,
        nchildren -> Int4,
    }
}

Checklist

  • [x] This issue can be reproduced on Rust's stable channel. (Your issue will be closed if this is not the case)
  • [x] This issue can be reproduced without requiring a third party crate

mlesin avatar Sep 13 '22 08:09 mlesin

Thanks for reporting this bug. It seems like that this impl is not general enough. Likely it must be change in such a way that it accepts all combinations of AliasedFields that implement ValidGrouping on their own. https://github.com/diesel-rs/diesel/blob/9502a9fd3df61bcfd59c63fecb79e242802e99d6/diesel/src/query_source/aliasing/aliased_field.rs#L80-L86 PR's fixing this are welcome.

As workaround you can list all columns explicitly in your group_by clause.

weiznich avatar Sep 19 '22 14:09 weiznich

Thanks for explanation! I'll try to create a PR, but I'm afraid I'm not competent enough in Rust to do it right and not break anything else :)

mlesin avatar Sep 20 '22 09:09 mlesin

@mlesin What needs to be done is the following:

  • The linked impl needs to be changed to the following:
impl<S, C1, C2> ValidGrouping<AliasField<S, C1>> for AliasedField<S, C2>
where 
    S: AliasSource, 
    C1: Column<Table = S::Target>
    C2: Column<Table = S::Target>
    C2: ValidGrouping<C1, IsAggregate = is_aggregate::Yes>
{
    type IsAggregate = is_aggregate::Yes; 
}
  • Add a test based on your example to check that the code is compiling
  • Open a PR with this changes

weiznich avatar Sep 21 '22 18:09 weiznich

Thank you very much for fixing this! Unfortunately I didn't have time to try to do it myself after your wonderful explanation, sorry :(

mlesin avatar Sep 26 '22 18:09 mlesin