Re: [PR] Calcite tests remove expected exception (druid)

2024-03-11 Thread via GitHub 


abhishekrb19 merged PR #16046:
URL: https://github.com/apache/druid/pull/16046


-- 
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-unsubscr...@druid.apache.org

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


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

Re: [PR] Calcite tests remove expected exception (druid)

2024-03-11 Thread via GitHub 


abhishekrb19 commented on PR #16046:
URL: https://github.com/apache/druid/pull/16046#issuecomment-1987811257

   The rule ordering cleanup changes can come in a later patch


-- 
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-unsubscr...@druid.apache.org

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


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

Re: [PR] Calcite tests remove expected exception (druid)

2024-03-11 Thread via GitHub 


kgyrtkirk commented on code in PR #16046:
URL: https://github.com/apache/druid/pull/16046#discussion_r1519268656


##
sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java:
##
@@ -295,15 +298,11 @@ public static Map 
getTimeseriesContextWithFloorTime(
   public final SqlEngine engine0;
   final boolean useDefault = NullHandling.replaceWithDefault();
 
-  @Rule(order = 1)
-  public ExpectedException expectedException = ExpectedException.none();
-
   @Rule(order = 2)

Review Comment:
   I agree that these could be changed; however one of the followup changes is 
to upgrade calcite* tests to junit5 - can we postpone this cleanup to that 
patch?



-- 
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-unsubscr...@druid.apache.org

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


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

Re: [PR] Calcite tests remove expected exception (druid)

2024-03-10 Thread via GitHub 


abhishekrb19 commented on code in PR #16046:
URL: https://github.com/apache/druid/pull/16046#discussion_r1519171613


##
sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java:
##
@@ -295,15 +298,11 @@ public static Map 
getTimeseriesContextWithFloorTime(
   public final SqlEngine engine0;
   final boolean useDefault = NullHandling.replaceWithDefault();
 
-  @Rule(order = 1)
-  public ExpectedException expectedException = ExpectedException.none();
-
   @Rule(order = 2)

Review Comment:
   We can also remove the junit rule order or re-order the rules (same applies 
to `@Rule(order = 3) queryFrameworkRule`:
   ```suggestion
 @Rule
   ```



-- 
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-unsubscr...@druid.apache.org

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


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

Re: [PR] Calcite tests remove expected exception (druid)

2024-03-07 Thread via GitHub 


abhishekrb19 commented on code in PR #16046:
URL: https://github.com/apache/druid/pull/16046#discussion_r1517095801


##
sql/src/test/java/org/apache/druid/sql/calcite/CalciteSubqueryTest.java:
##
@@ -675,98 +678,112 @@ public void testEmptyGroupWithOffsetDoesntInfiniteLoop()
   public void testMaxSubqueryRows()
   {
 if ("without memory limit".equals(testName)) {
-  expectedException.expect(ResourceLimitExceededException.class);
-  expectedException.expectMessage(
-  "Cannot issue the query, subqueries generated results beyond 
maximum[1] rows. Try setting the "
-  + "'maxSubqueryBytes' in the query context to 'auto' for enabling 
byte based limit, which chooses an optimal "
-  + "limit based on memory size and result's heap usage or manually 
configure the values of either 'maxSubqueryBytes' "
-  + "or 'maxSubqueryRows' in the query context. Manually alter the 
value carefully as it can cause the broker to "
-  + "go out of memory."
-  );
+  testMaxSubqueryRowsWithoutMemoryLimit();
+} else {
+  testMaxSubQueryRowsWithLimit();
+}
+  }
+
+  private void testMaxSubqueryRowsWithoutMemoryLimit()
+  {
+Throwable exception = assertThrows(ResourceLimitExceededException.class, 
() -> {
   Map modifiedQueryContext = new HashMap<>(queryContext);

Review Comment:
   It would be better to move all the test setup outside the `assertThrows` and 
narrow it down to the thing that we're expecting to throw an exception.



##
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##
@@ -12319,61 +12313,64 @@ public void 
testRequireTimeConditionLogicalValuePositive()
   @Test
   public void testRequireTimeConditionSimpleQueryNegative()
   {
-msqIncompatible();
-expectedException.expect(CannotBuildQueryException.class);
-expectedException.expectMessage("__time column");
+Throwable exception = assertThrows(CannotBuildQueryException.class, () -> {
+  msqIncompatible();

Review Comment:
   nit: Consider moving `msqIncompatible()` and any other test initialization 
outside `assertThrows`.
   Same applies to the other tests



##
sql/src/test/java/org/apache/druid/sql/calcite/CalciteMultiValueStringQueryTest.java:
##
@@ -123,23 +124,25 @@ public void testMultiValueStringWorksLikeStringGroupBy()
   public void testMultiValueStringGroupByDoesNotWork()
   {
 // Cannot vectorize due to usage of expressions.

Review Comment:
   should the comment be removed as well?



-- 
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-unsubscr...@druid.apache.org

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


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

Re: [PR] Calcite tests remove expected exception (druid)

2024-03-07 Thread via GitHub 


kgyrtkirk commented on PR #16046:
URL: https://github.com/apache/druid/pull/16046#issuecomment-1984417355

   thank you @abhishekrb19 for taking a look; I plan to later also run
   
https://docs.openrewrite.org/recipes/java/testing/junit5/usehamcrestassertthat 
   to get rid of old methods - but I have not seen any differences so far 
between the `assertThat` methods


-- 
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-unsubscr...@druid.apache.org

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


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

Re: [PR] Calcite tests remove expected exception (druid)

2024-03-07 Thread via GitHub 


kgyrtkirk commented on code in PR #16046:
URL: https://github.com/apache/druid/pull/16046#discussion_r1516786952


##
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##
@@ -7581,47 +7573,48 @@ public void testAvgDailyCountDistinct()
   @Test
   public void testHighestMaxNumericInFilter()
   {
-expectedException.expect(UOE.class);
-expectedException.expectMessage("Expected parameter[maxNumericInFilters] 
cannot exceed system set value of [100]");
+Throwable exception = assertThrows(UOE.class, () -> {
 
-testQuery(
-PLANNER_CONFIG_MAX_NUMERIC_IN_FILTER,
-ImmutableMap.of(QueryContexts.MAX_NUMERIC_IN_FILTERS, 2),
-"SELECT COUNT(*)\n"
-+ "FROM druid.numfoo\n"
-+ "WHERE dim6 IN (\n"
-+ "1,2,3\n"
-+ ")\n",
-CalciteTests.REGULAR_USER_AUTH_RESULT,
-ImmutableList.of(),
-ImmutableList.of()
-);
+  testQuery(
+  PLANNER_CONFIG_MAX_NUMERIC_IN_FILTER,
+  ImmutableMap.of(QueryContexts.MAX_NUMERIC_IN_FILTERS, 2),
+  "SELECT COUNT(*)\n"
+  + "FROM druid.numfoo\n"
+  + "WHERE dim6 IN (\n"
+  + "1,2,3\n"
+  + ")\n",
+  CalciteTests.REGULAR_USER_AUTH_RESULT,
+  ImmutableList.of(),
+  ImmutableList.of()
+  );
+});
+assertTrue(exception.getMessage().contains("Expected 
parameter[maxNumericInFilters] cannot exceed system set value of [100]"));
   }
 
   @Test
   public void testQueryWithMoreThanMaxNumericInFilter()
   {
-if (NullHandling.sqlCompatible()) {
-  // skip in sql compatible mode, this plans to an OR filter with equality 
filter children...
-  return;
-}
+assumeFalse(
+"skip in sql compatible mode, this plans to an OR filter with equality 
filter children",
+NullHandling.sqlCompatible()
+);
 msqIncompatible();
-expectedException.expect(UOE.class);
-expectedException.expectMessage(
-"The number of values in the IN clause for [dim6] in query exceeds 
configured maxNumericFilter limit of [2] for INs. Cast [3] values of IN clause 
to String");
 
-testQuery(
-PLANNER_CONFIG_MAX_NUMERIC_IN_FILTER,
-ImmutableMap.of(QueryContexts.MAX_NUMERIC_IN_FILTERS, 2),
-"SELECT COUNT(*)\n"
-+ "FROM druid.numfoo\n"
-+ "WHERE dim6 IN (\n"
-+ "1,2,3\n"
-+ ")\n",
-CalciteTests.REGULAR_USER_AUTH_RESULT,
-ImmutableList.of(),
-ImmutableList.of()
-);
+Throwable exception = assertThrows(UOE.class, () -> {
+  testQuery(
+  PLANNER_CONFIG_MAX_NUMERIC_IN_FILTER,
+  ImmutableMap.of(QueryContexts.MAX_NUMERIC_IN_FILTERS, 2),
+  "SELECT COUNT(*)\n"
+  + "FROM druid.numfoo\n"
+  + "WHERE dim6 IN (\n"
+  + "1,2,3\n"
+  + ")\n",
+  CalciteTests.REGULAR_USER_AUTH_RESULT,
+  ImmutableList.of(),
+  ImmutableList.of()
+  );
+});
+assertTrue(exception.getMessage().contains("The number of values in the IN 
clause for [dim6] in query exceeds configured maxNumericFilter limit of [2] for 
INs. Cast [3] values of IN clause to String"));

Review Comment:
   I think its better to use static imports than write out the class the actual 
assert/assume/etc comes from;
   if the usecase would require to use 2 of those static imported methods from 
2 different sources at the same time I would see differently - but if there is 
only one then I think it as something which reduces the readability with no 
added benefit



-- 
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-unsubscr...@druid.apache.org

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


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

Re: [PR] Calcite tests remove expected exception (druid)

2024-03-07 Thread via GitHub 


kgyrtkirk commented on code in PR #16046:
URL: https://github.com/apache/druid/pull/16046#discussion_r1516773585


##
sql/src/test/java/org/apache/druid/sql/calcite/CalciteParameterQueryTest.java:
##
@@ -589,32 +593,40 @@ public void testLongs()
   @Test
   public void testMissingParameter()
   {
-expectedException.expect(
-DruidExceptionMatcher.invalidSqlInput().expectMessageIs("No value 
bound for parameter (position [1])")
+DruidException exception = assertThrows(
+DruidException.class,
+() -> testQuery(
+"SELECT COUNT(*)\n"
++ "FROM druid.numfoo\n"
++ "WHERE l1 > ?",
+ImmutableList.of(),
+ImmutableList.of(new Object[] {3L}),
+ImmutableList.of()
+)
 );
-testQuery(
-"SELECT COUNT(*)\n"
-+ "FROM druid.numfoo\n"
-+ "WHERE l1 > ?",
-ImmutableList.of(),
-ImmutableList.of(new Object[]{3L}),
-ImmutableList.of()
+assertThat(
+exception,
+DruidExceptionMatcher.invalidSqlInput().expectMessageIs("No value 
bound for parameter (position [1])")
 );
   }
 
   @Test
   public void testPartiallyMissingParameter()
   {
-expectedException.expect(
-DruidExceptionMatcher.invalidSqlInput().expectMessageIs("No value 
bound for parameter (position [2])")
+DruidException exception = assertThrows(
+DruidException.class,
+() -> testQuery(
+"SELECT COUNT(*)\n"
++ "FROM druid.numfoo\n"
++ "WHERE l1 > ? AND f1 = ?",
+ImmutableList.of(),
+ImmutableList.of(new Object[] {3L}),
+ImmutableList.of(new SqlParameter(SqlType.BIGINT, 3L))
+)
 );
-testQuery(
-"SELECT COUNT(*)\n"
-+ "FROM druid.numfoo\n"
-+ "WHERE l1 > ? AND f1 = ?",
-ImmutableList.of(),
-ImmutableList.of(new Object[]{3L}),
-ImmutableList.of(new SqlParameter(SqlType.BIGINT, 3L))
+assertThat(

Review Comment:
   the ide have imported the wrong class...fixed it!



-- 
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-unsubscr...@druid.apache.org

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


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

Re: [PR] Calcite tests remove expected exception (druid)

2024-03-07 Thread via GitHub 


abhishekrb19 commented on code in PR #16046:
URL: https://github.com/apache/druid/pull/16046#discussion_r1516535624


##
sql/src/test/java/org/apache/druid/sql/calcite/CalciteParameterQueryTest.java:
##
@@ -589,32 +593,40 @@ public void testLongs()
   @Test
   public void testMissingParameter()
   {
-expectedException.expect(
-DruidExceptionMatcher.invalidSqlInput().expectMessageIs("No value 
bound for parameter (position [1])")
+DruidException exception = assertThrows(
+DruidException.class,
+() -> testQuery(
+"SELECT COUNT(*)\n"
++ "FROM druid.numfoo\n"
++ "WHERE l1 > ?",
+ImmutableList.of(),
+ImmutableList.of(new Object[] {3L}),
+ImmutableList.of()
+)
 );
-testQuery(
-"SELECT COUNT(*)\n"
-+ "FROM druid.numfoo\n"
-+ "WHERE l1 > ?",
-ImmutableList.of(),
-ImmutableList.of(new Object[]{3L}),
-ImmutableList.of()
+assertThat(

Review Comment:
   The codeQL notice seems legit. Can we replace the deprecated method with 
`MatcherAssert.assertThat()`:
   ```suggestion
   MatcherAssert.assertThat(
   ```



##
sql/src/test/java/org/apache/druid/sql/calcite/CalciteParameterQueryTest.java:
##
@@ -589,32 +593,40 @@ public void testLongs()
   @Test
   public void testMissingParameter()
   {
-expectedException.expect(
-DruidExceptionMatcher.invalidSqlInput().expectMessageIs("No value 
bound for parameter (position [1])")
+DruidException exception = assertThrows(
+DruidException.class,
+() -> testQuery(
+"SELECT COUNT(*)\n"
++ "FROM druid.numfoo\n"
++ "WHERE l1 > ?",
+ImmutableList.of(),
+ImmutableList.of(new Object[] {3L}),
+ImmutableList.of()
+)
 );
-testQuery(
-"SELECT COUNT(*)\n"
-+ "FROM druid.numfoo\n"
-+ "WHERE l1 > ?",
-ImmutableList.of(),
-ImmutableList.of(new Object[]{3L}),
-ImmutableList.of()
+assertThat(
+exception,
+DruidExceptionMatcher.invalidSqlInput().expectMessageIs("No value 
bound for parameter (position [1])")
 );
   }
 
   @Test
   public void testPartiallyMissingParameter()
   {
-expectedException.expect(
-DruidExceptionMatcher.invalidSqlInput().expectMessageIs("No value 
bound for parameter (position [2])")
+DruidException exception = assertThrows(
+DruidException.class,
+() -> testQuery(
+"SELECT COUNT(*)\n"
++ "FROM druid.numfoo\n"
++ "WHERE l1 > ? AND f1 = ?",
+ImmutableList.of(),
+ImmutableList.of(new Object[] {3L}),
+ImmutableList.of(new SqlParameter(SqlType.BIGINT, 3L))
+)
 );
-testQuery(
-"SELECT COUNT(*)\n"
-+ "FROM druid.numfoo\n"
-+ "WHERE l1 > ? AND f1 = ?",
-ImmutableList.of(),
-ImmutableList.of(new Object[]{3L}),
-ImmutableList.of(new SqlParameter(SqlType.BIGINT, 3L))
+assertThat(

Review Comment:
   ```suggestion
   MatcherAssert.assertThat(
   ```



##
sql/src/test/java/org/apache/druid/sql/calcite/CalciteParameterQueryTest.java:
##
@@ -624,14 +636,19 @@ public void testPartiallyMissingParameterInTheMiddle()
 List params = new ArrayList<>();
 params.add(null);
 params.add(new SqlParameter(SqlType.INTEGER, 1));
-expectedException.expect(
-DruidExceptionMatcher.invalidSqlInput().expectMessageIs("No value 
bound for parameter (position [1])")
+DruidException exception = assertThrows(
+DruidException.class,
+() -> testQuery(
+"SELECT 1 + ?, dim1 FROM foo LIMIT ?",
+ImmutableList.of(),
+ImmutableList.of(),
+params
+)
 );
-testQuery(
-"SELECT 1 + ?, dim1 FROM foo LIMIT ?",
-ImmutableList.of(),
-ImmutableList.of(),
-params
+
+assertThat(

Review Comment:
   ```suggestion
   MatcherAssert.assertThat(
   ```



##
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##
@@ -6133,12 +6124,13 @@ public void 
testCountStarWithTimeInIntervalFilterNonLiteral()
 msqIncompatible();
 testQueryThrows(
 "SELECT COUNT(*) FROM druid.foo "
-+ "WHERE TIME_IN_INTERVAL(__time, dim1)",
-expected -> {
-  expected.expect(CoreMatchers.instanceOf(DruidException.class));
-  
expected.expect(ThrowableMessageMatcher.hasMessage(CoreMatchers.containsString(
-  "Argument to function 'TIME_IN_INTERVAL' must be a literal (line 
[1], column [63])")));
-}
++ "WHERE TIME_IN_INTERVAL(__time, dim1)",

Review Comment:
   Fixup some inconsistent space. The original formatting looks correct:
   ```suggestion
+ "WHERE TIME_IN_INTERVAL(__time, dim1)",

Re: [PR] Calcite tests remove expected exception (druid)

2024-03-06 Thread via GitHub 


kgyrtkirk commented on PR #16046:
URL: https://github.com/apache/druid/pull/16046#issuecomment-1981459426

   failure of jdk11 static checks seem unrelated


-- 
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-unsubscr...@druid.apache.org

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


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