LakshSingla commented on code in PR #14890:
URL: https://github.com/apache/druid/pull/14890#discussion_r1351508499


##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/CalciteSelectQueryMSQTest.java:
##########
@@ -174,4 +174,25 @@ public void testArrayAggQueryOnComplexDatatypes()
       );
     }
   }
+
+  @Ignore
+  @Override
+  public void testEquiJoin2()
+  {
+
+  }
+
+  @Ignore
+  @Override
+  public void testEquiJoin3()
+  {
+
+  }
+
+  @Ignore
+  @Override
+  public void testEquiJoin4()
+  {
+
+  }

Review Comment:
   1. Can you add a comment on why these joins are not working with MSQ? Per 
our knowledge, MSQ supports joins and hence the inability to work with one is 
concerning and should be documented.
   2. There's a `msqIncompatible` flag that you can use instead, which is for 
this purpose itself unless this error happens in the planning phase.



##########
sql/src/test/java/org/apache/druid/sql/calcite/DecoupledPlanningCalciteQueryTest.java:
##########
@@ -53,4 +54,60 @@ public SqlTestFramework.PlannerFixture 
plannerFixture(PlannerConfig plannerConfi
         .cannotVectorize(cannotVectorize)
         .skipVectorize(skipVectorize);
   }
+
+  @Override
+  @Ignore
+  public void testEquiJoin()
+  {
+
+  }
+
+  @Override
+  @Ignore
+  public void testEquiJoin2()
+  {
+
+  }

Review Comment:
   Try using @DecoupledIgnore annotation on the original test method. 



##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########
@@ -14635,4 +14635,462 @@ public void 
testLatestByOnStringColumnWithoutMaxBytesSpecified()
             new Object[] {"abc", defaultString, "def", defaultString, "def", 
defaultString}
         ));
   }
+
+  @Test
+  public void testEquiJoin()

Review Comment:
   Also, can you please use descriptive names for the test, where they 
represent what the test is primarily supposed to be verifying, or documents the 
regression why the test was added. 
   
   For example: `testEquiJoinWhereLeftHandIsConstant`



##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########


Review Comment:
   The tests should be moved to `CalciteJoinQueryTest`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to