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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
