cryptoe commented on code in PR #14512:
URL: https://github.com/apache/druid/pull/14512#discussion_r1249643153
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/entity/ResultSetInformation.java:
##########
@@ -31,51 +31,27 @@
public class ResultSetInformation
{
- @Nullable
- private final Long numRows;
- @Nullable
- private final Long sizeInBytes;
-
@Nullable
private final ResultFormat resultFormat;
-
@Nullable
private final List<Object> records;
-
@Nullable
private final String dataSource;
+ @Nullable
+ private final List<PageInformation> pageInformationList;
@JsonCreator
public ResultSetInformation(
@JsonProperty("resultFormat") @Nullable ResultFormat resultFormat,
- @JsonProperty("numRows") @Nullable Long numRows,
- @JsonProperty("sizeInBytes") @Nullable Long sizeInBytes,
@JsonProperty("dataSource") @Nullable String dataSource,
- @JsonProperty("sampleRecords") @Nullable
- List<Object> records
+ @JsonProperty("sampleRecords") @Nullable List<Object> records,
+ @JsonProperty("pageInformationList") @Nullable List<PageInformation>
pageInformationList
Review Comment:
Lets rename this to pages.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java:
##########
@@ -284,28 +285,25 @@ public Response doGetResults(
}
final AuthenticationResult authenticationResult =
AuthorizationUtils.authenticationResultFromRequest(req);
- if (offset != null && offset < 0) {
+ if (page == null) {
Review Comment:
Page==null means we need to fetch all the pages. So this validation needs to
be removed.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java:
##########
@@ -576,12 +587,16 @@ private Optional<ResultSetInformation> getSampleResults(
);
return Optional.of(new ResultSetInformation(
null,
- // since the rows can be sampled, get the number of rows from
counters
- rowsAndSize.orElse(new Pair<>(null, null)).lhs,
- rowsAndSize.orElse(new Pair<>(null, null)).rhs,
dataSource,
// only populate sample results in case a select query is successful
- isSelectQuery ?
SqlStatementResourceHelper.getResults(payload).orElse(null) : null
+ isSelectQuery ?
SqlStatementResourceHelper.getResults(payload).orElse(null) : null,
+ ImmutableList.of(
+ new PageInformation(
+ rowsAndSize.orElse(new Pair<>(null, null)).lhs,
Review Comment:
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.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/entity/ResultSetInformation.java:
##########
@@ -31,51 +31,27 @@
public class ResultSetInformation
{
- @Nullable
- private final Long numRows;
- @Nullable
- private final Long sizeInBytes;
-
@Nullable
private final ResultFormat resultFormat;
-
@Nullable
private final List<Object> records;
-
@Nullable
private final String dataSource;
+ @Nullable
+ private final List<PageInformation> pageInformationList;
@JsonCreator
public ResultSetInformation(
@JsonProperty("resultFormat") @Nullable ResultFormat resultFormat,
- @JsonProperty("numRows") @Nullable Long numRows,
Review Comment:
I think there is value in having the numRows and the sizeInBytes. We can
rename the variables to
numTotalRows , totalSizeInBytes
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java:
##########
@@ -284,28 +285,25 @@ public Response doGetResults(
}
final AuthenticationResult authenticationResult =
AuthorizationUtils.authenticationResultFromRequest(req);
- if (offset != null && offset < 0) {
+ if (page == null) {
return buildNonOkResponse(
DruidException.forPersona(DruidException.Persona.USER)
.ofCategory(DruidException.Category.INVALID_INPUT)
.build(
- "offset cannot be negative. Please pass a
positive number."
+ "Page cannot be null."
)
);
}
- if (numberOfRows != null && numberOfRows < 0) {
+ if (page < 0) {
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."
)
);
}
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
