KSQL 420 join criteria validation #447

hjafarpour · 2017-11-09T00:48:41Z

No description provided.

dguy · 2017-11-09T10:44:49Z

ksql-engine/src/main/java/io/confluent/ksql/analyzer/Analyzer.java

@@ -236,8 +238,13 @@ protected Node visitJoin(final Join node, final AnalysisContext context) {
    JoinOn joinOn = (JoinOn) (node.getCriteria().get());


This line here will throw an exception if the criteria is not present

Added a check for this.

dguy · 2017-11-09T10:46:04Z

ksql-engine/src/main/java/io/confluent/ksql/analyzer/Analyzer.java

+    }
+    if (keyInfo == null) {
+      throw new KsqlException(String.format("%s : Invalid join criteria %s ", comparisonExpression
+          .getLocation().get().toString(), comparisonExpression));


calling get on an Opitional without checking if it is present. Might result in a NoSuchElementException

Added the check.

Did you forget to push the commit? I don't see the check.

dguy · 2017-11-09T10:55:18Z

ksql-engine/src/main/java/io/confluent/ksql/analyzer/Analyzer.java

          (DereferenceExpression) expression;
-      return leftDereferenceExpression.getFieldName();
+      String sourceAliasVal = dereferenceExpression.getBase().toString();


I find this confusing. Can we at least add a method to DereferenceExpression called getSourceAlias()?
Ideally we can come up with an interface that both DereferenceExpression and QualifiedNameReference implement that enable us to get the keyFieldName. We can then get rid of the instanceof checks here and delegate to the impl

This would be a bit difficult since these are two different nodes in our AST and SourceAlias is not always the lefthand side for these expressions. The interpretation of these expression also depends on the context.

dguy · 2017-11-09T10:56:57Z

ksql-engine/src/test/java/io/confluent/ksql/analyzer/QueryAnalyzerTest.java

+        metaStore);
+    final Query query = (Query) statements.get(0);
+    try {
+      final Analysis analysis = queryAnalyzer.analyize(query);


analysis is unused.

Removed the variable.

dguy · 2017-11-09T10:58:12Z

ksql-engine/src/test/java/io/confluent/ksql/analyzer/QueryAnalyzerTest.java

+      final Analysis analysis = queryAnalyzer.analyize(query);
+    } catch (KsqlException ex) {
+      assertThat(ex.getMessage().trim(), equalTo("Line: 1, Col: 46 : Invalid join criteria "
+                                             + "(TEST1.COL1 = TEST2.COLL)"));


Is there anyway we can add something to say which column is invalid?

Added more details.

dguy · 2017-11-09T10:58:38Z

ksql-engine/src/test/java/io/confluent/ksql/analyzer/QueryAnalyzerTest.java

+        "select * from test1 join test2 on test2.col2 = test1.col1;",
+        metaStore);
+    final Query query = (Query) statements.get(0);
+    final Analysis analysis = queryAnalyzer.analyize(query);


Might want to check that the result has the correct structure

Added join checks.

bluemonk3y · 2017-11-09T01:08:51Z

ksql-engine/src/main/java/io/confluent/ksql/analyzer/Analyzer.java

