Skip to content

Conversation

@wendigo
Copy link
Contributor

@wendigo wendigo commented Mar 26, 2025

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:

## Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Mar 26, 2025
@wendigo wendigo requested a review from Copilot March 26, 2025 18:56
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the inlining behavior so that the limit on inlined pages is enforced at the pipeline level rather than per operator. Key changes include:

  • Introduction of a new PipelineSpoolingController that aggregates inlining metrics using shared atomic counters.
  • Adjustments in the SpoolingController interface and OperatorSpoolingController to align with the new design.
  • Updates to tests and operator factory code to accommodate the new pipeline-level inlining limits.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
core/trino-main/src/main/java/io/trino/operator/PipelineSpoolingController.java Adds a new controller for pipeline-level inlining with atomic counters and updated logic in nextMode and execute.
core/trino-main/src/main/java/io/trino/operator/SpoolingController.java Updates the interface with unwrap and a new MetricSnapshot record and ActionMetrics class.
core/trino-main/src/main/java/io/trino/operator/OperatorSpoolingController.java Removes inline handling in nextMode and adjusts mode switching to support the new design.
core/trino-main/src/main/java/io/trino/operator/OutputSpoolingOperatorFactory.java Refactors controller instantiation to wrap the OperatorSpoolingController with the PipelineSpoolingController.
core/trino-main/src/main/java/io/trino/operator/PipelineContext.java Introduces new atomic counters and getter methods for inlined page metrics.
core/trino-main/src/test/java/io/trino/operator/TestSpoolingController.java Updates tests to use the new PipelineSpoolingController and verifies the new behavior.
Comments suppressed due to low confidence (1)

core/trino-main/src/test/java/io/trino/operator/TestSpoolingController.java:169

  • [nitpick] Directly unwrapping to OperatorSpoolingController ties the test to an implementation detail; consider exposing a method in the SpoolingController interface to retrieve the spooled segment target to decouple tests from a specific implementation.
assertThat(controller.unwrap(OperatorSpoolingController.class).getCurrentSpooledSegmentTarget())

@wendigo wendigo force-pushed the serafin/pipeline-inlining branch from 454a5e5 to 1df02a4 Compare March 26, 2025 20:18
@wendigo wendigo requested a review from losipiuk March 26, 2025 20:23
@wendigo wendigo force-pushed the serafin/pipeline-inlining branch from 1df02a4 to d6b7b98 Compare March 27, 2025 05:23
@wendigo wendigo force-pushed the serafin/pipeline-inlining branch from d6b7b98 to a43b363 Compare March 27, 2025 14:04
@wendigo wendigo merged commit 2322adb into master Mar 27, 2025
102 checks passed
@wendigo wendigo deleted the serafin/pipeline-inlining branch March 27, 2025 15:40
@github-actions github-actions bot added this to the 475 milestone Mar 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants