You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "yashmayya (via GitHub)" <gi...@apache.org> on 2024/08/05 13:44:04 UTC
[PR] Add broker API to run a query on both query engines and compare results [pinot]
yashmayya opened a new pull request, #13746:
URL: https://github.com/apache/pinot/pull/13746
- This patch adds a broker API to run a query on both the single stage and the multi stage query engines and compare the results. It complements the migration metric added in https://github.com/apache/pinot/pull/13628 in order to assist users that want to migrate from the v1 query engine to the v2 query engine.
- This tool is a more "active" way of checking queries than the "passive" metric that only checks whether the v1 queries being run are compilable in v2.
- Here, the query is actually run on both the query engines and the results are compared. Any differences such as a mismatch in the number of result rows, types of result columns etc. are explicitly called out to the user.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add broker API to run a query on both query engines and compare results [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #13746:
URL: https://github.com/apache/pinot/pull/13746#discussion_r1771564087
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -359,6 +359,8 @@ public static class Broker {
public static class Request {
public static final String SQL = "sql";
+ public static final String V1SQL = "v1sql";
Review Comment:
Thanks, I've changed it to `sqlV1` / `sqlV2`.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add broker API to run a query on both query engines and compare results [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #13746:
URL: https://github.com/apache/pinot/pull/13746#discussion_r1740884368
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -359,6 +359,8 @@ public static class Broker {
public static class Request {
public static final String SQL = "sql";
+ public static final String V1SQL = "v1sql";
Review Comment:
or use only `sse` and `mse` as the semantics of the fields are obvious and will be documented. `Sql` prefix is not required.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add broker API to run a query on both query engines and compare results [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13746:
URL: https://github.com/apache/pinot/pull/13746#discussion_r1740796684
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -359,6 +359,8 @@ public static class Broker {
public static class Request {
public static final String SQL = "sql";
+ public static final String V1SQL = "v1sql";
Review Comment:
Ideally we should always try to use single-stage query engine and multi-stage query engine instead of v1 and v2, but we are so used to the latest that sometimes we _leak_ these terms.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add broker API to run a query on both query engines and compare results [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #13746:
URL: https://github.com/apache/pinot/pull/13746#discussion_r1771565589
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -236,6 +243,78 @@ public void processSqlWithMultiStageQueryEnginePost(String query, @Suspended Asy
}
}
+ @POST
+ @Produces(MediaType.APPLICATION_JSON)
+ @Path("query/compare")
+ @ApiOperation(value = "Query Pinot using both the single-stage query engine and the multi-stage query engine and "
+ + "compare the results. The 'sql' field should be set in the request JSON to run the same query on both the "
+ + "query engines. Set '" + Request.V1SQL + "' and '" + Request.V2SQL + "' if the query needs to be adapted for "
+ + "the two query engines.")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Query result comparison response"),
+ @ApiResponse(code = 500, message = "Internal Server Error")
+ })
+ @ManualAuthorization
+ public void processSqlQueryWithBothEnginesAndCompareResults(String query, @Suspended AsyncResponse asyncResponse,
+ @Context org.glassfish.grizzly.http.server.Request requestContext,
+ @Context HttpHeaders httpHeaders) {
+ try {
+ JsonNode requestJson = JsonUtils.stringToJsonNode(query);
+ String v1Query;
+ String v2Query;
+ if (requestJson.has(Request.SQL)) {
+ v1Query = requestJson.get(Request.SQL).asText();
+ v2Query = v1Query;
+ } else if (requestJson.has(Request.V1SQL) && requestJson.has(Request.V2SQL)) {
+ v1Query = requestJson.get(Request.V1SQL).asText();
+ v2Query = requestJson.get(Request.V2SQL).asText();
+ } else {
+ throw new IllegalStateException("Payload should either contain the query string field '" + Request.SQL + "' "
+ + "or both of '" + Request.V1SQL + "' and '" + Request.V2SQL + "'");
+ }
Review Comment:
Makes sense, I've added this fallback although I haven't updated the API doc in order to avoid making it too confusing.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add broker API to run a query on both query engines and compare results [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13746:
URL: https://github.com/apache/pinot/pull/13746#discussion_r1740801249
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -379,4 +456,85 @@ static Response getPinotQueryResponse(BrokerResponse brokerResponse)
.entity((StreamingOutput) brokerResponse::toOutputStream).type(MediaType.APPLICATION_JSON)
.build();
}
+
+ @VisibleForTesting
+ static Response getPinotQueryComparisonResponse(String query, BrokerResponse v1Response, BrokerResponse v2Response) {
+ ObjectNode response = JsonUtils.newObjectNode();
+ response.set("v1Response", JsonUtils.objectToJsonNode(v1Response));
+ response.set("v2Response", JsonUtils.objectToJsonNode(v2Response));
+ response.set("comparisonAnalysis", JsonUtils.objectToJsonNode(analyzeDifferences(query, v1Response, v2Response)));
+
+ return Response.ok()
+ .header(PINOT_QUERY_ERROR_CODE_HEADER, -1)
+ .entity(response).type(MediaType.APPLICATION_JSON)
+ .build();
+ }
+
+ private static List<String> analyzeDifferences(String query, BrokerResponse v1Response, BrokerResponse v2Response) {
+ List<String> differences = new ArrayList<>();
+
+ if (v1Response.getExceptionsSize() != 0 || v2Response.getExceptionsSize() != 0) {
+ differences.add("Exception encountered while running the query on one or both query engines");
+ return differences;
+ }
+
+ if (v1Response.getResultTable() == null && v2Response.getResultTable() == null) {
+ return differences;
+ }
+
+ if (v1Response.getResultTable() == null) {
+ differences.add("v1 response has an empty result table");
+ return differences;
+ }
+
+ if (v2Response.getResultTable() == null) {
+ differences.add("v2 response has an empty result table");
+ return differences;
+ }
+
+ DataSchema.ColumnDataType[] v1ResponseTypes = v1Response.getResultTable().getDataSchema().getColumnDataTypes();
Review Comment:
AFAIR any simple select query without group by can return different results, even using the same engine. This should be more frequent if there are several segments involved in the query. In fact we would like to verify order if the query is order by and do not do that in the other case.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add broker API to run a query on both query engines and compare results [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #13746:
URL: https://github.com/apache/pinot/pull/13746#discussion_r1740349363
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -379,4 +456,85 @@ static Response getPinotQueryResponse(BrokerResponse brokerResponse)
.entity((StreamingOutput) brokerResponse::toOutputStream).type(MediaType.APPLICATION_JSON)
.build();
}
+
+ @VisibleForTesting
+ static Response getPinotQueryComparisonResponse(String query, BrokerResponse v1Response, BrokerResponse v2Response) {
+ ObjectNode response = JsonUtils.newObjectNode();
+ response.set("v1Response", JsonUtils.objectToJsonNode(v1Response));
+ response.set("v2Response", JsonUtils.objectToJsonNode(v2Response));
+ response.set("comparisonAnalysis", JsonUtils.objectToJsonNode(analyzeDifferences(query, v1Response, v2Response)));
+
+ return Response.ok()
+ .header(PINOT_QUERY_ERROR_CODE_HEADER, -1)
+ .entity(response).type(MediaType.APPLICATION_JSON)
+ .build();
+ }
+
+ private static List<String> analyzeDifferences(String query, BrokerResponse v1Response, BrokerResponse v2Response) {
+ List<String> differences = new ArrayList<>();
+
+ if (v1Response.getExceptionsSize() != 0 || v2Response.getExceptionsSize() != 0) {
+ differences.add("Exception encountered while running the query on one or both query engines");
+ return differences;
+ }
+
+ if (v1Response.getResultTable() == null && v2Response.getResultTable() == null) {
+ return differences;
+ }
+
+ if (v1Response.getResultTable() == null) {
+ differences.add("v1 response has an empty result table");
+ return differences;
+ }
+
+ if (v2Response.getResultTable() == null) {
+ differences.add("v2 response has an empty result table");
+ return differences;
+ }
+
+ DataSchema.ColumnDataType[] v1ResponseTypes = v1Response.getResultTable().getDataSchema().getColumnDataTypes();
Review Comment:
Do you want to recognise cases where column ordering is different? I dont know if there are cases where the engines may return the result in different column orders.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -379,4 +456,85 @@ static Response getPinotQueryResponse(BrokerResponse brokerResponse)
.entity((StreamingOutput) brokerResponse::toOutputStream).type(MediaType.APPLICATION_JSON)
.build();
}
+
+ @VisibleForTesting
+ static Response getPinotQueryComparisonResponse(String query, BrokerResponse v1Response, BrokerResponse v2Response) {
+ ObjectNode response = JsonUtils.newObjectNode();
+ response.set("v1Response", JsonUtils.objectToJsonNode(v1Response));
+ response.set("v2Response", JsonUtils.objectToJsonNode(v2Response));
+ response.set("comparisonAnalysis", JsonUtils.objectToJsonNode(analyzeDifferences(query, v1Response, v2Response)));
+
+ return Response.ok()
+ .header(PINOT_QUERY_ERROR_CODE_HEADER, -1)
+ .entity(response).type(MediaType.APPLICATION_JSON)
+ .build();
+ }
+
+ private static List<String> analyzeDifferences(String query, BrokerResponse v1Response, BrokerResponse v2Response) {
+ List<String> differences = new ArrayList<>();
+
+ if (v1Response.getExceptionsSize() != 0 || v2Response.getExceptionsSize() != 0) {
+ differences.add("Exception encountered while running the query on one or both query engines");
Review Comment:
nit: Add exception messages to help out the user ?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -379,4 +456,85 @@ static Response getPinotQueryResponse(BrokerResponse brokerResponse)
.entity((StreamingOutput) brokerResponse::toOutputStream).type(MediaType.APPLICATION_JSON)
.build();
}
+
+ @VisibleForTesting
+ static Response getPinotQueryComparisonResponse(String query, BrokerResponse v1Response, BrokerResponse v2Response) {
+ ObjectNode response = JsonUtils.newObjectNode();
+ response.set("v1Response", JsonUtils.objectToJsonNode(v1Response));
+ response.set("v2Response", JsonUtils.objectToJsonNode(v2Response));
+ response.set("comparisonAnalysis", JsonUtils.objectToJsonNode(analyzeDifferences(query, v1Response, v2Response)));
+
+ return Response.ok()
+ .header(PINOT_QUERY_ERROR_CODE_HEADER, -1)
+ .entity(response).type(MediaType.APPLICATION_JSON)
+ .build();
+ }
+
+ private static List<String> analyzeDifferences(String query, BrokerResponse v1Response, BrokerResponse v2Response) {
+ List<String> differences = new ArrayList<>();
+
+ if (v1Response.getExceptionsSize() != 0 || v2Response.getExceptionsSize() != 0) {
+ differences.add("Exception encountered while running the query on one or both query engines");
+ return differences;
+ }
+
+ if (v1Response.getResultTable() == null && v2Response.getResultTable() == null) {
+ return differences;
+ }
+
+ if (v1Response.getResultTable() == null) {
+ differences.add("v1 response has an empty result table");
+ return differences;
+ }
+
+ if (v2Response.getResultTable() == null) {
+ differences.add("v2 response has an empty result table");
+ return differences;
+ }
+
+ DataSchema.ColumnDataType[] v1ResponseTypes = v1Response.getResultTable().getDataSchema().getColumnDataTypes();
+ DataSchema.ColumnDataType[] v2ResponseTypes = v2Response.getResultTable().getDataSchema().getColumnDataTypes();
+
+ if (v1ResponseTypes.length != v2ResponseTypes.length) {
+ differences.add("Mismatch in number of columns returned. v1: " + v1ResponseTypes.length
+ + ", v2: " + v2ResponseTypes.length);
+ return differences;
+ }
+
+ String[] v1ColumnNames = v1Response.getResultTable().getDataSchema().getColumnNames();
+ String[] v2ColumnNames = v2Response.getResultTable().getDataSchema().getColumnNames();
+ for (int i = 0; i < v1ResponseTypes.length; i++) {
+ if (v1ResponseTypes[i] != v2ResponseTypes[i]) {
+ String columnName = v1ColumnNames[i].equals(v2ColumnNames[i])
+ ? v1ColumnNames[i]
+ : v1ColumnNames[i] + " / " + v2ColumnNames[i];
+ differences.add("Mismatch in column data type for column with name " + columnName
+ + ". v1 type: " + v1ResponseTypes[i] + ", v2 type: " + v2ResponseTypes[i]);
+ }
+ }
+
+ if (v1Response.getNumRowsResultSet() != v2Response.getNumRowsResultSet()) {
+ differences.add("Mismatch in number of rows returned. v1: " + v1Response.getNumRowsResultSet()
+ + ", v2: " + v2Response.getNumRowsResultSet());
+ return differences;
+ }
+
+ QueryContext queryContext = QueryContextConverterUtils.getQueryContext(query);
+ if (QueryContextUtils.isAggregationQuery(queryContext) && queryContext.getGroupByExpressions() == null) {
+ // Aggregation-only query with a single row output
+ for (int i = 0; i < v1ColumnNames.length; i++) {
+ if (!Objects.equals(v1Response.getResultTable().getRows().get(0)[i],
+ v2Response.getResultTable().getRows().get(0)[i])) {
+ differences.add("Mismatch in aggregation value for " + v1ColumnNames[i]
+ + ". v1 value: " + v1Response.getResultTable().getRows().get(0)[i]
+ + ", v2 value: " + v2Response.getResultTable().getRows().get(0)[i]);
+ }
+ }
+ }
+
+ // TODO: Compare response row values if it makes sense for the query type. Handle edge cases with group trimming,
Review Comment:
nit: document as a javadoc?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -359,6 +359,8 @@ public static class Broker {
public static class Request {
public static final String SQL = "sql";
+ public static final String V1SQL = "v1sql";
Review Comment:
Use official names ? `SSQE` and `MSQE`. Though I vote to drop the `Q` in the abbrevations.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add broker API to run a query on both query engines and compare results [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #13746:
URL: https://github.com/apache/pinot/pull/13746#issuecomment-2269231345
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/13746?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `0%` with `62 lines` in your changes missing coverage. Please review.
> Project coverage is 34.91%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`0d58ced`)](https://app.codecov.io/gh/apache/pinot/commit/0d58ced63e98b4a84970cd63ddafc727c010e5e6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 821 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/13746?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...pinot/broker/api/resources/PinotClientRequest.java](https://app.codecov.io/gh/apache/pinot/pull/13746?src=pr&el=tree&filepath=pinot-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fbroker%2Fapi%2Fresources%2FPinotClientRequest.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdENsaWVudFJlcXVlc3QuamF2YQ==) | 0.00% | [62 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13746?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (0d58ced). Click for more details.
>
> <details><summary>HEAD has 17 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (0d58ced) |
>|------|------|------|
>|temurin|12|9|
>|skip-bytebuffers-true|3|2|
>|skip-bytebuffers-false|7|5|
>|unittests|5|1|
>|unittests1|2|1|
>|java-11|5|2|
>|unittests2|3|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #13746 +/- ##
=============================================
- Coverage 61.75% 34.91% -26.84%
+ Complexity 207 6 -201
=============================================
Files 2436 2482 +46
Lines 133233 137434 +4201
Branches 20636 21389 +753
=============================================
- Hits 82274 47983 -34291
- Misses 44911 85799 +40888
+ Partials 6048 3652 -2396
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/13746/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/13746/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-0.01%)` | :arrow_down: |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/13746/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/13746/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-0.01%)` | :arrow_down: |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/13746/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/13746/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-61.71%)` | :arrow_down: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/13746/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.91% <0.00%> (-26.72%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/13746/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.91% <0.00%> (-26.84%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/13746/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-27.73%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/13746/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.91% <0.00%> (-26.84%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/13746/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.21% <ø> (-15.54%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/13746/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.21% <ø> (-0.68%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/13746/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/13746?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add broker API to run a query on both query engines and compare results [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #13746:
URL: https://github.com/apache/pinot/pull/13746#discussion_r1740893274
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -379,4 +456,85 @@ static Response getPinotQueryResponse(BrokerResponse brokerResponse)
.entity((StreamingOutput) brokerResponse::toOutputStream).type(MediaType.APPLICATION_JSON)
.build();
}
+
+ @VisibleForTesting
+ static Response getPinotQueryComparisonResponse(String query, BrokerResponse v1Response, BrokerResponse v2Response) {
+ ObjectNode response = JsonUtils.newObjectNode();
+ response.set("v1Response", JsonUtils.objectToJsonNode(v1Response));
+ response.set("v2Response", JsonUtils.objectToJsonNode(v2Response));
+ response.set("comparisonAnalysis", JsonUtils.objectToJsonNode(analyzeDifferences(query, v1Response, v2Response)));
+
+ return Response.ok()
+ .header(PINOT_QUERY_ERROR_CODE_HEADER, -1)
+ .entity(response).type(MediaType.APPLICATION_JSON)
+ .build();
+ }
+
+ private static List<String> analyzeDifferences(String query, BrokerResponse v1Response, BrokerResponse v2Response) {
+ List<String> differences = new ArrayList<>();
+
+ if (v1Response.getExceptionsSize() != 0 || v2Response.getExceptionsSize() != 0) {
+ differences.add("Exception encountered while running the query on one or both query engines");
+ return differences;
+ }
+
+ if (v1Response.getResultTable() == null && v2Response.getResultTable() == null) {
+ return differences;
+ }
+
+ if (v1Response.getResultTable() == null) {
+ differences.add("v1 response has an empty result table");
+ return differences;
+ }
+
+ if (v2Response.getResultTable() == null) {
+ differences.add("v2 response has an empty result table");
+ return differences;
+ }
+
+ DataSchema.ColumnDataType[] v1ResponseTypes = v1Response.getResultTable().getDataSchema().getColumnDataTypes();
Review Comment:
Column data type check will identify it. IME, if there is a query with 20 columns, there will be 20 messages in the `differences` array while the real problem is just that columns are ordered differently. Anyway - it is unclear if this issue exists and can be detected as an improvement in the future.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add broker API to run a query on both query engines and compare results [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13746:
URL: https://github.com/apache/pinot/pull/13746#discussion_r1771474421
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -359,6 +359,8 @@ public static class Broker {
public static class Request {
public static final String SQL = "sql";
+ public static final String V1SQL = "v1sql";
Review Comment:
My order of preference in descending order is:
1. `sqlV1` and `sqlV2` so they start with the same prefix of the default `sql`.
2. `v1Sql` and `v2Sql`
3. `sse` and `mse` sound a bit strange.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add broker API to run a query on both query engines and compare results [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #13746:
URL: https://github.com/apache/pinot/pull/13746#discussion_r1771460533
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -236,6 +243,76 @@ public void processSqlWithMultiStageQueryEnginePost(String query, @Suspended Asy
}
}
+ @POST
+ @Produces(MediaType.APPLICATION_JSON)
+ @Path("query/compare")
+ @ApiOperation(value = "Query Pinot using both the single stage query engine and the multi stage query engine and "
Review Comment:
Good point, I've gone with your option 2 since ideally we'd want most single-stage engine queries to work as is on the multi-stage query engine.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add broker API to run a query on both query engines and compare results [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #13746:
URL: https://github.com/apache/pinot/pull/13746#discussion_r1745157640
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -236,6 +243,76 @@ public void processSqlWithMultiStageQueryEnginePost(String query, @Suspended Asy
}
}
+ @POST
+ @Produces(MediaType.APPLICATION_JSON)
+ @Path("query/compare")
+ @ApiOperation(value = "Query Pinot using both the single stage query engine and the multi stage query engine and "
Review Comment:
I am using swagger APIs quite a bit today. Setting `sql` vs `v1Sql/v2Sql` maybe confusing. Majority of users will only use the doc in the swagger page to use the API. There are a couple of options to reduce confusion:
* Only provide `v1Sql`/`v2Sql`. Its not that hard to copy paste twice.
* Change the one-line documentation to add more info. There is precedence to multi-line description in swagger page. For example:
> Query Pinot using both the single stage query engine and the multi stage query engine and compare the results. Set sql field to run the same query in both engines. Set v1Sql & v2Sql instead if query text is different.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add broker API to run a query on both query engines and compare results [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #13746:
URL: https://github.com/apache/pinot/pull/13746#discussion_r1771460533
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -236,6 +243,76 @@ public void processSqlWithMultiStageQueryEnginePost(String query, @Suspended Asy
}
}
+ @POST
+ @Produces(MediaType.APPLICATION_JSON)
+ @Path("query/compare")
+ @ApiOperation(value = "Query Pinot using both the single stage query engine and the multi stage query engine and "
Review Comment:
Good point, I've gone with your option 2.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add broker API to run a query on both query engines and compare results [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13746:
URL: https://github.com/apache/pinot/pull/13746#discussion_r1771476794
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -236,6 +243,78 @@ public void processSqlWithMultiStageQueryEnginePost(String query, @Suspended Asy
}
}
+ @POST
+ @Produces(MediaType.APPLICATION_JSON)
+ @Path("query/compare")
+ @ApiOperation(value = "Query Pinot using both the single-stage query engine and the multi-stage query engine and "
+ + "compare the results. The 'sql' field should be set in the request JSON to run the same query on both the "
+ + "query engines. Set '" + Request.V1SQL + "' and '" + Request.V2SQL + "' if the query needs to be adapted for "
+ + "the two query engines.")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Query result comparison response"),
+ @ApiResponse(code = 500, message = "Internal Server Error")
+ })
+ @ManualAuthorization
+ public void processSqlQueryWithBothEnginesAndCompareResults(String query, @Suspended AsyncResponse asyncResponse,
+ @Context org.glassfish.grizzly.http.server.Request requestContext,
+ @Context HttpHeaders httpHeaders) {
+ try {
+ JsonNode requestJson = JsonUtils.stringToJsonNode(query);
+ String v1Query;
+ String v2Query;
+ if (requestJson.has(Request.SQL)) {
+ v1Query = requestJson.get(Request.SQL).asText();
+ v2Query = v1Query;
+ } else if (requestJson.has(Request.V1SQL) && requestJson.has(Request.V2SQL)) {
+ v1Query = requestJson.get(Request.V1SQL).asText();
+ v2Query = requestJson.get(Request.V2SQL).asText();
+ } else {
+ throw new IllegalStateException("Payload should either contain the query string field '" + Request.SQL + "' "
+ + "or both of '" + Request.V1SQL + "' and '" + Request.V2SQL + "'");
+ }
Review Comment:
Can we support `sql` by default? I mean to still support `sql` when either `sqlV1` or `sqlV2` are provided.
So the algorithm I suggest is:
```
v1Query = requestJson.has(Request.V1SQL) ? requestJson.get(Request.V1SQL) else requestJson.get(Request.SQL)
```
and same for `v2Query`
This is similar to what we do in resource based query tests where there is a `sql` field we use by default but if there is a `h2Sql` we use that one for h2.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add broker API to run a query on both query engines and compare results [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #13746:
URL: https://github.com/apache/pinot/pull/13746#discussion_r1740718124
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -359,6 +359,8 @@ public static class Broker {
public static class Request {
public static final String SQL = "sql";
+ public static final String V1SQL = "v1sql";
Review Comment:
The official docs do use the v1 and v2 terminology as well - https://docs.pinot.apache.org/reference/single-stage-engine, https://docs.pinot.apache.org/developers/advanced/v2-multi-stage-query-engine and it seems more succinct and clear to say `v1sql` than something like `ssqeSql`.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -379,4 +456,85 @@ static Response getPinotQueryResponse(BrokerResponse brokerResponse)
.entity((StreamingOutput) brokerResponse::toOutputStream).type(MediaType.APPLICATION_JSON)
.build();
}
+
+ @VisibleForTesting
+ static Response getPinotQueryComparisonResponse(String query, BrokerResponse v1Response, BrokerResponse v2Response) {
+ ObjectNode response = JsonUtils.newObjectNode();
+ response.set("v1Response", JsonUtils.objectToJsonNode(v1Response));
+ response.set("v2Response", JsonUtils.objectToJsonNode(v2Response));
+ response.set("comparisonAnalysis", JsonUtils.objectToJsonNode(analyzeDifferences(query, v1Response, v2Response)));
+
+ return Response.ok()
+ .header(PINOT_QUERY_ERROR_CODE_HEADER, -1)
+ .entity(response).type(MediaType.APPLICATION_JSON)
+ .build();
+ }
+
+ private static List<String> analyzeDifferences(String query, BrokerResponse v1Response, BrokerResponse v2Response) {
+ List<String> differences = new ArrayList<>();
+
+ if (v1Response.getExceptionsSize() != 0 || v2Response.getExceptionsSize() != 0) {
+ differences.add("Exception encountered while running the query on one or both query engines");
Review Comment:
The query response itself contains the entire v1 and v2 responses which will have the exceptions. It seems redundant to duplicate that here?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -379,4 +456,85 @@ static Response getPinotQueryResponse(BrokerResponse brokerResponse)
.entity((StreamingOutput) brokerResponse::toOutputStream).type(MediaType.APPLICATION_JSON)
.build();
}
+
+ @VisibleForTesting
+ static Response getPinotQueryComparisonResponse(String query, BrokerResponse v1Response, BrokerResponse v2Response) {
+ ObjectNode response = JsonUtils.newObjectNode();
+ response.set("v1Response", JsonUtils.objectToJsonNode(v1Response));
+ response.set("v2Response", JsonUtils.objectToJsonNode(v2Response));
+ response.set("comparisonAnalysis", JsonUtils.objectToJsonNode(analyzeDifferences(query, v1Response, v2Response)));
+
+ return Response.ok()
+ .header(PINOT_QUERY_ERROR_CODE_HEADER, -1)
+ .entity(response).type(MediaType.APPLICATION_JSON)
+ .build();
+ }
+
+ private static List<String> analyzeDifferences(String query, BrokerResponse v1Response, BrokerResponse v2Response) {
+ List<String> differences = new ArrayList<>();
+
+ if (v1Response.getExceptionsSize() != 0 || v2Response.getExceptionsSize() != 0) {
+ differences.add("Exception encountered while running the query on one or both query engines");
+ return differences;
+ }
+
+ if (v1Response.getResultTable() == null && v2Response.getResultTable() == null) {
+ return differences;
+ }
+
+ if (v1Response.getResultTable() == null) {
+ differences.add("v1 response has an empty result table");
+ return differences;
+ }
+
+ if (v2Response.getResultTable() == null) {
+ differences.add("v2 response has an empty result table");
+ return differences;
+ }
+
+ DataSchema.ColumnDataType[] v1ResponseTypes = v1Response.getResultTable().getDataSchema().getColumnDataTypes();
Review Comment:
I'm not aware of any such cases where the column ordering differs in the two engines. However, we do check for column data type mismatches.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org