+                                                      rightDataSource.getSchema());
+
+    String leftKeyFieldName = leftSide.getRight();
+    String rightKeyFieldName = rightSide.getRight();

    if (comparisonExpression.getType() != ComparisonExpression.Type.EQUAL) {
      throw new KsqlException("Join criteria is not supported.");


I know its not part of the PR - but can the message be improved - i.e. Join criteria only supports [EQUALS]

Good point, updated it!

bluemonk3y · 2017-11-09T01:09:58Z

ksql-engine/src/test/java/io/confluent/ksql/analyzer/QueryAnalyzerTest.java

+    try {
+      final Analysis analysis = queryAnalyzer.analyize(query);
+    } catch (KsqlException ex) {
+      assertThat(ex.getMessage().trim(), equalTo("Line: 1, Col: 46 : Invalid join criteria "


I cant see where the source message comes from - but more information would be useful - i.e. coll field does not exist.

Added more info to the error message.

apurvam · 2017-11-10T01:10:42Z

ksql-engine/src/main/java/io/confluent/ksql/analyzer/Analyzer.java

@@ -251,20 +258,43 @@ protected Node visitJoin(final Join node, final AnalysisContext context) {
    return null;
  }

-  private String fetchKeyFieldName(Expression expression) {
+  private Pair<String, String> fetchKeyFieldName(ComparisonExpression comparisonExpression, String sourceAlias, Schema


Not sure I follow the logic here. We are trying to match the alias with both the left and right side of the expression. Isn't this too inclusive? In the calling context we are fetching both the left and right side separately. If the alias matches either, won't this validation pass erroneously?

This is to make sure that we don't impose an order on the comparison expression sides. For instance, left side can have the table and right side can have the stream key. We expect to see the key for both stream and key regardless of their place in the comparison expression.

I see. Perhaps it would be worth adding a comment about this. Something like 'This method picks the side of the expression which matches the given alias'.

An alternative would be to send the left and right side separately with both aliases, and allow either alias to be picked. Both are equivalent, but the suggested comment on the behavior would help, IMO.

Added comments providing more details on the method.

apurvam · 2017-11-10T01:11:46Z

ksql-engine/src/test/java/io/confluent/ksql/integration/JoinIntTest.java

@@ -76,7 +76,7 @@ public void shouldLeftJoinOrderAndItems() throws Exception {

    final String queryString = String.format(
            "CREATE STREAM %s AS SELECT ORDERID, ITEMID, ORDERUNITS, DESCRIPTION FROM orders LEFT JOIN items " +
-                    " on orders.ITEMID = item.ITEMID WHERE orders.ITEMID = 'ITEM_1' ;",
+                    " on orders.ITEMID = items.ID WHERE orders.ITEMID = 'ITEM_1' ;",


This was an existing bug which now fails due to the newly raised KSQLException?

Yes, I fixed the test.

…eria-Validation

Better error messages for join criteria check.

…eria-Validation

dguy

LGTM

Added checks for join criteria.

d4067cb

hjafarpour requested review from bluemonk3y, dguy and apurvam November 9, 2017 00:48

dguy reviewed Nov 9, 2017

View reviewed changes

bluemonk3y approved these changes Nov 9, 2017

View reviewed changes

apurvam reviewed Nov 10, 2017

View reviewed changes

hjafarpour added 4 commits November 10, 2017 15:40

Merge remote-tracking branch 'upstream/4.0.x' into KSQL-420-Join-Crit…

418596c

…eria-Validation

Applied review feedback.

128f7f8

Better error messages for join criteria check.

Merge remote-tracking branch 'upstream/4.0.x' into KSQL-420-Join-Crit…

4f02fee

…eria-Validation

Added more comments for new methods.

639583e

dguy approved these changes Nov 14, 2017

View reviewed changes

hjafarpour merged commit 19c6d41 into confluentinc:4.0.x Nov 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KSQL 420 join criteria validation #447

KSQL 420 join criteria validation #447

hjafarpour commented Nov 9, 2017

dguy Nov 9, 2017

hjafarpour Nov 11, 2017

dguy Nov 9, 2017

hjafarpour Nov 11, 2017

apurvam Nov 11, 2017

dguy Nov 9, 2017

hjafarpour Nov 14, 2017

dguy Nov 9, 2017

hjafarpour Nov 11, 2017

dguy Nov 9, 2017

hjafarpour Nov 11, 2017

dguy Nov 9, 2017

hjafarpour Nov 11, 2017

bluemonk3y Nov 9, 2017

hjafarpour Nov 11, 2017

bluemonk3y Nov 9, 2017

hjafarpour Nov 11, 2017 •

edited

Loading

apurvam Nov 10, 2017

hjafarpour Nov 11, 2017

apurvam Nov 11, 2017

hjafarpour Nov 14, 2017

apurvam Nov 10, 2017

hjafarpour Nov 11, 2017

dguy left a comment

		@@ -236,8 +238,13 @@ protected Node visitJoin(final Join node, final AnalysisContext context) {
		JoinOn joinOn = (JoinOn) (node.getCriteria().get());

KSQL 420 join criteria validation #447

KSQL 420 join criteria validation #447

Conversation

hjafarpour commented Nov 9, 2017

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

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

hjafarpour Nov 11, 2017 • 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

dguy left a comment

Choose a reason for hiding this comment

hjafarpour Nov 11, 2017 •

edited

Loading