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

Truncate responses to /v1/query #13140

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

Rramu91
Copy link

@Rramu91 Rramu91 commented Jul 11, 2022

Description

Too many of large queries, especially coming from BI tools - can slow down the loading of the UI. Hence we are truncating the responses sent to /v1/query.

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

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

How would you describe this change to a non-technical end user or system administrator?
Too many of large queries, especially coming from BI tools - can slow down the loading of the UI. Hence we are truncating the responses to /v1/query.

Related issues, pull requests, and links

Documentation

( ) 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

( ) 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 Jul 11, 2022
@posulliv
Copy link
Contributor

@Rramu91 the TrimmedBasicQueryInfo constructor already trims the query text if the query text is larger than 300 characters:

        String queryText = requireNonNull(queryInfo.getQuery(), "query is null");
        if (queryText.length() <= MAX_QUERY_PREVIEW_LENGTH) {
            this.queryTextPreview = queryText;
        }
        else {
            this.queryTextPreview = queryText.substring(0, MAX_QUERY_PREVIEW_LENGTH - 1) + " ...";
        }

Right now, there is no configuration parameter to change the value of MAX_QUERY_PREVIEW_LENGTH from 300.

This truncation only happens on the query overview page of the UI. There is no truncation of the query text on the query details page.

Are you not seeing the query text being truncated on the main UI page?

@colebow
Copy link
Member

colebow commented Jul 11, 2022

Thanks for the contribution! There are a few thoughts here, so to get into them...

  • Just as a style nit, please remember to capitalize the first word of each commit. Trino generally uses this as a style guide for commit messages.
  • If we are going to make this change, we'll need the config property to be included in the docs on this page.
  • There's likely to be a better approach to shortening a response than truncation, like compressing parts of the expression tree which are less informative. It may not get you a guarantee of under x characters, but renaming the property to mention verbosity instead of message length would work fine and probably be a bigger improvement.
  • ...but in general, we're hesitant to add more configuration properties, as they add complexity for users.

@Rramu91
Copy link
Author

Rramu91 commented Jul 12, 2022

@Rramu91 the TrimmedBasicQueryInfo constructor already trims the query text if the query text is larger than 300 characters:

        String queryText = requireNonNull(queryInfo.getQuery(), "query is null");
        if (queryText.length() <= MAX_QUERY_PREVIEW_LENGTH) {
            this.queryTextPreview = queryText;
        }
        else {
            this.queryTextPreview = queryText.substring(0, MAX_QUERY_PREVIEW_LENGTH - 1) + " ...";
        }

Right now, there is no configuration parameter to change the value of MAX_QUERY_PREVIEW_LENGTH from 300.

This truncation only happens on the query overview page of the UI. There is no truncation of the query text on the query details page.

Are you not seeing the query text being truncated on the main UI page?

@Rramu91 the TrimmedBasicQueryInfo constructor already trims the query text if the query text is larger than 300 characters:

        String queryText = requireNonNull(queryInfo.getQuery(), "query is null");
        if (queryText.length() <= MAX_QUERY_PREVIEW_LENGTH) {
            this.queryTextPreview = queryText;
        }
        else {
            this.queryTextPreview = queryText.substring(0, MAX_QUERY_PREVIEW_LENGTH - 1) + " ...";
        }

Right now, there is no configuration parameter to change the value of MAX_QUERY_PREVIEW_LENGTH from 300.

This truncation only happens on the query overview page of the UI. There is no truncation of the query text on the query details page.

Are you not seeing the query text being truncated on the main UI page?

Thanks. Since we did not have this fix merged, we fixed this internally a while ago. Query text are indeed getting truncated on the main UI page but It makes sense to add truncation in query details page as well. We have many queries with >800k chars in query text. Think it makes sense to truncate and not show full query text in the UI for these.

@@ -82,7 +85,7 @@ public List<BasicQueryInfo> getAllQueryInfo(@QueryParam("state") String stateFil
ImmutableList.Builder<BasicQueryInfo> builder = ImmutableList.builder();
for (BasicQueryInfo queryInfo : queries) {
if (stateFilter == null || queryInfo.getState() == expectedState) {
builder.add(queryInfo);
builder.add(truncateQueryInfo(queryInfo, webUiConfig.getQueryMaxDisplayLength()));
Copy link
Member

Choose a reason for hiding this comment

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

what is the purpose of truncating queries here?

Copy link
Member

@phd3 phd3 left a comment

Choose a reason for hiding this comment

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

IMO truncation on the query overview page is sufficient, as that's what will get loaded the most and the sizes add up over queries. The loading of query details will be much less frequent, with only one query string being pulled in. And the string may not be the most expensive component anyway in the data being loaded on that page.

@mosabua
Copy link
Member

mosabua commented Jan 12, 2024

👋 @Rramu91 @phd3 - this PR has become inactive. We hope you are still interested in working on it. Please let us know, and we can try to get reviewers to help with that.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

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.

None yet

5 participants