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

Merge `Expression` with `SelectableExpression` #6

Closed
sgrif opened this Issue Nov 25, 2015 · 2 comments

Comments

Projects
None yet
1 participant
@sgrif
Member

sgrif commented Nov 25, 2015

I'd like to make this refactoring to clean up parts of our type system. However, I can't seem to figure out how to express tuples with that refactoring, as right now a tuple of (A, B, C) can be expressed in two ways if all 3 of the inner types are nullable, it can be Nullable<(SA, SB, SC)> or (Nullable<SA>, Nullable<SB>, Nullable<SC>).

This is important for the ergonomics of left outer joins. I'd be fine with dropping the second case for joins, but we definitely cannot drop it more generally.

@sgrif sgrif changed the title from Move the second type argument of `SelectableExpression` to an associated type to Merge `Expression` with `AsExpression` Jan 30, 2016

@sgrif sgrif changed the title from Merge `Expression` with `AsExpression` to Merge `Expression` with `SelectableExpression` Jan 30, 2016

@sgrif

This comment has been minimized.

Member

sgrif commented Jan 31, 2016

I've updated the title of this to where I think this should go based on recent developments. Ultimately Expression and SelectableExpression are redundant, and Expression::SqlType is basically meaningless.

What I'm currently imagining is the resulting trait being

pub trait Expression<Source> {
    type SqlType;
}

One thing that's stood out to me lately is that the only case where SelectableExpression and Expression differ is when the source is the right side of the left outer joins, which makes the type nullable.

It is entirely possible that this indicates a fundamental flaw either in how we handle nulls, or how we vary the sql type for joins. I am open to any ideas/opinions about that, or this change in general.

@sgrif

This comment has been minimized.

Member

sgrif commented Feb 26, 2017

A combination of #709, #764 and the nullable change described in #764 fully resolve this.

@sgrif sgrif closed this Feb 26, 2017

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