Partition spooled pages while encoding data to segments #26013
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
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
wendigo
force-pushed
the
serafin/partition-spooling-pages
branch
from
d687e96 to
c7b6bd0
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR partitions large spooled metadata pages into smaller segments to prevent oversized segments that clients may have difficulty reading. Key changes include updating the metadata serialization/deserialization API to work with lists of blocks, revising tests accordingly, and updating the operator logic to handle both spooled and inlined pages with segment counters.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| core/trino-main/src/test/java/io/trino/server/protocol/spooling/TestSpooledMetadataBlockSerde.java | Updated tests to compare list-based metadata serialization/deserialization. |
| core/trino-main/src/test/java/io/trino/operator/TestSpoolingPagePartitioner.java | Added tests for partitioning pages based on size thresholds. |
| core/trino-main/src/sql/planner/LocalExecutionPlanner.java | Modified validation to support spooled metadata pages with multiple positions. |
| core/trino-main/src/server/protocol/spooling/SpoolingQueryDataProducer.java | Updated to iterate over multiple metadata blocks from deserialization. |
| core/trino-main/src/server/protocol/spooling/SpooledMetadataBlockSerde.java | Revised serialize/deserialize API to support multiple metadata blocks. |
| core/trino-main/src/server/protocol/spooling/SpooledMetadataBlock.java | Removed the default serialize method to rely on the updated API. |
| core/trino-main/src/operator/SpoolingPagePartitioner.java | Introduced page partitioning logic to split large pages, ensuring each partition meets defined thresholds. |
| core/trino-main/src/operator/OutputSpoolingOperatorFactory.java | Adjusted operator behavior to use the new API, added segment counters, and routed both spooled and inlined outputs through the serialization process. |
Comments suppressed due to low confidence (2)
core/trino-main/src/main/java/io/trino/server/protocol/spooling/SpooledMetadataBlockSerde.java:94
- Update the method documentation for serialize/deserialize to clearly state that they now support multiple SpooledMetadataBlock instances, and outline any backwards compatibility considerations.
public static List<SpooledMetadataBlock> deserialize(Page page)
core/trino-main/src/main/java/io/trino/operator/OutputSpoolingOperatorFactory.java:328
- Ensure downstream consumers expecting a serialized Page instance are updated to handle the new inline API output (i.e. a List wrapped by the serialize method). Clarifying this in both the code and documentation can prevent integration issues.
private List<SpooledMetadataBlock> inline(List<Page> pages)
core/trino-main/src/main/java/io/trino/operator/SpoolingPagePartitioner.java
Show resolved
Hide resolved
losipiuk
reviewed
Jun 14, 2025
|
|
||
| private SpoolingPagePartitioner() {} | ||
|
|
||
| public static List<List<Page>> partition(List<Page> pages, long targetSize) |
There was a problem hiding this comment.
We buffer multiple pages
There was a problem hiding this comment.
I get it - but why not partition page by page. Passing multiple pages makes interface more complex. Let me read deeper
core/trino-main/src/main/java/io/trino/operator/SpoolingPagePartitioner.java
Show resolved
Hide resolved
losipiuk
reviewed
Jun 18, 2025
Comment on lines
+152
to
+153
| private final AtomicLong spooledSegmentsCount = new AtomicLong(); | ||
| private final AtomicLong inlinedSegmentsCount = new AtomicLong(); |
There was a problem hiding this comment.
those could be added in separate commit
losipiuk
reviewed
Jun 18, 2025
core/trino-main/src/main/java/io/trino/operator/OutputSpoolingOperatorFactory.java
Outdated
Show resolved
Hide resolved
wendigo
force-pushed
the
serafin/partition-spooling-pages
branch
from
c7b6bd0 to
dcdd278
Compare
This prevents dictionary blocks to expand to a single huge segment that most clients won't be able to read due to its size. This now partitions these kind of pages to smaller ones and allows the operator to output multiple spooled/inlined pages in a single getOutput call.
wendigo
force-pushed
the
serafin/partition-spooling-pages
branch
from
dcdd278 to
5116d3a
Compare
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.
This prevents dictionary blocks to expand to a single huge segment that most clients won't be able to read due to its size.
This now partitions these kind of pages to smaller ones and allows the operator to output multiple spooled/inlined pages in a single getOutput call.
Fixes #25999
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.
( ) Release notes are required, with the following suggested text: