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.

Sign up for GitHub

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

Jump to bottom

Add page information to SqlStatementResource API #14512

Merged
merged 5 commits into from
Jul 3, 2023
Merged

Add page information to SqlStatementResource API #14512

merged 5 commits into from
Jul 3, 2023

Conversation

adarshsanjeev
Copy link
Contributor

@adarshsanjeev adarshsanjeev commented Jun 30, 2023 •
edited
Loading

This PR makes a few changes to the SqlStatementResource

  • Changes the get results API in SqlStatementResource to take a page number instead of row/offset.
  • Adds "pages" containing information on each page to the results status.
  • Update the "numRows" and "sizeInByes" to "numTotalRows" and "totalSizeInBytes" respectively, which are totalled across all pages.

Payload:

{
    "queryId": "query-14527e43-d031-41f2-a75b-e36565eacc1c",
     ...
    "result": {
        "numTotalRows": 2,
        "totalSizeInBytes": 528,
        "dataSource": "__query_select",
        "sampleRecords": [ ... ],
        "pages": [
            {
                "numRows": 2,
                "sizeInBytes": 528,
                "id": 0
            }
        ]
    }
}

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

cryptoe
cryptoe reviewed Jul 2, 2023
View reviewed changes
Copy link
Contributor

@cryptoe cryptoe left a comment

Choose a reason for hiding this comment

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

Thanks for this. Left some comments.

...re/multi-stage-query/src/main/java/org/apache/druid/msq/sql/entity/ResultSetInformation.java Outdated Show resolved Hide resolved
...re/multi-stage-query/src/main/java/org/apache/druid/msq/sql/entity/ResultSetInformation.java Show resolved Hide resolved
...multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java Outdated Show resolved Hide resolved
...multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java
return buildNonOkResponse(
DruidException.forPersona(DruidException.Persona.USER)
.ofCategory(DruidException.Category.INVALID_INPUT)
.build(
"numRows cannot be negative. Please pass a positive number."
"Page cannot be negative. Please pass a positive number."
)
);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

If the page does not exist its also an error.
For eg: pageId=999 is passed but we only have 2 pages in the result, its also a invalid input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added this check below as "if page > 0" for the report since we don't really have a list of pages to check the index for yet and manually creating a list of a single page to check this doesn't make sense

...multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java
isSelectQuery ? SqlStatementResourceHelper.getResults(payload).orElse(null) : null,
ImmutableList.of(
new PageInformation(
rowsAndSize.orElse(new Pair<>(null, null)).lhs,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be populate from the counters.
We need to rework

SqlStatementResourceHelper.getRowsAndSizeFromPayload(
          payload,
          isSelectQuery
      );

so its returns pageInformation.
The next set of changes that I am making then would not touch this piece of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might need to be handled at least partially in the next change, as counters don't really mean anything for results if the destination is the task report.

@cryptoe cryptoe added the Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 label Jul 3, 2023
@cryptoe cryptoe added this to the 27.0 milestone Jul 3, 2023
@cryptoe cryptoe added the Needs web console change Backend API changes that would benefit from frontend support in the web console label Jul 3, 2023
cryptoe
cryptoe approved these changes Jul 3, 2023
View reviewed changes
Copy link
Contributor

@cryptoe cryptoe left a comment

Choose a reason for hiding this comment

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

Changes LGTM.
Thanks @adarshsanjeev

@cryptoe cryptoe merged commit 27a70d5 into apache:master Jul 3, 2023
@vogievetsky vogievetsky mentioned this pull request Jul 7, 2023
Merged
abhishekagarwal87 pushed a commit that referenced this pull request Jul 17, 2023
@vogievetsky
Web console: catchup to all the backend changes (#14540)
d5f6749 
This PR catches the console up to all the backend changes for Druid 27

Specifically:

Add page information to SqlStatementResource API #14512
Allow empty tiered replicants map for load rules #14432
Adding Interactive API's for MSQ engine #14416
Add replication factor column to sys table #14403
Account for data format and compression in MSQ auto taskAssignment #14307
Errors take 3 #14004
AmatyaAvadhanula pushed a commit to AmatyaAvadhanula/druid that referenced this pull request Jul 17, 2023
@vogievetsky @AmatyaAvadhanula
Web console: catchup to all the backend changes (apache#14540)
940507c 
This PR catches the console up to all the backend changes for Druid 27

Specifically:

Add page information to SqlStatementResource API apache#14512
Allow empty tiered replicants map for load rules apache#14432
Adding Interactive API's for MSQ engine apache#14416
Add replication factor column to sys table apache#14403
Account for data format and compression in MSQ auto taskAssignment apache#14307
Errors take 3 apache#14004
@AmatyaAvadhanula AmatyaAvadhanula mentioned this pull request Jul 17, 2023
Merged
abhishekagarwal87 pushed a commit that referenced this pull request Jul 17, 2023
@AmatyaAvadhanula @vogievetsky
Web console: catchup to all the backend changes (#14540) (#14596)
231854f 
This PR catches the console up to all the backend changes for Druid 27

Specifically:

Add page information to SqlStatementResource API #14512
Allow empty tiered replicants map for load rules #14432
Adding Interactive API's for MSQ engine #14416
Add replication factor column to sys table #14403
Account for data format and compression in MSQ auto taskAssignment #14307
Errors take 3 #14004

Co-authored-by: Vadim Ogievetsky <vadim@ogievetsky.com>
sergioferragut pushed a commit to sergioferragut/druid that referenced this pull request Jul 21, 2023
@adarshsanjeev @sergioferragut
Add page information to SqlStatementResource API (apache#14512)
939e320 
    * Changes the get results API in SqlStatementResource to take a page number instead of row/offset.
    * Adds "pages" containing information on each page to the results status.
    * Update the "numRows" and "sizeInByes" to "numTotalRows" and "totalSizeInBytes" respectively, which are totalled across all pages.
sergioferragut pushed a commit to sergioferragut/druid that referenced this pull request Jul 21, 2023
@vogievetsky @sergioferragut
Web console: catchup to all the backend changes (apache#14540)
d52c3ff 
This PR catches the console up to all the backend changes for Druid 27

Specifically:

Add page information to SqlStatementResource API apache#14512
Allow empty tiered replicants map for load rules apache#14432
Adding Interactive API's for MSQ engine apache#14416
Add replication factor column to sys table apache#14403
Account for data format and compression in MSQ auto taskAssignment apache#14307
Errors take 3 apache#14004
@AmatyaAvadhanula AmatyaAvadhanula mentioned this pull request Aug 6, 2023
Closed
@vogievetsky vogievetsky removed the Needs web console change Backend API changes that would benefit from frontend support in the web console label Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LakshSingla LakshSingla LakshSingla left review comments

@cryptoe cryptoe cryptoe approved these changes

Assignees
No one assigned
Labels
Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262
Projects
None yet
Milestone
27.0
Development

Successfully merging this pull request may close these issues.
4 participants
@adarshsanjeev @cryptoe @LakshSingla @vogievetsky