-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Add page information to SqlStatementResource API #14512
Add page information to SqlStatementResource API #14512
Conversation
There was a problem hiding this 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
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." | ||
) | ||
); | ||
} | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
isSelectQuery ? SqlStatementResourceHelper.getResults(payload).orElse(null) : null, | ||
ImmutableList.of( | ||
new PageInformation( | ||
rowsAndSize.orElse(new Pair<>(null, null)).lhs, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
...ns-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/entity/PageInformation.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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
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
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
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>
* 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.
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
This PR makes a few changes to the SqlStatementResource
Payload:
This PR has: