Re: [PR] fix bug join query with require time condition (druid)

["LakshSingla (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org