Skip to content
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

FULL OUTER JOIN support #4526

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

FULL OUTER JOIN support #4526

wants to merge 5 commits into from

Conversation

oeed
Copy link
Contributor

@oeed oeed commented Mar 7, 2025

This PR adds support for FULL OUTER JOIN queries, which are supported by both Postgres and SQLite (but not MySQL). I'm unaware if there are a specific reason it wasn't originally included, perhaps just as they're not super common, but I've run it to a situation where I do so figured it was worthwhile PRing.

This isn't quite ready, but it has the general structure. It primarily just needs tests and checking that it's done in a good way. Have left a few comments with questions about whether its taking the best approach.

@oeed oeed changed the title FULL OUTER JOIN FULL OUTER JOIN support Mar 7, 2025
Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this. This is great to see ❤️.

I think that's already a solid start implementation wise. I would like to tweak the backend restrictions a bit to make this extendable by third party backends + restrict it really only to postgres + sqlite. Otherwise the implementation is already ready in my opinion.

What is still missing are tests + documentation. Also the compile test coverage could be improved. For tests it would be great to extend the existing join tests here to have at least:

  • A test for a plain full join
  • A test for using several full joins in a row
  • Mixing full joins with left join and inner join

@oeed
Copy link
Contributor Author

oeed commented Mar 7, 2025

Great, note re docs and tests.

That's a good point about 3rd party backends. How would you recommend doing so, a trait implemented on the backend? Are there any existing examples of that?

Sorry was on mobile and didn't realised you'd replied to each note.

@weiznich
Copy link
Member

weiznich commented Mar 7, 2025

That's a good point about 3rd party backends. How would you recommend doing so, a trait implemented on the backend? Are there any existing examples of that?

See the instructions here: #4526 (comment)

@oeed oeed force-pushed the full-outer-join branch from 3c47828 to abd9f11 Compare March 9, 2025 21:11
self.left
.source
.default_selection()
.nullable()
Copy link
Contributor Author

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 in Nullable, even if already Nullable, when using multiple full_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 the sql_type version), which would essentially return Self if already Nullable<...>. However, I've been getting conflicting impl errors between the T and Nullable<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 checking IsNull with the indirection from Expression::SqlType is preventing Rust from realising they're disjoint as is done in the sql_type version. (still fails if adding + SingleValue)

/// Convert values into their nullable form, without double wrapping them in `Nullable<...>`.
pub trait IntoNullable<ST> {
    /// The nullable value of this type.
    ///
    /// For all values except `Nullable`, this will be `Nullable<Self>`.
    type Nullable;

    /// Convert this value into its nullable representation.
    ///
    /// For `Nullable<T>`, this remain as `Nullable<T>`, otherwise the value will be wrapped and be `Nullable<Self>`.
    fn into_nullable(self) -> Self::Nullable;
}

impl<T> IntoNullable for T
where
    T: Expression,
    T::SqlType: SqlType<IsNull = sql_types::is_nullable::NotNull>,
{
    type Nullable = Nullable<T>;

    fn into_nullable(self) -> Self::Nullable {
        Nullable(self)
    }
}

impl<T> IntoNullable for Nullable<T>
where
    Nullable<T>: Expression,
    <Nullable<T> as Expression>::SqlType: SqlType, // also fails with SqlType<IsNull = sql_types::is_nullable::IsNullable>
{
    type Nullable = Self;

    fn into_nullable(self) -> Self::Nullable {
        self
    }
}

Then replace Nullable<Left::DefaultSelection> with <Left::DefaultSelection as IntoNullable>::Nullable.

Alternatively, we could add a AppendSelection impl
for Nullable<(Nullable<Left>, Nullable<Mid>)> which unpacks it, but you have the same type issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants