Fix nested joins by mkleen · Pull Request #13742 · crate/crate

mkleen · 2023-02-28T16:28:40Z

This makes nested joins work such as:

SELECT *
    FROM j1
    JOIN (j2 JOIN j3 ON j2.x = j3.x)
    ON j1.x = j2.x;

by adding a JoinRelation which is used to determine the correct JoinPairs.

fixes #13503

Summary of the changes / Why this improves CrateDB

Checklist

Added an entry in CHANGES.txt for user facing changes
Updated documentation & sql_features table for user facing changes
Touched code is covered by tests
CLA is signed
This does not contain breaking changes, or if it does:
- It is released within a major release
- It is recorded in CHANGES.txt
- It was marked as deprecated in an earlier release if possible
- You've thought about the consequences and other components are adapted
  (E.g. AdminUI)

server/src/main/java/io/crate/analyze/relations/RelationAnalysisContext.java

BaurzhanSakhariev

Nice work, thanks!

Left a question and minor comments.

server/src/main/java/io/crate/analyze/relations/RelationAnalyzer.java

server/src/test/java/io/crate/planner/operators/JoinTest.java

server/src/testFixtures/java/io/crate/testing/JoinPairAssert.java

server/src/test/java/io/crate/planner/operators/JoinTest.java

server/src/testFixtures/java/io/crate/testing/JoinPairAssert.java

mkleen · 2023-03-03T06:58:34Z

Thanks for the feedback.
This needs one more iteration, also the changelog entry is missing.

matriv · 2023-03-03T08:07:59Z

@mkleen Thank you for the work on this, please ping and I will also review once you complete the new iteration.

mkleen · 2023-03-07T08:12:45Z

Ready for another review.

server/src/main/java/io/crate/analyze/relations/RelationAnalyzer.java

BaurzhanSakhariev

👍
Thanks for covering extra cases!

LGTM, added a minor question on tests

server/src/test/java/io/crate/integrationtests/JoinIntegrationTest.java

docs/appendices/release-notes/unreleased.rst

server/src/main/java/io/crate/analyze/JoinRelation.java

server/src/test/java/io/crate/integrationtests/JoinIntegrationTest.java

server/src/main/java/io/crate/analyze/relations/RelationAnalyzer.java

matriv

Thank you, also left some comments, and also:
Can we have some more complex testing with more levels of nesting and to also have same tables reused, e.g.:

SELECT ....
FROM t1 JOIN (t2 JOIN (t3 JOIN t1 ON ...) t3 ON ...)

server/src/main/java/io/crate/analyze/relations/RelationAnalysisContext.java

server/src/main/java/io/crate/analyze/relations/RelationAnalyzer.java

server/src/test/java/io/crate/planner/operators/JoinTest.java

mkleen · 2023-03-07T09:49:55Z

Thanks for the feedback. I am wondering if it maybe makes sense to split this into two prs. One to resolve the nested join issue, the other one for using.

matriv · 2023-03-07T09:57:41Z

Thanks for the feedback. I am wondering if it maybe makes sense to split this into two prs. One to resolve the nested join issue, the other one for using.

If it's not too much trouble, why not, we can have 2 separate changelog entries as well.

mkleen · 2023-03-07T15:19:48Z

So the initial issue was the following query, which works now.

SELECT 
  * 
FROM 
  j1 
  JOIN (
    j2 
    JOIN j3 ON j2.x = j3.x
  ) ON j1.x = j2.x

However, we can currently not go infinitely nested. The following query does not work and I would consider this out-of-scope for a fix. This would require significant changes in the way joins are treated internally. This is anyway not a common use case.

SELECT 
  y, 
  z 
FROM 
  a 
  JOIN (
    b 
    JOIN (
      c 
      JOIN a ON c.z = a.x
    ) temp1 ON b.y = temp1.x
  ) temp2 on a.x = temp2.x;

My suggestion is to throw a reasonable error message and leave it as is. @matriv @mfussenegger @BaurzhanSakhariev

matriv · 2023-03-07T15:27:04Z

It's fine to address the most complex nesting later as a feature.
For the simpler nesting can we do something like this?

FROM 
  j1 
  JOIN (
    j2 
    JOIN j3 ON j2.x = j1.x AND j3.x = j2.x
  ) ON j1.x = j2.x

mkleen · 2023-03-14T09:49:04Z

Ready for another review. I split this pr now. The join using stuff goes into a follow-up.

BaurzhanSakhariev

👍
Thanks! Added a minor comment

server/src/test/java/io/crate/integrationtests/JoinIntegrationTest.java

matriv

Thank you! Very nice to have a nice error message for the unsupported cases, instead of an internal error.

Left some very minor comments, no need to address
Does it worth mentioning the improved error message in the changelog entry?

server/src/main/java/io/crate/analyze/JoinRelation.java

server/src/test/java/io/crate/planner/operators/JoinTest.java

This makes nested joins work such as: SELECT * FROM j1 JOIN (j2 JOIN j3 ON j2.x = j3.x) ON j1.x = j2.x; by adding a JoinRelation which is used to determine the correct JoinPairs. Fixes #13503

mkleen force-pushed the mkleen/join-ordering-v4 branch 8 times, most recently from d157d04 to 7849d46 Compare March 2, 2023 13:29

