New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set up the groundwork for multi-table joins #928

Merged
merged 2 commits into from Jun 5, 2017

Conversation

Projects
None yet
3 participants
@sgrif
Member

sgrif commented Jun 1, 2017

This solves the fundamental problem of needing
SelectableExpression<Join<Left, Right>> for column to work when we
don't know where the table concretely appears, just that it appears
somewhere. With this commit, the logic is now that a column can be
selected from any join where its table appears within the join exactly
once. This disallows users.inner_join(posts.inner_join(users)), which
we don't want to allow as it would require the second instance of
users to be aliased.

This does require that there is an impl ContainsTable<left> for right
for every two tables that would appear. Right now I'm just brute-forcing
these when the association between the two tables is defined. For
multi-table joins to work when two of the tables in the join do not have
an association, this will require a manual impl. We'll probably provide
something like enable_multi_table_joins!(users, comments) or similar.
This may go away eventually with specialization, if projections on
default associated types are eventually allowed in the fully monomorphic
case.

It should be noted that this commit does not enable multi-table joins on
itself. Join still does not implement JoinDsl, and tables do not
implement JoinTo<Join<Mid, Right>>. The code required to enable that
is pretty minimal, but I don't want to flip the switch just yet, as
we're actually generating invalid SQL at the moment and that needs to be
resolved.

However, with this change, all of the limitations that prevent
multi-table joins at the type level are eliminated.

@sgrif sgrif requested review from killercup and Eijebong Jun 1, 2017

sgrif added some commits Jun 1, 2017

Set up the groundwork for multi-table joins
This solves the fundamental problem of needing
`SelectableExpression<Join<Left, Right>> for column` to work when we
don't know where the table concretely appears, just that it appears
somewhere. With this commit, the logic is now that a column can be
selected from any join where its table appears within the join exactly
once. This disallows `users.inner_join(posts.inner_join(users))`, which
we don't want to allow as it would require the second instance of
`users` to be aliased.

This does require that there is an `impl ContainsTable<left> for right`
for every two tables that would appear. Right now I'm just brute-forcing
these when the association between the two tables is defined. For
multi-table joins to work when two of the tables in the join do not have
an association, this will require a manual impl. We'll probably provide
something like `enable_multi_table_joins!(users, comments)` or similar.
This *may* go away eventually with specialization, if projections on
default associated types are eventually allowed in the fully monomorphic
case.

It should be noted that this commit does not enable multi-table joins on
itself. `Join` still does not implement `JoinDsl`, and tables do not
implement `JoinTo<Join<Mid, Right>>`. The code required to enable that
is pretty minimal, but I don't want to flip the switch just yet, as
we're actually generating invalid SQL at the moment and that needs to be
resolved.

However, with this change, all of the limitations that prevent
multi-table joins at the type level are eliminated.
Fix failing compile tests
I've also renamed `ContainsTable` to `AppearsInFromClause`, as the error
message complaining that `users: AppearsInFromClause<posts>` is not
satisfied is much clearer.
@Eijebong

This looks good overall except those cryptic error messages. I'm not sure we can improve them though.

@@ -27,24 +27,24 @@ macro_rules! __diesel_column {
}
impl<QS> AppearsOnTable<QS> for $column_name where
QS: ContainsTable<$($table)::*, Count=Once>,
QS: AppearsInFromClause<$($table)::*, Count=Once>,

This comment has been minimized.

@Eijebong

Eijebong Jun 5, 2017

Member

Yeah, this is much more readable

@Eijebong

Eijebong Jun 5, 2017

Member

Yeah, this is much more readable

