Skip to content

Conversation

@dain
Copy link
Member

@dain dain commented Jun 19, 2025

Description

  • Decouple DynamicFilterService and SqlQueryExecution
  • When updating query info, return existing final info if already set

Release notes

(X) This is not user-visible or is docs only, and no release notes are required.

dain added 2 commits June 19, 2025 10:34
DynamicFilterService does not need to depend on SqlQueryExecution and
this makes the code less readable.
@dain dain requested a review from electrum June 19, 2025 17:49
@cla-bot cla-bot bot added the cla-signed label Jun 19, 2025
@electrum electrum requested a review from Copilot June 19, 2025 17:51
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 decouples DynamicFilterService from SqlQueryExecution and ensures that once a query’s final info is set, subsequent updates return the existing info.

  • Change registerQuery signature to accept Session and PlanNode instead of SqlQueryExecution
  • Update SqlQueryExecution to call the new registerQuery overload
  • Modify QueryStateMachine.updateQueryInfo to reuse an already-set final QueryInfo

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
core/trino-main/src/main/java/io/trino/server/DynamicFilterService.java Removed dependency on SqlQueryExecution and updated method signature for registerQuery
core/trino-main/src/main/java/io/trino/execution/SqlQueryExecution.java Updated call to dynamicFilterService.registerQuery with new parameters
core/trino-main/src/main/java/io/trino/execution/QueryStateMachine.java Enhanced updateQueryInfo to return existing final info if already set
Comments suppressed due to low confidence (1)

core/trino-main/src/main/java/io/trino/execution/QueryStateMachine.java:1390

  • Consider adding a unit test to verify that updateQueryInfo returns the existing final QueryInfo when finalQueryInfo is already set to prevent regressions in this new behavior.
            if (!finalQueryInfo.compareAndSet(Optional.empty(), Optional.of(queryInfo))) {

}

dynamicFilterService.registerQuery(this, plan.getRoot());
dynamicFilterService.registerQuery(getSession(), getQueryPlan().orElseThrow().getRoot(), plan.getRoot());
Copy link

Copilot AI Jun 19, 2025

Choose a reason for hiding this comment

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

[nitpick] Extract getQueryPlan().orElseThrow().getRoot() into a local variable to avoid repeated Optional unwrapping in the method call for clarity and potential reuse.

Suggested change
dynamicFilterService.registerQuery(getSession(), getQueryPlan().orElseThrow().getRoot(), plan.getRoot());
var queryPlanRoot = getQueryPlan().orElseThrow().getRoot();
dynamicFilterService.registerQuery(getSession(), queryPlanRoot, plan.getRoot());

Copilot uses AI. Check for mistakes.
@dain dain merged commit e12d382 into trinodb:master Jun 19, 2025
185 of 186 checks passed
@github-actions github-actions bot added this to the 477 milestone Jun 19, 2025
@dain dain deleted the cleanup branch June 20, 2025 20:59
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.

2 participants