mkleen commented Mar 2, 2023

View reviewed changes

server/src/main/java/io/crate/analyze/relations/RelationAnalysisContext.java Show resolved Hide resolved

mkleen force-pushed the mkleen/join-ordering-v4 branch 2 times, most recently from f773b9c to bb8d749 Compare March 2, 2023 14:03

mkleen marked this pull request as ready for review March 2, 2023 14:10

mkleen requested review from BaurzhanSakhariev and matriv March 2, 2023 14:10

mkleen force-pushed the mkleen/join-ordering-v4 branch from bb8d749 to 8030659 Compare March 2, 2023 14:39

BaurzhanSakhariev reviewed Mar 2, 2023

View reviewed changes

server/src/test/java/io/crate/planner/operators/JoinTest.java Show resolved Hide resolved

BaurzhanSakhariev reviewed Mar 2, 2023

View reviewed changes

server/src/testFixtures/java/io/crate/testing/JoinPairAssert.java Outdated Show resolved Hide resolved

BaurzhanSakhariev reviewed Mar 2, 2023

View reviewed changes

server/src/test/java/io/crate/planner/operators/JoinTest.java Show resolved Hide resolved

BaurzhanSakhariev reviewed Mar 2, 2023

View reviewed changes

server/src/testFixtures/java/io/crate/testing/JoinPairAssert.java Outdated Show resolved Hide resolved

mkleen added the v/5.2 label Mar 2, 2023

mkleen force-pushed the mkleen/join-ordering-v4 branch 2 times, most recently from 6a15641 to 179b8f5 Compare March 6, 2023 16:09

mkleen changed the title ~~Make nested joins work~~ Fix nested joins Mar 6, 2023

mkleen force-pushed the mkleen/join-ordering-v4 branch from abbffad to 410d109 Compare March 7, 2023 08:09

mkleen requested review from BaurzhanSakhariev and mfussenegger March 7, 2023 08:12

mkleen force-pushed the mkleen/join-ordering-v4 branch from 410d109 to ecf4b2f Compare March 7, 2023 08:32

mkleen commented Mar 7, 2023

View reviewed changes

server/src/main/java/io/crate/analyze/relations/RelationAnalyzer.java Outdated Show resolved Hide resolved

BaurzhanSakhariev approved these changes Mar 7, 2023

View reviewed changes

server/src/test/java/io/crate/integrationtests/JoinIntegrationTest.java Outdated Show resolved Hide resolved

mfussenegger reviewed Mar 7, 2023

View reviewed changes

matriv reviewed Mar 7, 2023

View reviewed changes

mkleen force-pushed the mkleen/join-ordering-v4 branch 6 times, most recently from ba2e368 to 54ed6ad Compare March 14, 2023 09:46

mkleen requested review from BaurzhanSakhariev, matriv and mfussenegger March 14, 2023 09:49

BaurzhanSakhariev approved these changes Mar 14, 2023

View reviewed changes

server/src/test/java/io/crate/integrationtests/JoinIntegrationTest.java Outdated Show resolved Hide resolved

mfussenegger approved these changes Mar 14, 2023

View reviewed changes

matriv approved these changes Mar 14, 2023

View reviewed changes

server/src/main/java/io/crate/analyze/JoinRelation.java Outdated Show resolved Hide resolved

server/src/test/java/io/crate/planner/operators/JoinTest.java Outdated Show resolved Hide resolved

server/src/test/java/io/crate/planner/operators/JoinTest.java Outdated Show resolved Hide resolved

Fix nested joins

c32ddf8

This makes nested joins work such as: SELECT * FROM j1 JOIN (j2 JOIN j3 ON j2.x = j3.x) ON j1.x = j2.x; by adding a JoinRelation which is used to determine the correct JoinPairs. Fixes #13503

mkleen force-pushed the mkleen/join-ordering-v4 branch from fbbde00 to c32ddf8 Compare March 14, 2023 11:28

Merge branch 'master' into mkleen/join-ordering-v4

200828a

mkleen added the ready-to-merge Let Mergify merge the PR once approved and checks pass label Mar 14, 2023

mergify bot merged commit e9624a9 into master Mar 14, 2023

mergify bot deleted the mkleen/join-ordering-v4 branch March 14, 2023 12:15

mergify bot mentioned this pull request Mar 14, 2023

Fix nested joins (backport #13742) #13789

Merged

BaurzhanSakhariev mentioned this pull request Apr 28, 2023

Fix issue with implicit join pairs #14029

Closed

Conversation

mkleen commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the changes / Why this improves CrateDB

Checklist

Uh oh!

Uh oh!

BaurzhanSakhariev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mkleen commented Mar 3, 2023

Uh oh!

matriv commented Mar 3, 2023

Uh oh!

mkleen commented Mar 7, 2023

Uh oh!

Uh oh!

BaurzhanSakhariev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matriv left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mkleen commented Mar 7, 2023

Uh oh!

matriv commented Mar 7, 2023

Uh oh!

mkleen commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matriv commented Mar 7, 2023

Uh oh!

mkleen commented Mar 14, 2023

Uh oh!

BaurzhanSakhariev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mkleen commented Feb 28, 2023 •

edited

Loading

matriv left a comment •

edited

Loading

mkleen commented Mar 7, 2023 •

edited

Loading