Add broker API to run a query on both query engines and compare results #13746

yashmayya · 2024-08-05T13:44:02Z

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 Automatically detect whether a v1 query could have run on the v2 query engine #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.

codecov-commenter · 2024-08-05T14:35:14Z

Codecov Report

Attention: Patch coverage is 64.19753% with 29 lines in your changes missing coverage. Please review.

Project coverage is 64.83%. Comparing base (59551e4) to head (f40583d).
Report is 1081 commits behind head on master.

Files with missing lines	Patch %	Lines
...pinot/broker/api/resources/PinotClientRequest.java	64.19%	18 Missing and 11 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #13746      +/-   ##
============================================
+ Coverage     61.75%   64.83%   +3.08%     
- Complexity      207     1534    +1327     
============================================
  Files          2436     2579     +143     
  Lines        133233   141334    +8101     
  Branches      20636    21655    +1019     
============================================
+ Hits          82274    91635    +9361     
+ Misses        44911    42937    -1974     
- Partials       6048     6762     +714

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (+99.99%)`	⬆️
integration	`100.00% <ø> (+99.99%)`	⬆️
integration1	`100.00% <ø> (+99.99%)`	⬆️
integration2	`0.00% <ø> (ø)`
java-11	`64.78% <64.19%> (+3.07%)`	⬆️
java-21	`64.71% <64.19%> (+3.09%)`	⬆️
skip-bytebuffers-false	`64.80% <64.19%> (+3.06%)`	⬆️
skip-bytebuffers-true	`64.70% <64.19%> (+36.97%)`	⬆️
temurin	`64.83% <64.19%> (+3.08%)`	⬆️
unittests	`64.83% <64.19%> (+3.08%)`	⬆️
unittests1	`56.33% <ø> (+9.44%)`	⬆️
unittests2	`34.93% <64.19%> (+7.20%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vrajat

Will you be adding documentation in docs or as javadocs ? It will be useful to document the gaps or conditions the comparator does not check for. Caveats are useful as the pass/fail result doesnt guarantee that the queries are similar.

vrajat · 2024-09-02T05:33:36Z

pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java

+    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");


nit: Add exception messages to help out the user ?

The query response itself contains the entire v1 and v2 responses which will have the exceptions. It seems redundant to duplicate that here?

vrajat · 2024-09-02T05:40:37Z

pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java

+      return differences;
+    }
+
+    DataSchema.ColumnDataType[] v1ResponseTypes = v1Response.getResultTable().getDataSchema().getColumnDataTypes();


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.

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.

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.

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.

vrajat · 2024-09-02T05:40:56Z

pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java

+      }
+    }
+
+    // TODO: Compare response row values if it makes sense for the query type. Handle edge cases with group trimming,


nit: document as a javadoc?

vrajat · 2024-09-02T05:42:57Z

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";


Use official names ? SSQE and MSQE. Though I vote to drop the Q in the abbrevations.

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.

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.

or use only sse and mse as the semantics of the fields are obvious and will be documented. Sql prefix is not required.

My order of preference in descending order is:

sqlV1 and sqlV2 so they start with the same prefix of the default sql.

v1Sql and v2Sql

sse and mse sound a bit strange.

Thanks, I've changed it to sqlV1 / sqlV2.

yashmayya

Will you be adding documentation in docs or as javadocs ? It will be useful to document the gaps or conditions the comparator does not check for. Caveats are useful as the pass/fail result doesnt guarantee that the queries are similar.

Agreed, I plan to add those details to the official documentation at https://docs.pinot.apache.org/.

yashmayya · 2024-09-02T10:51:12Z

pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java

+    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");


The query response itself contains the entire v1 and v2 responses which will have the exceptions. It seems redundant to duplicate that here?

yashmayya · 2024-09-02T10:52:42Z

pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java

+      return differences;
+    }
+
+    DataSchema.ColumnDataType[] v1ResponseTypes = v1Response.getResultTable().getDataSchema().getColumnDataTypes();


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.

yashmayya · 2024-09-02T10:55:08Z

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";


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.

vrajat · 2024-09-05T09:48:28Z

pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java

+  @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 "


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.

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.

gortiz · 2024-09-23T13:46:00Z

pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java

+      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 + "'");
+      }


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.

Makes sense, I've added this fallback although I haven't updated the API doc in order to avoid making it too confusing.

yashmayya added multi-stage Related to the multi-stage query engine rest-api labels Aug 5, 2024

siddharthteotia requested a review from jasperjiaguo August 8, 2024 22:29

yashmayya force-pushed the v1-v2-results-comparator branch from 0d58ced to 485c9dd Compare August 27, 2024 09:34

yashmayya marked this pull request as ready for review August 27, 2024 11:55

yashmayya requested review from gortiz and Jackie-Jiang August 27, 2024 11:56

yashmayya force-pushed the v1-v2-results-comparator branch from 485c9dd to f00bfae Compare August 28, 2024 09:19

vrajat reviewed Sep 2, 2024

View reviewed changes

yashmayya commented Sep 2, 2024

View reviewed changes

vrajat reviewed Sep 5, 2024

View reviewed changes

yashmayya force-pushed the v1-v2-results-comparator branch from f00bfae to 8f21e2a Compare September 23, 2024 13:37

gortiz reviewed Sep 23, 2024

View reviewed changes

yashmayya force-pushed the v1-v2-results-comparator branch from df41491 to cf5bafa Compare September 23, 2024 15:48

yashmayya added 3 commits September 24, 2024 18:00

Add broker API to run a query on both query engines and compare results

3e96471

Add support for separate v1 and v2 SQL queries; add more tests

e286e95

Add more details to API description; move TODO comment to method Javadoc

cbf303a

yashmayya force-pushed the v1-v2-results-comparator branch from cf5bafa to 269f934 Compare September 24, 2024 12:31

Rename v1 / v2 sql query fields; allow fallback to 'sql' field

f40583d

yashmayya force-pushed the v1-v2-results-comparator branch from 269f934 to f40583d Compare September 24, 2024 16:18

vrajat approved these changes Sep 30, 2024

View reviewed changes

gortiz approved these changes Sep 30, 2024

View reviewed changes

gortiz merged commit 591f193 into apache:master Sep 30, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add broker API to run a query on both query engines and compare results #13746

Add broker API to run a query on both query engines and compare results #13746

yashmayya commented Aug 5, 2024

codecov-commenter commented Aug 5, 2024 •

edited

Loading

vrajat left a comment

vrajat Sep 2, 2024

yashmayya Sep 2, 2024

vrajat Sep 2, 2024

yashmayya Sep 2, 2024

gortiz Sep 2, 2024

vrajat Sep 2, 2024

vrajat Sep 2, 2024

vrajat Sep 2, 2024

yashmayya Sep 2, 2024

gortiz Sep 2, 2024

vrajat Sep 2, 2024

gortiz Sep 23, 2024

yashmayya Sep 23, 2024

yashmayya left a comment

yashmayya Sep 2, 2024

yashmayya Sep 2, 2024

yashmayya Sep 2, 2024

vrajat Sep 5, 2024

yashmayya Sep 23, 2024 •

edited

Loading

gortiz Sep 23, 2024

yashmayya Sep 23, 2024

Add broker API to run a query on both query engines and compare results #13746

Add broker API to run a query on both query engines and compare results #13746

Conversation

yashmayya commented Aug 5, 2024

codecov-commenter commented Aug 5, 2024 • edited Loading

Codecov Report

vrajat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yashmayya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yashmayya Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 5, 2024 •

edited

Loading

yashmayya Sep 23, 2024 •

edited

Loading