Add finishingTime to io.trino.spi.eventlistener.QueryStatistics #27202
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR introduces a new finishingTime field to the QueryStatistics SPI and propagates it through constructors and JSON serialization, updates compatibility metadata, integrates the metric into PlanPrinter and QueryMonitor, and adjusts downstream tests to accommodate the extra parameter. Class diagram for updated QueryStatistics with finishingTimeclassDiagram
class QueryStatistics {
- Optional<Duration> outputBlockedTime
- Optional<Duration> failedOutputBlockedTime
- Optional<Duration> physicalInputReadTime
+ Optional<Duration> finishingTime
- long peakUserMemoryBytes
- long peakTaskUserMemory
- long peakTaskTotalMemory
...
+ getFinishingTime(): Optional<Duration>
...
}
Class diagram for updated QueryMonitor.createQueryStatistics()classDiagram
class QueryMonitor {
+ queryImmediateFailureEvent(...)
- createQueryStatistics(QueryInfo): QueryStatistics
}
class QueryStatistics {
+ QueryStatistics(..., Optional<Duration> finishingTime, ...)
}
QueryMonitor --> QueryStatistics: creates
Class diagram for PlanPrinter using finishingTimeclassDiagram
class PlanPrinter {
+ textDistributedPlan(..., QueryStats queryStats, ...): String
}
class QueryStats {
+ getFinishingTime(): Duration
}
PlanPrinter --> QueryStats: uses finishingTime
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The QueryStatistics constructor keeps growing with positional parameters—consider refactoring to a builder or grouping related fields to improve readability and avoid ordering mistakes.
- In PlanPrinter, make sure you handle cases where finishingTime is absent so you don’t end up calling convertToMostSuccinctTimeUnit on an empty Optional and crash.
- Verify that all event listener plugins (beyond the ones covered by tests) have been updated to propagate the new finishingTime field for consistency across all listeners.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The QueryStatistics constructor keeps growing with positional parameters—consider refactoring to a builder or grouping related fields to improve readability and avoid ordering mistakes.
- In PlanPrinter, make sure you handle cases where finishingTime is absent so you don’t end up calling convertToMostSuccinctTimeUnit on an empty Optional and crash.
- Verify that all event listener plugins (beyond the ones covered by tests) have been updated to propagate the new finishingTime field for consistency across all listeners.
## Individual Comments
### Comment 1
<location> `plugin/trino-mysql-event-listener/src/test/java/io/trino/plugin/eventlistener/mysql/TestMysqlEventListener.java:109` </location>
<code_context>
Optional.of(ofMillis(113)),
Optional.of(ofMillis(114)),
Optional.of(ofMillis(115)),
+ Optional.of(ofMillis(116)),
115L,
116L,
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding assertions to verify the correct handling and propagation of finishingTime.
Please add assertions to confirm that finishingTime is correctly propagated and stored by the event listener, covering both present and empty cases.
Suggested implementation:
```java
Optional.of(ofMillis(113)),
Optional.of(ofMillis(114)),
Optional.of(ofMillis(115)),
Optional.of(ofMillis(116)),
115L,
116L,
117L,
Optional.empty(),
Optional.empty(),
Optional.empty(),
Optional.empty(),
115L,
116L,
117L,
// Assertions for finishingTime propagation and storage
// Present cases
assertTrue(eventListener.getFinishingTime(0).isPresent());
assertEquals(eventListener.getFinishingTime(0).get(), ofMillis(113));
assertTrue(eventListener.getFinishingTime(1).isPresent());
assertEquals(eventListener.getFinishingTime(1).get(), ofMillis(114));
assertTrue(eventListener.getFinishingTime(2).isPresent());
assertEquals(eventListener.getFinishingTime(2).get(), ofMillis(115));
assertTrue(eventListener.getFinishingTime(3).isPresent());
assertEquals(eventListener.getFinishingTime(3).get(), ofMillis(116));
// Empty cases
assertFalse(eventListener.getFinishingTime(4).isPresent());
assertFalse(eventListener.getFinishingTime(5).isPresent());
assertFalse(eventListener.getFinishingTime(6).isPresent());
assertFalse(eventListener.getFinishingTime(7).isPresent());
```
- Ensure that `eventListener.getFinishingTime(int index)` is a valid method for accessing the finishingTime for each event. If not, you may need to adjust the accessor method to match your event storage structure.
- If the assertions are not in a test method, wrap them in an appropriate test method (e.g., `@Test public void testFinishingTimePropagation() { ... }`).
- Import necessary assertion methods: `import static org.junit.jupiter.api.Assertions.*;`
</issue_to_address>
### Comment 2
<location> `plugin/trino-mysql-event-listener/src/test/java/io/trino/plugin/eventlistener/mysql/TestMysqlEventListener.java:275` </location>
<code_context>
Optional.empty(),
Optional.empty(),
Optional.empty(),
</code_context>
<issue_to_address>
**suggestion (testing):** Test coverage for edge cases: finishingTime as Optional.empty.
Please add a test that asserts correct behavior when finishingTime is absent, ensuring no exceptions are thrown and output remains valid.
Suggested implementation:
```java
Optional.of(ofMillis(113)),
Optional.of(ofMillis(114)),
Optional.of(ofMillis(115)),
Optional.of(ofMillis(116)),
115L,
116L,
117L,
Optional.empty(),
Optional.empty(),
Optional.empty(),
Optional.empty(),
115L,
116L,
117L,
@Test
public void testFinishingTimeAbsentDoesNotThrow() {
// Arrange: create an event with finishingTime as Optional.empty()
Optional<Long> finishingTime = Optional.empty();
// You may need to adjust the constructor and parameters below to match your event class
MysqlEvent event = new MysqlEvent(
/* other required parameters */,
finishingTime
);
// Act & Assert: ensure no exception is thrown and output is valid
try {
String output = event.toString(); // or event.serialize(), etc.
assertNotNull(output, "Output should not be null when finishingTime is absent");
// Optionally, check that output does not contain finishingTime
assertFalse(output.contains("finishingTime"), "Output should not contain finishingTime when absent");
} catch (Exception e) {
fail("No exception should be thrown when finishingTime is Optional.empty()");
}
}
```
- Replace `MysqlEvent` and its constructor parameters with the actual event class and required arguments in your codebase.
- If your event handling or serialization is done differently, adjust the test accordingly to match your implementation.
- Ensure necessary imports are present: `import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.*;`
</issue_to_address>
### Comment 3
<location> `plugin/trino-http-event-listener/src/test/java/io/trino/plugin/httpquery/TestHttpEventListener.java:153` </location>
<code_context>
Optional.empty(),
Optional.empty(),
Optional.empty(),
</code_context>
<issue_to_address>
**suggestion (testing):** Missing positive test for finishingTime with a non-empty value.
Please add a test for finishingTime with a non-empty value to verify correct handling and serialization/deserialization.
Suggested implementation:
```java
Optional.empty(),
Optional.empty(),
Optional.empty(),
Optional.empty(),
0L,
0L,
0L,
// Positive test for finishingTime with a non-empty value
Optional.of(123456789L), // finishingTime
Optional.empty(),
Optional.empty(),
Optional.empty(),
0L,
0L,
0L
```
You will need to:
1. Add assertions in the corresponding test method to verify that the non-empty `finishingTime` value is correctly handled, serialized, and deserialized.
2. If the test data is used in a parameterized test or a helper method, ensure the new case is included in the test execution and validation logic.
</issue_to_address>
### Comment 4
<location> `plugin/trino-http-server-event-listener/src/test/java/io/trino/plugin/httpquery/TestHttpServerEventListener.java:141` </location>
<code_context>
Optional.empty(),
Optional.empty(),
Optional.empty(),
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for finishingTime with a non-empty value.
Add a test to ensure the event listener processes a non-empty finishingTime value correctly.
Suggested implementation:
```java
Optional.empty(),
Optional.empty(),
Optional.empty(),
Optional.of(Instant.parse("2024-06-01T12:34:56.789Z")),
0L,
0L,
0L,
```
You should also add an assertion in your test method to verify that the event listener correctly processes the non-empty finishingTime value. For example, check that the processed event contains the expected finishingTime.
If your test class uses a builder or a helper method to construct the event, ensure that it supports setting finishingTime.
</issue_to_address>
### Comment 5
<location> `plugin/trino-kafka-event-listener/src/test/java/io/trino/plugin/eventlistener/kafka/TestUtils.java:119` </location>
<code_context>
Optional.empty(),
Optional.empty(),
Optional.empty(),
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for finishingTime with a non-empty value.
Add a test verifying that a non-empty finishingTime is handled and forwarded as expected by the Kafka event listener.
Suggested implementation:
```java
Optional.empty(),
Optional.empty(),
Optional.empty(),
Optional.empty(),
0L,
0L,
0L,
@Test
public void testFinishingTimeNonEmpty() {
Optional<Long> finishingTime = Optional.of(123456789L);
// Assuming createEvent is a method that takes finishingTime as a parameter
KafkaEvent event = createEvent(
/* other params */,
finishingTime,
/* other params */
);
assertTrue(event.getFinishingTime().isPresent());
assertEquals(123456789L, event.getFinishingTime().get().longValue());
// Add additional assertions as needed to verify forwarding
}
```
- You may need to adjust the parameters in `createEvent` to match your actual method signature.
- If the test class uses a different event creation or assertion pattern, adapt the test accordingly.
- Ensure that the Kafka event listener is being invoked and that the test verifies the forwarding of the finishingTime value.
</issue_to_address>
### Comment 6
<location> `plugin/trino-openlineage/src/test/java/io/trino/plugin/openlineage/TrinoEventData.java:112` </location>
<code_context>
Optional.empty(),
Optional.empty(),
Optional.empty(),
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for finishingTime with a non-empty value.
Add a test to confirm that OpenLineage correctly processes a non-empty finishingTime value.
Suggested implementation:
```java
Optional.empty(),
Optional.empty(),
Optional.empty(),
Optional.empty(),
0L,
0L,
0L,
// --- Test for non-empty finishingTime ---
// This test ensures OpenLineage processes a non-empty finishingTime value correctly.
@Test
public void testNonEmptyFinishingTime() {
Optional<Long> finishingTime = Optional.of(1680000000000L);
TrinoEventData eventData = new TrinoEventData(
/* other params */,
finishingTime,
/* other params */
);
assertTrue(eventData.getFinishingTime().isPresent());
assertEquals(1680000000000L, eventData.getFinishingTime().get().longValue());
}
```
You may need to adjust the constructor parameters for `TrinoEventData` in the new test to match your actual implementation. Ensure that the test method is placed within the appropriate test class and that imports for `@Test`, `Optional`, and assertions are present.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
raunaqmorarka
force-pushed
the
finishing-time
branch
from
d0721b4 to
442b96b
Compare
wendigo
approved these changes
Nov 4, 2025
lukasz-stec
approved these changes
Nov 4, 2025
Contributor
|
@raunaqmorarka Do we have section "## Event listener" in release note, this should be "spi" ? |
Member
Author
It should go in general |
Contributor
|
Thanks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:
Summary by Sourcery
Add a new finishingTime metric to the QueryStatistics SPI and propagate it through the system so that event listeners and plan printing can include query finishing time.
New Features:
Enhancements:
Tests: