FrankChen021 commented on code in PR #19909:
URL: https://github.com/apache/druid/pull/19909#discussion_r3737029733
##########
sql/src/test/java/org/apache/druid/sql/calcite/parser/DruidSqlParserUtilsTest.java:
##########
@@ -60,46 +61,36 @@ public class DruidSqlParserUtilsTest
/**
* Sanity checking that the formats of TIME_FLOOR(__time, Period) work as
expected
*/
- @RunWith(Parameterized.class)
- public static class TimeFloorToGranularityConversionTest
+ @Nested
+ public class TimeFloorToGranularityConversionTest
{
- @Parameterized.Parameters(name = "{1}")
public static Iterable<Object[]> constructorFeeder()
Review Comment:
Evaluated — no code change is needed. Druid targets Java 25
(`maven.compiler.release`), where non-constant static members in inner classes
are legal. Jupiter supports static `@MethodSource` factories, and this nested
class uses that supported form. The SQL test compilation and full SQL test
suite passed locally, so converting this factory to an instance method and
changing lifecycle would be an unnecessary lifecycle change.
##########
sql/src/test/java/org/apache/druid/sql/calcite/parser/DruidSqlParserUtilsTest.java:
##########
@@ -453,7 +452,8 @@ public void
testConvertSqlNodeToGranularityWithIncorrectIngestionGranularityInTi
}
}
- public static class NonParameterizedTests
+ @Nested
+ public class NonParameterizedTests
{
private static final DateTimeZone TZ_LOS_ANGELES =
DateTimes.inferTzFromString("America/Los_Angeles");
Review Comment:
Evaluated — no code change is needed. With the project's Java 25 target,
non-constant static fields in an inner class are legal. This nested class also
uses `PER_CLASS` for its `@BeforeAll` lifecycle. The SQL test compilation and
full SQL test suite passed locally; making these fields instance fields would
not address an actual compilation issue.
##########
sql/src/test/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRuleTest.java:
##########
@@ -52,10 +51,9 @@ public class DruidLogicalValuesRuleTest
{
private static final PlannerContext DEFAULT_CONTEXT =
Mockito.mock(PlannerContext.class);
- @RunWith(Parameterized.class)
- public static class GetValueFromLiteralSimpleTypesTest extends
InitializedNullHandlingTest
+ @Nested
+ public class GetValueFromLiteralSimpleTypesTest extends
InitializedNullHandlingTest
Review Comment:
Evaluated — no code change is needed. Java 25 permits non-constant static
helpers in non-static inner classes, and the existing static `@MethodSource`
factory is valid for Jupiter. The SQL test compilation and full SQL test suite
passed locally. Removing `@Nested` or changing the lifecycle would therefore be
an unnecessary behavior change.
--
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]
