Make query JSON more compact for UI #26988
Conversation
Reviewer's GuideThis update adds a configurable Jackson serializer for DataSize to produce succinct, human-readable sizes and wires it into the server JSON binder, then updates the UI query endpoint to use a custom JsonCodec that enables this serializer when returning query info as JSON. Sequence diagram for query info JSON serialization with succinct DataSizesequenceDiagram
participant Client
participant UiQueryResource
participant JsonCodec_QueryInfo
participant DataSizeSerializer
participant ObjectMapper
Client->>UiQueryResource: GET /ui/query/{queryId}
UiQueryResource->>JsonCodec_QueryInfo: toJson(queryInfo)
JsonCodec_QueryInfo->>DataSizeSerializer: serialize DataSize fields (succinct enabled)
DataSizeSerializer->>JsonCodec_QueryInfo: returns human-readable DataSize
JsonCodec_QueryInfo->>UiQueryResource: returns JSON
UiQueryResource->>Client: returns JSON response with human-readable DataSize
Class diagram for the new DataSizeSerializer and related changesclassDiagram
class DataSizeSerializer {
+SUCCINCT_DATA_SIZE_ENABLED : String
+serialize(dataSize: DataSize, jsonGenerator: JsonGenerator, serializerProvider: SerializerProvider)
}
class JsonSerializer_DataSize
DataSizeSerializer --|> JsonSerializer_DataSize
class UiQueryResource {
-queryInfoCodec: JsonCodec<QueryInfo>
+UiQueryResource(objectMapper, dispatchManager, accessControl, sessionContextFactory)
-buildQueryInfoCodec(objectMapper): JsonCodec<QueryInfo>
}
UiQueryResource ..> DataSizeSerializer : uses
class JsonCodecFactory {
+jsonCodec(type: Class<T>): JsonCodec<T>
+prettyPrint()
}
UiQueryResource ..> JsonCodecFactory : uses
class DataSize {
+succinct(): DataSize
+toString(): String
}
DataSizeSerializer ..> DataSize : serializes
class ServerMainModule {
+setup(Binder binder)
}
ServerMainModule ..> DataSizeSerializer : binds
class JsonCodec_T {
+toJson(value: T): String
}
class JsonCodec_QueryInfo
UiQueryResource ..> JsonCodec_QueryInfo : uses
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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `core/trino-main/src/main/java/io/trino/server/DataSizeSerializer.java:29` </location>
<code_context>
+ public static final String SUCCINCT_DATA_SIZE_ENABLED = "dataSize.succinct.enabled";
+
+ @Override
+ public void serialize(DataSize dataSize, JsonGenerator jsonGenerator, SerializerProvider serializerProvider)
+ throws IOException
+ {
</code_context>
<issue_to_address>
**suggestion:** Consider null handling for DataSize in serializer.
Explicitly check for null DataSize and handle it appropriately to prevent NullPointerExceptions.
</issue_to_address>
### Comment 2
<location> `core/trino-main/src/main/java/io/trino/server/DataSizeSerializer.java:32-34` </location>
<code_context>
+ public void serialize(DataSize dataSize, JsonGenerator jsonGenerator, SerializerProvider serializerProvider)
+ throws IOException
+ {
+ if (serializerProvider.getAttribute(SUCCINCT_DATA_SIZE_ENABLED) == Boolean.TRUE) {
+ jsonGenerator.writeString(dataSize.succinct().toString());
+ return;
</code_context>
<issue_to_address>
**suggestion:** Type comparison for attribute may be too strict.
Using 'Boolean.TRUE.equals(...)' is safer, as it correctly handles boxed Booleans and avoids issues with strict '==' comparison.
```suggestion
if (Boolean.TRUE.equals(serializerProvider.getAttribute(SUCCINCT_DATA_SIZE_ENABLED))) {
jsonGenerator.writeString(dataSize.succinct().toString());
return;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
core/trino-main/src/main/java/io/trino/server/DataSizeSerializer.java
Outdated
Show resolved
Hide resolved
wendigo
force-pushed
the
serafin/data-size-handling
branch
2 times, most recently
from
1fd23d4 to
17bcbf1
Compare
wendigo
changed the title
wendigo
force-pushed
the
serafin/data-size-handling
branch
2 times, most recently
from
737625f to
de68b0d
Compare
|
I've added digest pruning so that query json will be smaller :) |
|
The query json is not meant to be for human consumption. |
|
@martint but it is used by humans :) (at least by some of us) |
core/trino-web-ui/src/main/resources/webapp/src/components/QueryList.jsx
Outdated
Show resolved
Hide resolved
We use it all the time to debug performance issues. The fields removed here don't make the json any less consume-able by machines and bring down the size of the json pulled by the UI or any other user. |
wendigo
force-pushed
the
serafin/data-size-handling
branch
from
bf66e16 to
4ae62b8
Compare
|
UI changes dropped (will revisit later with better proposal) |
raunaqmorarka
changed the title
wendigo
force-pushed
the
serafin/data-size-handling
branch
from
4ae62b8 to
19dfed6
Compare
|
It's not just removing the t-digests. It's changing the encoding of data sizes to use the succinct representation, which is lossy. |
The succinct representation should only be used on the final DataSize values that go out from the coordinator REST endpoint. I think we want even the UI to show succinct representation as that is easier to read. |
|
@martint it is lossy but only applies to the query json download. Not to every other serialization path. These are final values, not ones before aggregation. For query json order of magnitude matters. Not down to a single byte values. Eventually we could make this configurable using server config but I doubt that this would be used. |
|
The UI should be the one to decide how to render the data sizes. For example, if it's trying to draw a chart, it may need to normalize to a different unit that the most succinct one for one metric.
That's not what I see in the code. It seems to apply to all requests to |
| // Do not output @class property for metric types | ||
| mapper.addMixIn(Metric.class, DropTypeInfo.class); | ||
| // Do not output @type property for OperatorInfo | ||
| mapper.addMixIn(OperatorInfo.class, DropTypeInfo.class); |
There was a problem hiding this comment.
This is brittle, and will get out of sync as the QueryInfo continues to evolve.
There was a problem hiding this comment.
If we want a dump in a human-consumable format, we should consider introducing an explicit API for that instead of monkey-patching the representation produced by the programmatic API used by the UI.
|
@martint only to the |
|
Yes, sorry, that's what I meant: |
|
I narrowed it down only to the |
- Serialize data sizes using succinct form (only query json download) - Do not output raw digest in query.json - Do not serialize type information for metrics - Do not serialize type information for operator infos - Cleanup old prunning logic to reduce copying
wendigo
force-pushed
the
serafin/data-size-handling
branch
from
6e98bb4 to
c3d385b
Compare
wendigo
changed the title
DataSize are serialized to exact bytes to ensure that aggregated values are exact and errors do not accumulate. For the human ingestion it is better to format to most succinct string.
Description
Additional context and related issues
Release notes
(x) 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.
( ) Release notes are required, with the following suggested text:
Summary by Sourcery
Enable succinct, human-readable DataSize formatting in UI JSON responses for QueryInfo.
Enhancements: