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

Add support for self (left / right / inner / cross) join #292

Merged
merged 3 commits into from
Mar 6, 2017
Merged

Add support for self (left / right / inner / cross) join #292

merged 3 commits into from
Mar 6, 2017

Conversation

Spike2050
Copy link
Contributor

Changes as discussed in issue #290.

Has (passing) tests and added description in readme.md.

I also adjusted some comments from the other joins.

Copy link
Member

@lukaseder lukaseder left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your suggestion. I've commented on a couple of issues. Let me know what you think.

README.md Outdated
@@ -62,6 +65,9 @@ Seq.of(1, 2, 3, 4).grouped(i -> i % 2);
// (tuple(1, 1), tuple(2, 2))
Seq.of(1, 2, 4).innerJoin(Seq.of(1, 2, 3), (a, b) -> a == b);

// (tuple(1, 1), tuple(2, 2))
Seq.of(1, 2).innerSelfJoin((t, u) -> t == u)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this would be a rather inefficient alternative to:

Seq.of(1, 2).map(i -> tuple(i, i))

Perhaps, a more convincing example might be interesting here, similar to your outer join examples, where child / parent relationships are joined. What do you think?

Copy link
Contributor Author

@Spike2050 Spike2050 Mar 5, 2017

Choose a reason for hiding this comment

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

Ill just change that to

// (tuple(1, 2), tuple(2, 1))
Seq.of(1, 2).innerSelfJoin((t, u) -> t != u)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, nice thinking. That's a common use-case in SQL too

README.md Outdated
@@ -77,6 +83,9 @@ Seq.of(1, 2, 3).join("|", "^", "$");
// (tuple(1, 1), tuple(2, 2), tuple(4, null))
Seq.of(1, 2, 4).leftOuterJoin(Seq.of(1, 2, 3), (a, b) -> a == b);

// (tuple(tuple(1, 0), NULL), tuple(tuple(2, 1), tuple(1, 0)))
Seq.of(new Tuple2<Integer, Integer>(1, 0), new Tuple2<Integer, Integer>(2, 1)).leftOuterSelfJoin((t, u) -> t.v2 == u.v1)
Copy link
Member

Choose a reason for hiding this comment

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

For brevity's sake, we can replace the constructor call with the Tuple.tuple() convenience method and assume a static import, so:

Seq.of(tuple(1, 0), tuple(2, 1))...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

np

* Inner join 2 streams into one.
* <p>
* <code><pre>
* // (tuple(1, 1), tuple(2, 2))
* Seq.of(1, 2, 3).innerJoin(Seq.of(1, 2), t -> Objects.equals(t.v1, t.v2))
* Seq.of(1, 2, 3).innerJoin(Seq.of(1, 2), (t, u) -> Objects.equals(t, u))
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for these. Would you mind fixing these in a separate pull request, as they're not strictly related to this particular change...

return seq(list).flatMap(t -> seq(list)
.filter(u -> predicate.test(t, u))
.map(u -> tuple(t, u)));
}
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I wonder if we perhaps would be better off if this method delegates to the existing innerJoin() method, and we start optimising these joins in a different task...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I could just do this

default Seq<Tuple2<T, T>> innerSelfJoin(BiPredicate<? super T, ? super T> predicate) {

    // This algorithm isn't lazy and has substantial complexity for large argument streams!
    List<? extends T> list = toList();

    return seq(list).innerJoin(seq(list), predicate);
}

Copy link
Member

Choose a reason for hiding this comment

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

Even better

.onEmpty(null)
.map(u -> tuple(t, u)))
.onClose(other::close);
}
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for fixing the formatting. Perhaps, this could be done in a separate task as well. In particular, now, the onClose() method is not aligned correctly according to your fix's intention.

.onEmpty(null)
.map(u -> tuple(t, u)))
.onClose(other::close);
}
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for fixing the formatting. Perhaps, this could be done in a separate task as well. In particular, now, the onClose() method is not aligned correctly according to your fix's intention.

@Spike2050
Copy link
Contributor Author

Spike2050 commented Mar 5, 2017

I made another pull request with just the comments and idents

@Spike2050
Copy link
Contributor Author

I incorporated the discussed changes, you can check it again.

@lukaseder
Copy link
Member

That looks great, thank you very much again for your contribution!

@lukaseder lukaseder merged commit 50469cf into jOOQ:master Mar 6, 2017
@Spike2050
Copy link
Contributor Author

Pleasure

When do you plan your next release?

@lukaseder
Copy link
Member

I guess we have enough new things. Will look into it these days

@Spike2050
Copy link
Contributor Author

Ever thought about doing an official Snapshot/Nightly/Alpha?

I mean I did install a private one on my nexus, to test the selfjoins with my other projects, but an official one would be nice.

@lukaseder
Copy link
Member

If there's something OOTB that does this on Github with absolutely no manual work involved, sure. Do you know any?

@Spike2050
Copy link
Contributor Author

Spike2050 commented Mar 7, 2017

Well I work with Jenkins, which does this for me, but I think Travis can do that too.

I found this link which descripes a Travis Setup to deploy to Maven, including SNAPSHOT-version. But as I see it you might need to hide your Travis configuration from public Github repo, because it would store your Maven repo login. It might be what you need.

@billoneil
Copy link
Contributor

billoneil commented Mar 7, 2017 via email

@lukaseder
Copy link
Member

Hmm, yes, I could use the jOOQ CI server (Jenkins) to deploy nightlies to Maven Central... But that's already too much effort, given the relatively little progress this library makes between releases :)

And Jitpack sounds nice, but another infrastructure dependency to look after.

I'll wait with this, until more users request it (also for the other jOO* libraries).

@Spike2050
Copy link
Contributor Author

Hey lukaseder.

What ever came of the realease you were looking into?

Greets

@lukaseder
Copy link
Member

I'm sorry - not sure if I understand, what are you asking me, @Spike2050 ?

@bendem
Copy link

bendem commented Nov 15, 2017

When do you plan your next release?

I guess we have enough new things. Will look into it these days

I think he's speaking about the fact that the last release you did was more than a year ago and that there are a bunch of new stuff to release now (I'd totally be interested in a new release as well) :).

@Spike2050
Copy link
Contributor Author

I think he's speaking about the fact that the last release you did was more than a year ago and that there are a bunch of new stuff to release now (I'd totally be interested in a new release as well) :).

Well that :-)

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

Successfully merging this pull request may close these issues.

4 participants