@@ -27,13 +27,17 @@ fn main() {
let join = users::table.left_outer_join(posts::table);
// Invalid, only Nullable<title> is selectable
let _ = join.select(posts::title); //~ ERROR E0277
let _ = join.select(posts::title);

This comment has been minimized.

@Eijebong

Eijebong Jun 5, 2017

Member
error[E0271]: type mismatch resolving `<users::table as diesel::query_source::AppearsInFromClause<posts::table>>::Count == diesel::query_source::Succ<diesel::query_source::Never>`
  --> src/main.rs:30:18
   |
30 |     let _ = join.select(posts::title);
   |                  ^^^^^^ expected struct `diesel::query_source::Never`, found struct `diesel::query_source::Succ`
   |
   = note: expected type `diesel::query_source::Never`
              found type `diesel::query_source::Succ<diesel::query_source::Never>`

This is really hard to understand what's going on here. Is there any way this could be better ?

@Eijebong

Eijebong Jun 5, 2017

Member
error[E0271]: type mismatch resolving `<users::table as diesel::query_source::AppearsInFromClause<posts::table>>::Count == diesel::query_source::Succ<diesel::query_source::Never>`
  --> src/main.rs:30:18
   |
30 |     let _ = join.select(posts::title);
   |                  ^^^^^^ expected struct `diesel::query_source::Never`, found struct `diesel::query_source::Succ`
   |
   = note: expected type `diesel::query_source::Never`
              found type `diesel::query_source::Succ<diesel::query_source::Never>`

This is really hard to understand what's going on here. Is there any way this could be better ?

This comment has been minimized.

@killercup

killercup Jun 5, 2017

Member

Yep, it's one of those errors we should cover in the docs. Or, you know customize.

(Also, shouldn't Succ<Never> be Once? Sadly not doable without specialization I guess)

@killercup

killercup Jun 5, 2017

Member

Yep, it's one of those errors we should cover in the docs. Or, you know customize.

(Also, shouldn't Succ<Never> be Once? Sadly not doable without specialization I guess)

This comment has been minimized.

@sgrif

sgrif Jun 5, 2017

Member

Once all of this is merged and stable functionality-wise, I think I will replace Succ with Once and MoreThanOnce just for the minor improvement in the error messages.

@sgrif

sgrif Jun 5, 2017

Member

Once all of this is merged and stable functionality-wise, I think I will replace Succ with Once and MoreThanOnce just for the minor improvement in the error messages.

// Valid
let _ = join.select(posts::title.nullable());
// Valid -- NULL to a function will return null
let _ = join.select(lower(posts::title).nullable());
// Invalid, only Nullable<title> is selectable
let _ = join.select(lower(posts::title)); //~ ERROR E0277
let _ = join.select(lower(posts::title));

This comment has been minimized.

@Eijebong

Eijebong Jun 5, 2017

Member

Same error here

@Eijebong

Eijebong Jun 5, 2017

Member

Same error here

@killercup

This looks good! It's pretty clever, but the right amount of cleverness for the job.

I'm pretty sure I understood what is happening in which cases (and I'm already looking forward to reading #930), but I've left some questions (and suggestions) inline. I also added some comments with general observations, I hope that's okay and doesn't distract too much from the other feedback.

pub struct Never;
#[allow(missing_debug_implementations, missing_copy_implementations)]
pub struct Succ<T>(T);
pub type Once = Succ<Never>;

This comment has been minimized.

@killercup

killercup Jun 5, 2017

Member

ℹ️ Peano numbering for Plus.

Maybe add this as comment headline in the source?

Maybe move trait Plus definition here as well? Put it all in one file/mod?

@killercup

killercup Jun 5, 2017

Member

ℹ️ Peano numbering for Plus.

Maybe add this as comment headline in the source?

Maybe move trait Plus definition here as well? Put it all in one file/mod?

type Count = Join::Count;
}
pub trait Plus<T> {

This comment has been minimized.

@killercup

killercup Jun 5, 2017

Member

ℹ️ Count occurrences with peano numbering. Once is the initial value used in impl ContainsTable<table>.

Maybe add ↑ as doc comment?

@killercup

killercup Jun 5, 2017

Member

ℹ️ Count occurrences with peano numbering. Once is the initial value used in impl ContainsTable<table>.

Maybe add ↑ as doc comment?

@@ -48,3 +48,13 @@ pub trait Table: QuerySource + AsQuery + Sized {
fn primary_key(&self) -> Self::PrimaryKey;
fn all_columns() -> Self::AllColumns;
}
pub trait AppearsInFromClause<QS> {

This comment has been minimized.

@killercup

killercup Jun 5, 2017

Member

ℹ️ Tracks how many times a table occurs in QuerySource (from/join clause)

Maybe add ↑ as doc comment?

@killercup

killercup Jun 5, 2017

Member

ℹ️ Tracks how many times a table occurs in QuerySource (from/join clause)

Maybe add ↑ as doc comment?

impl<T, Left, Right, Kind> AppearsInFromClause<T> for Join<Left, Right, Kind> where
Left: AppearsInFromClause<T>,
Right: AppearsInFromClause<T>,
Left::Count: Plus<Right::Count>,

This comment has been minimized.

@killercup

killercup Jun 5, 2017

Member

ℹ️ Constraint D: Table T occurs on left side of join one more time than on right side

Actually, why is that?

@killercup

killercup Jun 5, 2017

Member

ℹ️ Constraint D: Table T occurs on left side of join one more time than on right side

Actually, why is that?

This comment has been minimized.

@sgrif

sgrif Jun 5, 2017

Member

I'm not sure I understand the question here? This is basically just saying that both counts are either Never or Succ.

@sgrif

sgrif Jun 5, 2017

Member

I'm not sure I understand the question here? This is basically just saying that both counts are either Never or Succ.

This comment has been minimized.

@killercup

killercup Jun 5, 2017

Member

Ah, that makes sense. (I added the question today, but wrote the comment on Saturday)

@killercup

killercup Jun 5, 2017

Member

Ah, that makes sense. (I added the question today, but wrote the comment on Saturday)

}
impl<T, Join, On> AppearsInFromClause<T> for JoinOn<Join, On> where
Join: AppearsInFromClause<T>,

This comment has been minimized.

@killercup

killercup Jun 5, 2017

Member

ℹ️ Expose inner count in JoinOn wrapper

@killercup

killercup Jun 5, 2017

Member

ℹ️ Expose inner count in JoinOn wrapper

> for $column_name where
Left: $crate::JoinTo<$($table)::*>
impl<QS> AppearsOnTable<QS> for $column_name where
QS: AppearsInFromClause<$($table)::*, Count=Once>,

This comment has been minimized.

@killercup

killercup Jun 5, 2017

Member

ℹ️ Constraint A: Can only select column if column's table occurs exactly once in from/join

@killercup

killercup Jun 5, 2017

Member

ℹ️ Constraint A: Can only select column if column's table occurs exactly once in from/join

$($table)::*: $crate::JoinTo<Right>
$column_name: AppearsOnTable<Join<Left, Right, LeftOuter>>,
Left: AppearsInFromClause<$($table)::*, Count=Once>,
Right: AppearsInFromClause<$($table)::*, Count=Never>,

This comment has been minimized.

@killercup

killercup Jun 5, 2017

Member

ℹ️ Constraint B: Can only select column when column's table appears on left side of Left Outer Join and not on right side

@killercup

killercup Jun 5, 2017

Member

ℹ️ Constraint B: Can only select column when column's table appears on left side of Left Outer Join and not on right side

impl<Join, On> AppearsOnTable<JoinOn<Join, On>> for $column_name where
$column_name: AppearsOnTable<Join>,
$column_name: AppearsOnTable<Join<Left, Right, Inner>>,
Join<Left, Right, Inner>: AppearsInFromClause<$($table)::*, Count=Once>,

This comment has been minimized.

@killercup

killercup Jun 5, 2017

Member

ℹ️ Constraint C: Can only select column when column's table appears once on either left or right side of Left Inner Join

@killercup

killercup Jun 5, 2017

Member

ℹ️ Constraint C: Can only select column when column's table appears once on either left or right side of Left Inner Join

@@ -418,6 +403,10 @@ macro_rules! table_body {
}
}
impl AppearsInFromClause<table> for table {

This comment has been minimized.

@killercup

killercup Jun 5, 2017

Member

Why use the same name for the type param as for the struct?

@killercup

killercup Jun 5, 2017

Member

Why use the same name for the type param as for the struct?

This comment has been minimized.

@sgrif

sgrif Jun 5, 2017

Member

Because they're the same type. This is where we say a table contains itself exactly once. If we can get the changes we need to specialization, I'd have these two blanket impls instead:

impl<T, U> AppearsInFromClause<T> for U {
    type Count = Never;
}

// The table constraint is to make this impl disjoint from the impl we provide for `Join`
impl<T: Table> AppearsInFromClause<T> for T {
    type Count = Once;
}
@sgrif

sgrif Jun 5, 2017

Member

Because they're the same type. This is where we say a table contains itself exactly once. If we can get the changes we need to specialization, I'd have these two blanket impls instead:

impl<T, U> AppearsInFromClause<T> for U {
    type Count = Never;
}

// The table constraint is to make this impl disjoint from the impl we provide for `Join`
impl<T: Table> AppearsInFromClause<T> for T {
    type Count = Once;
}
@@ -534,6 +524,9 @@ macro_rules! joinable_inner {
primary_key_ty = $primary_key_ty:ty,
primary_key_expr = $primary_key_expr:expr,
) => {
impl $crate::query_source::AppearsInFromClause<$right_table_ty> for $left_table_ty {
type Count = $crate::query_source::Never;

This comment has been minimized.

@killercup

killercup Jun 5, 2017

Member

ℹ️ Right side of left inner join does not contain table on left side

@killercup

killercup Jun 5, 2017

Member

ℹ️ Right side of left inner join does not contain table on left side

@sgrif sgrif merged commit d1537db into master Jun 5, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sgrif sgrif deleted the sg-multi-table-joins branch Jun 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment