Copilot commented on code in PR #17184:
URL: https://github.com/apache/pinot/pull/17184#discussion_r2513594790
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/operator/LeafOperatorTest.java:
##########
@@ -55,9 +62,7 @@
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
-import static org.testng.Assert.assertEquals;
-import static org.testng.Assert.assertSame;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.*;
Review Comment:
Replace wildcard import with explicit imports. Wildcard imports can make it
unclear which assertions are being used and violate the project's preference
for explicit imports.
```suggestion
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertTrue;
import static org.testng.Assert.assertFalse;
import static org.testng.Assert.assertNull;
import static org.testng.Assert.assertNotNull;
import static org.testng.Assert.fail;
```
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/operator/LeafOperatorTest.java:
##########
@@ -401,4 +406,90 @@ void execute() {
operator.close();
}
+
+ private static OpChainExecutionContext createMinimalContext() {
+ MailboxService mailboxService =
+ new MailboxService("localhost", 0, InstanceType.SERVER, new
PinotConfiguration());
+ StageMetadata stageMetadata =
+ new StageMetadata(0, Collections.emptyList(), new
HashMap<>(Map.of("tableName", "t")));
+ WorkerMetadata workerMetadata = new WorkerMetadata(1, new HashMap<>());
+ return new OpChainExecutionContext(mailboxService, 1L, "cid",
System.currentTimeMillis() + 60_000,
+ System.currentTimeMillis() + 60_000, "broker", Collections.emptyMap(),
stageMetadata, workerMetadata, null,
+ true);
+ }
+
+ private static LeafOperator createLeafOperator() {
+ OpChainExecutionContext context = createMinimalContext();
+ List<ServerQueryRequest> requests = new ArrayList<>();
+ requests.add(null); // not used by this test
Review Comment:
Adding null to the requests list with a comment 'not used by this test'
suggests the test setup is incomplete. Consider using a proper mock or creating
a minimal valid ServerQueryRequest to make the test more robust and
representative of actual usage.
```suggestion
requests.add(mock(ServerQueryRequest.class)); // minimal valid mock, not
used by this test
```
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/LeafOperator.java:
##########
@@ -292,7 +293,11 @@ void cancelSseTasks() {
}
private synchronized void mergeExecutionStats(Map<String, String>
executionStats) {
- for (Map.Entry<String, String> entry : executionStats.entrySet()) {
+ Map<String, String> snapshot;
+ synchronized (executionStats) {
+ snapshot = new HashMap<>(executionStats);
+ }
Review Comment:
The method is already synchronized on `this`, but then synchronizes on
`executionStats`. If `executionStats` is also synchronized elsewhere, this
could lead to deadlock. Consider documenting the locking order or using a
single lock strategy. Additionally, if `executionStats` is always synchronized
before being passed to this method, the inner synchronization may be redundant.
```suggestion
Map<String, String> snapshot = new HashMap<>(executionStats);
```
--
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]
