test: don't allow FK-outer joins and add QTT cases for FK-join with flipped join condition #7526

mjsax · 2021-05-14T07:02:14Z

Description

Add a check to disallow OUTER join for FK-joins.

Add new tests for FK-joins with "flipped" join condition. Luckily JoinTree.build() take care of the order by calling join.flip() if necessary, and thus we don't need to write additional code for FK-joins for this case. However, we should have the corresponding tests.

Testing done

Only added tests for binary joins. Not sure if we should add tests for n-way joins at this point, as don't plan to support n-way joins yet.

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

vcrfxia

Thanks @mjsax . Comments inline.

vcrfxia · 2021-05-14T15:06:51Z

ksqldb-functional-tests/src/test/resources/query-validation-tests/fk-join.json

@@ -12,6 +12,18 @@
        "message": "Invalid join condition: table-table joins require to join on the primary key of the right input table. Got LEFT_TABLE.ID1 = RIGHT_TABLE.F2."
      }
    },
+    {
+      "name": "Should fail on right non-key attribute for inner-join -- revers join condition order",


I don't think we need copies of these tests with the feature flag disabled. If the purpose of the test is to check that table-table joins are only allowed on primary keys, then tests for that already exist. If the purpose is to check that the behavior does not change if the join condition is flipped, then that belongs in the regular joins.json file since it's not related to foreign key joins.

I think adding duplicates of all the new FK join tests will unnecessarily blow up this file and create more busywork for us (harder reviews, more work when we ungate the feature flag) than it's worth.

What is your concrete proposal? I think we should have some of those tests, to make sure that the condition is correctly identified even if the join condition is flipped. And yes, we should have corresponding tests in joins.json in addition.

My concrete proposal is:

delete all the new tests in this PR that do not have the feature flag enabled

remove the comments from the copies of the tests with the feature flag enabled about deleting the test when ungating from the feature flag

remove - feature flag enabled from the names of the copies of the tests that have the feature flag enabled

If we're planning to support flipped join conditions out of the box (which it sounds like we are), then we should add positive test coverage to ensure this is properly supported. I will add that into #7528.

I thought about this a little bit. I agree that those this test (and it's "siblings") should not be part of fk-join test. However, joins.json is very long and hard to keep an overview what we test and what not. Will add table-table-join.json and move these 10 test over. Want to keep the reverse order as I think we should test that the condition is detected for both cases (and also that we "flip" left/right condition in the error message).

I would hope we can cleanup join.json eventually, but it's a little tricky to "move" test as we also need to migrate old plans accordingly. Plan tp can dig into this as a follow up.

vcrfxia · 2021-05-14T15:08:24Z

ksqldb-functional-tests/src/test/resources/query-validation-tests/fk-join.json

+      "properties": {
+        "ksql.joins.foreign.key.enable": true
+      },
+      "comments": [


If you go with my suggestion above to remove the copies of these tests without the feature flag enabled, then we can also remove these comments. When we ungate the feature flag, we'll simply remove the properties block rather than deleting the test. You can also update the test name to remove the - feature flag enabled portion now so we don't have to update the test name later when ungating the feature flag.

vcrfxia · 2021-05-14T15:10:12Z

ksqldb-functional-tests/src/test/resources/query-validation-tests/fk-join.json

+      ],
+      "expectedException": {
+        "type": "io.confluent.ksql.util.KsqlStatementException",
+        "message": "Invalid join condition: table-table joins require to join on the primary key of the right input table. Got LEFT_TABLE.ID1 = RIGHT_TABLE.F2."


I think it'd be marginally better if the error message showed the join expressions in the same order as in the user's query but given the structure of the code right now this is somewhat difficult to achieve so I think this is fine.

I had a similar though... But I think we could fix it: JoinTree could set a flag "flipped" that we preserve that allows us to flip back in the error message. Let me add this.

vcrfxia · 2021-05-14T15:11:36Z

ksqldb-functional-tests/src/test/resources/query-validation-tests/fk-join.json

@@ -193,6 +327,25 @@
        "message": "Invalid join condition: table-table joins require to join on the primary key of the right input table. Got LEFT_TABLE.ID1 = RIGHT_TABLE.F2."
      }
    },
+    {
+      "name": "Should fail on right non-key attribute for outer-join (reverse join condition order) - feature flag enabled",


Why do we need this test? FK joins aren't supported for outer joins at all.

Ah. Good catch. Seems I did to much "blind" c&p :)

mjsax · 2021-05-18T04:20:09Z

ksqldb-functional-tests/src/test/resources/query-validation-tests/fk-join.json

@@ -1,99 +1,105 @@
 {
  "tests": [
    {
-      "name": "Should fail on right non-key attribute for inner-join",


This diff is confusion... This test (and the next 3 tests) got moved to table-table-join.json

mjsax · 2021-05-18T04:20:21Z

ksqldb-functional-tests/src/test/resources/query-validation-tests/fk-join.json

      }
    },
    {
-      "name": "Should fail on right non-key attribute for inner-join with qualifiers",
+      "name": "Should fail on outer-join",


mjsax · 2021-05-18T04:20:50Z

ksqldb-functional-tests/src/test/resources/query-validation-tests/fk-join.json

@@ -1,99 +1,105 @@
 {
  "tests": [
    {
-      "name": "Should fail on right non-key attribute for inner-join",
+      "name": "Should fail on left non-key attribute for inner-join",


existing test, just bad diff

mjsax · 2021-05-18T04:21:08Z

ksqldb-functional-tests/src/test/resources/query-validation-tests/fk-join.json

      }
    },
    {
-      "name": "Should fail on right non-key attribute for left-join",
+      "name": "Should fail on left non-key attribute for inner-join -- revers join condition order",


new test: duplicates the above existing test.

mjsax · 2021-05-18T04:21:31Z

ksqldb-functional-tests/src/test/resources/query-validation-tests/fk-join.json

      }
    },
    {
-      "name": "Should fail on left non-key attribute for inner-join",
+      "name": "Should fail on left non-key attribute for left-join -- revers join condition order",


new test: duplicates the above existing test.

mjsax · 2021-05-18T04:21:37Z

ksqldb-functional-tests/src/test/resources/query-validation-tests/fk-join.json

      ],
      "expectedException": {
        "type": "io.confluent.ksql.util.KsqlStatementException",
        "message": "Invalid join condition: foreign-key table-table joins are not supported. Got LEFT_TABLE.F1 = RIGHT_TABLE.ID2."
      }
    },
    {
-      "name": "Should fail on left non-key attribute for outer-join",
+      "name": "Should fail on left non-key attribute for outer-join -- revers join condition order",


new test: duplicates the above existing test.

mjsax · 2021-05-18T04:22:39Z

ksqldb-functional-tests/src/test/resources/query-validation-tests/fk-join.json

      }
    },
    {
-      "name": "Should fail on right non-key attribute for inner-join with qualifiers",


This test (and the next tests) got moved to table-table-join.json

mjsax · 2021-05-18T04:23:06Z

ksqldb-functional-tests/src/test/resources/query-validation-tests/fk-join.json

      }
    },
    {
-      "name": "Should fail on right non-key attribute for inner-join with alias",
+      "name": "Should fail on outer-join -- reverse join condition",


new test (duplicate of the previous new test)

mjsax · 2021-05-18T04:23:44Z

ksqldb-functional-tests/src/test/resources/query-validation-tests/table-table-join.json

+{
+  "tests": [
+    {
+      "name": "Should fail on right non-key attribute for inner-join",


moved from fk-join.json

mjsax · 2021-05-18T04:24:09Z

ksqldb-functional-tests/src/test/resources/query-validation-tests/table-table-join.json

+      }
+    },
+    {
+      "name": "Should fail on right non-key attribute for inner-join -- revers join condition order",


duplicate the test (similar pattern below)

vcrfxia

I forgot this PR was still open... we're going to have a lot of merge conflicts with #7528 and #7537

Didn't look at the latest changes too carefully but LGTM based on the comments that the latest revision claims to have addressed.

test: add QTT cases for FK-join with flipped join condition

d262f36

mjsax requested a review from a team as a code owner May 14, 2021 07:02

mjsax mentioned this pull request May 14, 2021

Support non-key joins. #4424

Closed

vcrfxia reviewed May 14, 2021

View reviewed changes

mjsax changed the title ~~test: add QTT cases for FK-join with flipped join condition~~ test: don't allow FK-outer joins and add QTT cases for FK-join with flipped join condition May 18, 2021

Github comments; added outer-join check

0a3ced2

mjsax commented May 18, 2021

View reviewed changes

This was referenced May 18, 2021

test: adds tests for foreign key joins #7528

Closed

test: add test coverage for FK joins #7537

Merged

nit

59faf75

vcrfxia approved these changes May 18, 2021

View reviewed changes

vcrfxia merged commit b073bbc into confluentinc:master May 18, 2021

mjsax deleted the fk-join-relax-order branch June 10, 2021 05:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: don't allow FK-outer joins and add QTT cases for FK-join with flipped join condition #7526

test: don't allow FK-outer joins and add QTT cases for FK-join with flipped join condition #7526

mjsax commented May 14, 2021 •

edited

Loading

vcrfxia left a comment

vcrfxia May 14, 2021

mjsax May 14, 2021

vcrfxia May 14, 2021

mjsax May 18, 2021

vcrfxia May 14, 2021

vcrfxia May 14, 2021

mjsax May 14, 2021

vcrfxia May 14, 2021

mjsax May 14, 2021

mjsax May 18, 2021 •

edited

Loading

mjsax May 18, 2021

mjsax May 18, 2021

mjsax May 18, 2021

mjsax May 18, 2021

mjsax May 18, 2021

mjsax May 18, 2021

mjsax May 18, 2021

mjsax May 18, 2021

mjsax May 18, 2021

vcrfxia left a comment

test: don't allow FK-outer joins and add QTT cases for FK-join with flipped join condition #7526

test: don't allow FK-outer joins and add QTT cases for FK-join with flipped join condition #7526

Conversation

mjsax commented May 14, 2021 • edited Loading

Description

Testing done

Reviewer checklist

vcrfxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjsax May 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcrfxia left a comment

Choose a reason for hiding this comment

mjsax commented May 14, 2021 •

edited

Loading

mjsax May 18, 2021 •

edited

Loading