Skip to content

Add page information to SqlStatementResource API#14512

Merged
cryptoe merged 5 commits intoapache:masterfrom
adarshsanjeev:page-information-sql-resource
Jul 3, 2023
Merged

Add page information to SqlStatementResource API#14512
cryptoe merged 5 commits intoapache:masterfrom
adarshsanjeev:page-information-sql-resource

Conversation

@adarshsanjeev
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread ...re/multi-stage-query/src/main/java/org/apache/druid/msq/sql/entity/ResultSetInformation.java Outdated
Comment thread ...re/multi-stage-query/src/main/java/org/apache/druid/msq/sql/entity/ResultSetInformation.java
Comment thread ...multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java Outdated
Comment thread ...multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java
)
);
}

Copy link
Copy Markdown
Contributor

@cryptoe cryptoe Jul 2, 2023

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
Copy Markdown
Contributor Author

@adarshsanjeev adarshsanjeev Jul 3, 2023

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

Comment thread ...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
Copy Markdown
Contributor

@cryptoe cryptoe Jul 2, 2023

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
Copy Markdown
Contributor Author

@adarshsanjeev adarshsanjeev Jul 3, 2023

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
Copy Markdown
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
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
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