Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
FULL OUTER JOIN support #4526
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
base: master
Are you sure you want to change the base?
FULL OUTER JOIN support #4526
Changes from 5 commits
abd9f11
2bf2269
4612bfc
9eda01e
0ed6cc4
879906d
9ca966e
dbf77cc
b0695b4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually flawed, and I can't seem to find a good solution. Because
.nullable()
always wraps the selection inNullable
, even if alreadyNullable
, when using multiplefull_join()
calls you end up with(Nullable<(Nullable<A>, Nullable<B>)>, Nullable<C>)
, rather than(Nullable<A>, Nullable<B>, Nullable<C>)
.One solution could be to make a
IntoNullable
trait on expressions (akin to thesql_type
version), which would essentially returnSelf
if alreadyNullable<...>
. However, I've been getting conflicting impl errors between theT
andNullable<T>
blocks and can't seem to find a way to get this to work. Any ideas or other approaches that could be taken? Seemingly the fact it's checkingIsNull
with the indirection fromExpression::SqlType
is preventing Rust from realising they're disjoint as is done in the sql_type version. (still fails if adding+ SingleValue
)Then replace
Nullable<Left::DefaultSelection>
with<Left::DefaultSelection as IntoNullable>::Nullable
.Alternatively, we could add a
AppendSelection
implfor
Nullable<(Nullable<Left>, Nullable<Mid>)>
which unpacks it, but you have the same type issues.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what could work is using the associated type specialization trick used in other places.
That means we would need to restructure that trait like follows:
in that way you have to two impl that are clearly not overlapping as they are for different types and a third impl on top of it that forwards to the other impls. You then need to call it slightly different as
<E::SqlType as IntoNullableExpression<E>>::into_nullable_expression(e)
from the calling side, but given that this is only one location + that's not public API I would consider that acceptable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh clever! Will try update to use that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This runs in to an issue unfortunately, as tuples seem to be
IsNullable
if any of the values areIsNullable
. e.g.(Integer, Text, Nullable<Text>)::IsNull == IsNullable
. Thus, it keeps(Integer, Text, Nullable<Text>)
as(Integer, Text, Nullable<Text>)
, rather than turning it in toNullable<(Integer, Text, Nullable<Text>)>
.Tuples are using
OneIsNullable
, should they perhaps instead be usingAllAreNullable
? Or if that's correct, is there another way to do this check?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No that's correct for tuples. Giving it another thought I noticed that this all is not required.
(Nullable<(Nullable<A>, Nullable<B>)>, Nullable<C>)
is as far as I remember the expected result here. The sql type code handles unwrapping that to something more meaningful if I remember that correctly. In fact that seems to be also the thing we do forLEFT JOINS
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure I follow sorry, I was under the impression that
AppendSelection
should instead give(Nullable<A>, Nullable<B>, Nullable<C>)
? In its doc comment it says:The following compiles correctly using left joins:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That means I need to have another look at this to give better suggestions. I'm not sure if I find the time this or next week. If that doesn't happen until then please ping me again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed c718389 which at least fixes compiling all the tests. I'm still not 100% sure if this impl is correct. It might require an additional step that wraps every element in the tuple returned from the
Join
query source (left hand side) in aNullable
wrapper if it's not already nullable. I think this would apply if you have first a inner join and then a full join. If that is still problematic it should be possible to implement such a tuple mapping by having a helper trait for all supported tuple sizes that just applies theIntoNullableExpression
trait to all tuple elements.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for that, I added a test that did confirm that inner join then full join is problematic, so I've added
IntoNullableExpression
for tuples.It is now essentially working, however, I've needed to add
IntoNullableExpression
forSkipSelectableExpressionBoundCheckWrapper
asimpl<ST, E> IntoNullableExpression<E> for ST
needs to be whereST: SingleValue
to avoid conflicting with the tuple impls andSkipSelectableExpressionBoundCheckWrapper
no longer has an impl. That's conflicting withimpl<ST, E> IntoNullableExpression<E> for ST
though. Looking forward to negative trait bounds stabilising...I don't have more time right now to try and fix, but will come back in a few more days when I get the time. If you have thoughts on it let me know though.