Split `SelectableExpression` into two traits #764

sgrif · 2017-02-26T12:55:39Z

The change in #709 had the side effect of re-introducing #104.
With the design that we have right now, nullability isn't propagating
upwards. This puts the issue of "expressions aren't validating that the
type of its arguments haven't become nullable, and thus nulls are
slipping in where they shouldn't be" at odds with "we can't use complex
expressions in filters for joins because the SQL type changed".

This semi-resolves the issue by restricting when we care about
nullability. Ultimately the only time it really matters is when we're
selecting data, as we need to enforce that the result goes into an
Option. For places where we don't see the bytes in Rust (filter,
order, etc), NULL is effectively false.

This change goes back to fully fixing #104, but brings back a small
piece of #621. I've changed everything that is a composite expression to
only be selectable if the SQL type hasn't changed. This means that you
won't be able to do things like
users.left_outer_join(posts).select(posts::id + 1), but you will be
able to use whatever you want in filter.

This change is also to support what I think will fix the root of all
these issues. The design of "Here's the SQL type on this query source"
is just fundamentally not what we need. There is only one case where the
type changes, and that is to become null when it is on the right side of
a left join, the left side of a right join, or either side of a full
join.

One of the changes that #709 made was to require that you explicitly
call .nullable() on a tuple if you wanted to get Option<(i32, String)> instead of (Option<i32>, Option<String>). This has worked
out fine, and isn't a major ergonomic pain. The common case is just to
use the default select clause anyway. So I want to go further down this
path.

The longer term plan is to remove SqlTypeForSelect entirely, and not
implement SelectableExpression for columns on the nullable side of a
join. We will then provide these two blanket impls:

impl<Left, Right, T> SelectableExpression<LeftOuterJoin<Left, Right>>
    for Nullable<T> where T: SelectableExpression<Right>,
{}

impl<Left, Right, Head, Tail> SelectableExpression<LeftOuterJoin<Left, Right>>
    for Nullable<Cons<Head, Tail>> where
        Nullable<Head>: SelectableExpression<LeftOuterJoin<Left, Right>>,
        Nullable<Tail>: SelectableExpression<LeftOuterJoin<Left, Right>>,
{}

(Note: Those impls overlap. Providing them as blanket impls would
require rust-lang/rust#40097. Providing them as
non-blanket impls would require us to mark Nullable and possibly
Cons as #[fundamental])

The end result will be that nullability naturally propagates as we want
it to. Given sql_function!(lower, lower_t, (x: Text) -> Text), doing
select(lower(posts::name).nullable()) will work. lower(posts::name)
will fail because posts::name doesn't impl SelectableExpression.
lower(posts::name.nullable()) will fail because while
SelectableExpression will be met, the SQL type of the argument isn't
what's expected. Putting .nullable at the very top level naturally
follows SQL's semantics here.

killercup

(AFAICT Most of the diff is straightforward impls of the new marker trait.)

LGTM. I think it's a good idea to differentiate between these use cases, and I actually really like the name of AppearsOnTable! :) Looking forward to the PR that fixes the join.

killercup · 2017-03-03T18:54:28Z

diesel/src/expression/mod.rs

@@ -102,6 +102,27 @@ impl<T: Expression> AsExpression<T::SqlType> for T {
    }
 }

+/// Indicates that all elements of an expression are valid given a from clause.
+/// This is used ot ensure that `users.filter(posts::id.eq(1))` fails to


killercup · 2017-03-03T19:08:04Z

diesel/src/expression/mod.rs

