FrankChen021 commented on code in PR #19881:
URL: https://github.com/apache/druid/pull/19881#discussion_r3717592929
##########
extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/theta/oldapi/OldApiSketchAggregationTest.java:
##########
@@ -92,15 +88,17 @@ public static Collection<?> constructorFeeder()
return constructors;
}
- @After
+ @AfterEach
public void teardown() throws IOException
{
helper.close();
}
Review Comment:
Fixed in commit
[6d8055b3f8](https://github.com/apache/druid/commit/6d8055b3f8). `teardown()`
now checks `helper != null`, closes it when initialized, and clears the field
afterward. This prevents cleanup from masking an initialization failure and
avoids retaining a closed helper.
The focused `OldApiSketchAggregationTest` run passed all 36 parameterized
tests. The combined focused command covered this class and the other two
reviewed classes and passed 86 tests; Maven Checkstyle and SpotBugs both
reported no violations or errors.
##########
extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/theta/SketchAggregationWithSimpleDataTest.java:
##########
@@ -98,20 +95,20 @@ public static Collection<?> constructorFeeder()
return constructors;
}
- @Before
+ @BeforeEach
public void setup() throws Exception
{
SketchModule.registerSerde();
sm = new SketchModule();
try (
- final AggregationTestHelper toolchest =
AggregationTestHelper.createGroupByQueryAggregationTestHelper(
+ final AggregationTestHelper toolchest =
AggregationTestHelper.createGroupByQueryAggregationTestHelperWithTempDir(
sm.getJacksonModules(),
config,
tempFolder
Review Comment:
Fixed in commit
[6d8055b3f8](https://github.com/apache/druid/commit/6d8055b3f8). I removed the
mutable `config`/`vectorize` lifecycle state and changed setup to
`setup(GroupByQueryConfig config)`, called at the start of each of the four
`@ParameterizedTest` methods. Each parameterized invocation now builds its
indexes with its own config after JUnit 5 supplies the parameters; `vectorize`
remains the test method parameter used for the query context.
Validation:
```text
mvn -ntp test -pl extensions-core/datasketches -am
-Dtest='org.apache.druid.query.aggregation.datasketches.theta.SketchAggregationWithSimpleDataTest,org.apache.druid.query.aggregation.datasketches.theta.oldapi.OldApiSketchAggregationTest,org.apache.druid.query.aggregation.datasketches.hll.HllSketchMergeAggregatorFactoryTest'
-Dsurefire.failIfNoSpecifiedTests=false -Pskip-static-checks
-Dweb.console.skip=true -T1C
```
Result: 86 tests passed, 0 failures/errors/skips. Checkstyle and SpotBugs
also passed.
##########
extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/hll/HllSketchMergeAggregatorFactoryTest.java:
##########
@@ -82,50 +83,56 @@ public void setUp()
vectorFactory = new
TestVectorColumnSelectorFactory().addCapabilities(FIELD_NAME,
columnCapabilities);
}
- @Test(expected = AggregatorFactoryNotMergeableException.class)
- public void testGetMergingFactoryBadName() throws Exception
+ @Test
+ public void testGetMergingFactoryBadName()
{
- HllSketchMergeAggregatorFactory other = new
HllSketchMergeAggregatorFactory(
- NAME + "-diff",
- FIELD_NAME,
- LG_K,
- TGT_HLL_TYPE,
- STRING_ENCODING,
- SHOULD_FINALIZE,
- ROUND
- );
- targetRound.getMergingFactory(other);
+ Assertions.assertThrows(AggregatorFactoryNotMergeableException.class, ()
-> {
+ HllSketchMergeAggregatorFactory other = new
HllSketchMergeAggregatorFactory(
+ NAME + "-diff",
+ FIELD_NAME,
+ LG_K,
+ TGT_HLL_TYPE,
+ STRING_ENCODING,
+ SHOULD_FINALIZE,
+ ROUND
+ );
+ targetRound.getMergingFactory(other);
+ });
}
- @Test(expected = AggregatorFactoryNotMergeableException.class)
- public void testGetMergingFactoryBadType() throws Exception
+ @Test
+ public void testGetMergingFactoryBadType()
{
- HllSketchBuildAggregatorFactory other = new
HllSketchBuildAggregatorFactory(
- NAME,
- FIELD_NAME,
- LG_K,
- TGT_HLL_TYPE,
- STRING_ENCODING,
- SHOULD_FINALIZE,
- ROUND
- );
- targetRound.getMergingFactory(other);
+ Assertions.assertThrows(AggregatorFactoryNotMergeableException.class, ()
-> {
+ HllSketchBuildAggregatorFactory other = new
HllSketchBuildAggregatorFactory(
+ NAME,
+ FIELD_NAME,
+ LG_K,
+ TGT_HLL_TYPE,
+ STRING_ENCODING,
+ SHOULD_FINALIZE,
+ ROUND
+ );
+ targetRound.getMergingFactory(other);
+ });
}
- @Test(expected = AggregatorFactoryNotMergeableException.class)
- public void testGetMergingFactoryDifferentStringEncoding() throws Exception
+ @Test
+ public void testGetMergingFactoryDifferentStringEncoding()
{
- HllSketchMergeAggregatorFactory other = new
HllSketchMergeAggregatorFactory(
- NAME,
- FIELD_NAME,
- LG_K,
- TGT_HLL_TYPE,
- StringEncoding.UTF8,
- SHOULD_FINALIZE,
- ROUND
- );
- HllSketchAggregatorFactory result = (HllSketchAggregatorFactory)
targetRound.getMergingFactory(other);
- Assert.assertEquals(LG_K, result.getLgK());
+ Assertions.assertThrows(AggregatorFactoryNotMergeableException.class, ()
-> {
+ HllSketchMergeAggregatorFactory other = new
HllSketchMergeAggregatorFactory(
+ NAME,
+ FIELD_NAME,
+ LG_K,
+ TGT_HLL_TYPE,
+ StringEncoding.UTF8,
+ SHOULD_FINALIZE,
+ ROUND
+ );
+ HllSketchAggregatorFactory result = (HllSketchAggregatorFactory)
targetRound.getMergingFactory(other);
+ Assertions.assertEquals(LG_K, result.getLgK());
Review Comment:
Fixed in commit
[6d8055b3f8](https://github.com/apache/druid/commit/6d8055b3f8). The
unreachable `result` assignment and assertion were removed from
`testGetMergingFactoryDifferentStringEncoding`; the lambda now only invokes the
method expected to throw.
The focused `HllSketchMergeAggregatorFactoryTest` run passed all 18 tests.
The combined focused DataSketches command passed 86 tests, and Maven Checkstyle
and SpotBugs passed with 0 violations/errors.
--
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]
