Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure cached query results are discarded #11452

Merged
merged 1 commit into from
Mar 16, 2022

Conversation

linzebing
Copy link
Member

Description

Is this change a fix, improvement, new feature, refactoring, or other?

Fix.

Previous attempt was da2cd0a, but it doesn't work out in the following cases:

  1. If query only needs to call getNextResult once, on the first call lastResult is null, we will not have the next token to clear the cached result
  2. It's possible for exchange to return empty result yet not finished. In this case we will not have the next token to clear the cached result either.

This PR improves the logic to check resultRows, as lastResult will be set to a QueryResults containing resultRows at the end. If it's non-empty, then we need next token to clear the cached result. Note that the queryInfo.getOutputStage().isPresent() check is needed as

// For queries with no output, return a fake boolean result for clients that require it.
if ((queryInfo.getState() == QueryState.FINISHED) && queryInfo.getOutputStage().isEmpty()) {
return queryResultRowsBuilder(session)
.withSingleBooleanValue(createColumn("result", BooleanType.BOOLEAN), true)
.build();
}

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Core engine.

How would you describe this change to a non-technical end user or system administrator?

This ensures cached query results are discarded upon completion, which improves memory usage of coordinator.

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

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

@cla-bot cla-bot bot added the cla-signed label Mar 13, 2022
@linzebing linzebing requested review from arhimondr and losipiuk March 13, 2022 23:51
@findepi findepi requested a review from electrum March 14, 2022 08:43
@linzebing linzebing force-pushed the clear-cached-results branch 2 times, most recently from 507c349 to 121a5a7 Compare March 15, 2022 06:22
@github-actions github-actions bot added the docs label Mar 15, 2022
@linzebing linzebing force-pushed the clear-cached-results branch from 121a5a7 to d2c7f63 Compare March 15, 2022 15:10
@linzebing linzebing removed the docs label Mar 15, 2022
@arhimondr arhimondr merged commit 00cc00e into trinodb:master Mar 16, 2022
@github-actions github-actions bot added this to the 374 milestone Mar 16, 2022
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