@@ -110,20 +131,20 @@ impl<T: Expression> AsExpression<T::SqlType> for T {
 /// Columns will implement this for their table. Certain special types, like
 /// `CountStar` and `Bound` will implement this for all sources. All other
 /// expressions will inherit this from their children.
-pub trait SelectableExpression<QS>: Expression {
+pub trait SelectableExpression<QS>: AppearsOnTable<QS> {


Maybe add a line about caring about nullability to the docs (esp. as this is a subtrait of AppearsOnTable now)? Actually, to be really explicit you can still keep Expression as supertrait (it's referred to in the docs).

The docs currently say: "The associated type is usually the same as Expression::SqlType, but is used to indicate that a column is always nullable when it appears on the right side of a left outer join, even if it wasn't nullable to begin with.". Is there more to say about nullability? (I'll be rewriting the docs when I remove the associated type)

Nah, it's fine. Mentioning the relation to AppearsOnTable might've been nice, but it's a supertrait, so people should look at it anyway (and it has good docs).

The change in #709 had the side effect of re-introducing #104. With the design that we have right now, nullability isn't propagating upwards. This puts the issue of "expressions aren't validating that the type of its arguments haven't become nullable, and thus nulls are slipping in where they shouldn't be" at odds with "we can't use complex expressions in filters for joins because the SQL type changed". This semi-resolves the issue by restricting when we care about nullability. Ultimately the only time it really matters is when we're selecting data, as we need to enforce that the result goes into an `Option`. For places where we don't see the bytes in Rust (filter, order, etc), `NULL` is effectively `false`. This change goes back to fully fixing #104, but brings back a small piece of #621. I've changed everything that is a composite expression to only be selectable if the SQL type hasn't changed. This means that you won't be able to do things like `users.left_outer_join(posts).select(posts::id + 1)`, but you will be able to use whatever you want in `filter`. This change is also to support what I think will fix the root of all these issues. The design of "Here's the SQL type on this query source" is just fundamentally not what we need. There is only one case where the type changes, and that is to become null when it is on the right side of a left join, the left side of a right join, or either side of a full join. One of the changes that #709 made was to require that you explicitly call `.nullable()` on a tuple if you wanted to get `Option<(i32, String)>` instead of `(Option<i32>, Option<String>)`. This has worked out fine, and isn't a major ergonomic pain. The common case is just to use the default select clause anyway. So I want to go further down this path. The longer term plan is to remove `SqlTypeForSelect` entirely, and *not* implement `SelectableExpression` for columns on the nullable side of a join. We will then provide these two blanket impls: ```rust impl<Left, Right, T> SelectableExpression<LeftOuterJoin<Left, Right>> for Nullable<T> where T: SelectableExpression<Right>, {} impl<Left, Right, Head, Tail> SelectableExpression<LeftOuterJoin<Left, Right>> for Nullable<Cons<Head, Tail>> where Nullable<Head>: SelectableExpression<LeftOuterJoin<Left, Right>>, Nullable<Tail>: SelectableExpression<LeftOuterJoin<Left, Right>>, {} ``` (Note: Those impls overlap. Providing them as blanket impls would require rust-lang/rust#40097. Providing them as non-blanket impls would require us to mark `Nullable` and possibly `Cons` as `#[fundamental]`) The end result will be that nullability naturally propagates as we want it to. Given `sql_function!(lower, lower_t, (x: Text) -> Text)`, doing `select(lower(posts::name).nullable())` will work. `lower(posts::name)` will fail because `posts::name` doesn't impl `SelectableExpression`. `lower(posts::name.nullable())` will fail because while `SelectableExpression` will be met, the SQL type of the argument isn't what's expected. Putting `.nullable` at the very top level naturally follows SQL's semantics here.

sgrif requested a review from killercup February 26, 2017 12:55

sgrif force-pushed the sg-split-selectable-expression branch from ee05eb1 to c9fc988 Compare February 26, 2017 17:19

sgrif mentioned this pull request Feb 26, 2017

Merge Expression with SelectableExpression #6

Closed

killercup approved these changes Mar 3, 2017

View reviewed changes

sgrif force-pushed the sg-split-selectable-expression branch from 515483f to 6dec748 Compare March 3, 2017 19:21

sgrif merged commit 6dec748 into master Mar 3, 2017

sgrif deleted the sg-split-selectable-expression branch March 3, 2017 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split `SelectableExpression` into two traits #764

Split `SelectableExpression` into two traits #764

sgrif commented Feb 26, 2017

killercup left a comment

killercup Mar 3, 2017

killercup Mar 3, 2017

sgrif Mar 3, 2017

killercup Mar 3, 2017

Split SelectableExpression into two traits #764

Split SelectableExpression into two traits #764

Conversation

sgrif commented Feb 26, 2017

killercup left a comment

Choose a reason for hiding this comment

killercup Mar 3, 2017

Choose a reason for hiding this comment

killercup Mar 3, 2017

Choose a reason for hiding this comment

sgrif Mar 3, 2017

Choose a reason for hiding this comment

killercup Mar 3, 2017

Choose a reason for hiding this comment

Split `SelectableExpression` into two traits #764

Split `SelectableExpression` into two traits #764