Adding Interactive API's for MSQ engine by cryptoe · Pull Request #14416 · apache/druid

cryptoe · 2023-06-13T14:57:00Z

The current SQL endpoint is more suited toward hot queries where the client blocks for the results. The current task endpoint is more of a fire-and-forget API, primarily meant for ingestion.

This PR aims to expose a new API called
"@path("/druid/v2/sql/statements/")" which takes the same payload as the current "/druid/v2/sql" endpoint and allows users to fetch results in an async manner.

The statement execution API runs a single SQL statement which can be:

SELECT

DDL

CREATE [TEMPORARY|EXTERNAL] TABLE AS 

INSERT

POST SQL statement

POST /sql/statements

Executes a SQL statement.

Request: The post payload is in the same format as https://druid.apache.org/docs/latest/querying/sql.html#http-post
so that users can easily switch to the new async apis.
Two additional context field:


   mode: Determines how the query results are fetched. It will have 3 values [SYNC, ASYNC, AUTO] .  For the MVP mode=ASYNC is only supported. 

   execTimeout: The maximum amount of time that the query can execute. Handy to prevent “runaway queries.” Must be lower than any system-configured value. This is distinct from the results expiration timeout. Optional.(not supported for initial version)

Response

#http 200
{
  "state":"RUNNING",
  "queryID":"queryIDXX",
  "createdAt":"2023-03-07T12:58:17Z",
  "schema": [
    {
      "name": "col1",
      "type": "varchar"
    },
    {
      "name": "col2",
      "type": "varchar"
    }
  ]
}

#http 200
{
  "state": "FINISHED",
  "createdAt": "2023-03-07T12:58:17Z",
  "queryID": "queryIDXX",
  "durationInMS": 20,
  "schema": [
    {
      "name": "col1",
      "type": "varchar"
    },
    {
      "name": "col2",
      "type": "varchar"
    }
  ],
  "result": {
    "totalRows": 12412,
    "totalSize": 12000,
    "format":"json",
    "header":true,
    "sampleRecords": []
  }
}

Query execution failures

This sections is exactly similar as https://druid.apache.org/docs/latest/querying/querying.html#query-execution-failures
If a query fails, Druid returns a response with an HTTP response code and a JSON object with the following structure:

{
  "error" : "Query timeout",
  "errorMessage" : "Timeout waiting for task.",
  "errorClass" : "java.util.concurrent.TimeoutException",
  "host" : "druid1.example.com:8083"
}

Status

GET /sql/statements/{id}

Returns the same response as the post API.

Results

GET /sql/statements/{id}/results?offset=x&numRows=y&size=s&timeout=z

Returns a page of results if they are available.

    id Query Id.
    offset. Row offset of the first row to fetch.
    numRows: number of rows to fetch
    size: Size limit of the results, in bytes. (Optional: size limit in rows. The smallest value wins.) (not part of initial implementation)
    timeout: The timeout, in MS, to wait for results.

Close Query

DELETE /sql/statements/{id}

Cancels a running/accepted query.

TODO

Add more docs both user facing and javadocs
UT's

This PR has:

abhishekagarwal87 · 2023-06-19T12:04:04Z

+
+      TaskStatusResponse taskResponse = overlordClient.getTaskStatus(queryId);
+      if (taskResponse == null) {
+        return Response.status(Response.Status.NOT_FOUND).build();


It will be useful to add a message that includes queryId in it.

abhishekagarwal87 · 2023-06-19T12:04:36Z

+
+      TaskStatusPlus statusPlus = taskResponse.getStatus();
+      if (statusPlus == null || !MSQControllerTask.TYPE.equals(statusPlus.getType())) {
+        return Response.status(Response.Status.NOT_FOUND).build();


it will be useful to add an error message along with response code.

abhishekagarwal87 · 2023-06-19T12:07:01Z

+        );
+      } else if (sqlStatementState == SqlStatementState.FAILED) {
+        return buildNonOkResponse(
+            Response.Status.NOT_FOUND.getStatusCode(),


it shouldn't be a 404 IMO. the status code should come from statusPlus itself.

abhishekagarwal87 · 2023-06-19T12:08:08Z

+            new QueryException(null, statusPlus.getErrorMsg(), null, null),
+            queryId
+        );
+      } else {


you can avoid this nested code by using following structure
if (abc)
return xyz
if (abcd)
return something_else
code for default

abhishekagarwal87 · 2023-06-19T12:14:29Z

+    TaskStatusPlus statusPlus = taskResponse.getStatus();
+    if (statusPlus == null || !MSQControllerTask.TYPE.equals(statusPlus.getType())) {
+      return Optional.empty();
+    }


is this expected? If not, can we log some warning?

I think we should not add a warning since its bad user input.

Adding more sanity post test

2. Adding rows and size to response. 3. Adding dataSource to response. 4. Adding exceptionDetails to response.

# Conflicts: # sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java

abhishekagarwal87 · 2023-06-23T07:38:57Z

+  @GET
+  @Path("/enabled")
+  @Produces(MediaType.APPLICATION_JSON)
+  public Response doGetEnabled(@Context final HttpServletRequest request)


this context will be helpful in the javadocs.

abhishekagarwal87 · 2023-06-23T07:40:53Z

+    if (numRows != 1) {
+      throw new RE("Expected a single row but got [%d] rows. Please check broker logs for more information.", numRows);
+    }
+    String taskId = (String) rows.get(0)[0];


please add a size check on rows.get(0)

abhishekagarwal87 · 2023-06-23T07:42:21Z

+      for (Map.Entry<String, Object> worker : counterMap.entrySet()) {
+        Object workerChannels = worker.getValue();
+        if (workerChannels == null || !(workerChannels instanceof Map)) {
+          return Optional.empty();


do you ever expect this though?

No. Just defensive coding.

abhishekagarwal87 · 2023-06-23T09:54:58Z

+      return FutureUtils.getUnchecked(future, true);
+    }
+    catch (RuntimeException e) {
+      throw new QueryException(null, "Unable to contact overlord " + e.getMessage(), null, null, null);


whats the expected action associated with this error message?

This can happen for n reasons. Changed it to a developer exception.

DruidException.forPersona(DruidException.Persona.DEVELOPER) .ofCategory(DruidException.Category.UNCATEGORIZED) .build("Unable to contact overlord " + e.getMessage()); }

abhishekagarwal87 · 2023-06-23T09:59:49Z

+        MSQControllerTask msqControllerTask = getMSQControllerTaskOrThrow(queryId, authenticationResult.getIdentity());
+        Optional<List<ColNameAndType>> signature = getSignature(msqControllerTask);
+        if (!signature.isPresent()) {
+          return Response.ok().build();


hmm. what does it mean for signature to be absent?

Signature should not be empty but that's for the MSQ engine to decide.
We can't return results if the signature is empty. So if the results call get's an empty signature, we just return an empty result.

abhishekagarwal87 · 2023-06-23T10:01:57Z

+        if (!signature.isPresent()) {
+          return Response.ok().build();
+        }
+        Optional<List<Object>> results = getResults(getPayload(overlordWork(overlordClient.taskReportAsMap(queryId))));


so the results are in the task report?

2. Moving to DruidExceptions

adarshsanjeev

Partially reviewing this, still going through a few classes. Looks good to me overall. Had a few nits.

adarshsanjeev · 2023-06-27T12:28:21Z

  }

-  private byte[] responseToByteArray(Response resp) throws IOException
+  public static byte[] responseToByteArray(Response resp) throws IOException


Minor nit: Could be pushed up to CalciteTestBase so that other classes can use it in the future

I generally avoid pushing stuff to calcite test base since its already a big class until necessary. We can probably do that if this method is being used in other places in the future.

adarshsanjeev · 2023-06-27T14:53:17Z


  ListenableFuture<Map<String, Object>> taskReportAsMap(String taskId);

+  ListenableFuture<TaskPayloadResponse> taskPayload(String taskId);


nit: This function might need a better name to avoid confusing it with the task payload containing results, since we have a payload() function in SqlStatementResource already. Maybe taskDefinition?

This is borrowed for the existing indexingServiceClient hence I did not want to change the name.

Code coverage things. Review comments.

adarshsanjeev · 2023-06-28T08:19:42Z

+      final long last = SqlStatementResourceHelper.getLastIndex(numberOfRows, start);
+
+
+      getStatementStatus(queryId, authenticationResult.getIdentity(), false);


Is this call required? It seems to be checked after this already.

Oh yes that needs to be removed. Thanks for the catch.

adarshsanjeev

Looks good to me overall. I don't see any blocking issues here.

cryptoe · 2023-06-28T11:57:03Z

+  {
+
+    try {
+      Access authResult = AuthorizationUtils.authorizeAllResourceActions(


@abhishekagarwal87 I am following the pattern from

druid/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/SqlTaskResource.java

Line 110 in baa64e6

final Access authResult = AuthorizationUtils.authorizeAllResourceActions(

.

We might need to add a new method to AuthorizationUtilsto set the req header but that can be done as part of a follow up PR.

@abhishekagarwal87 Raised a PR : #14878

cryptoe · 2023-06-28T12:23:25Z

Thanks @abhishekagarwal87 @adarshsanjeev for the reviews. User Facing docs would be coming soon.

…14527) One of the most requested features in druid is to have an ability to download big result sets. As part of #14416 , we added an ability for MSQ to be queried via a query friendly endpoint. This PR builds upon that work and adds the ability for MSQ to write select results to durable storage. We write the results to the durable storage location <prefix>/results/<queryId> in the druid frame format. This is exposed to users by /v2/sql/statements/:queryId/results.

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>

This PR aims to expose a new API called "@path("/druid/v2/sql/statements/")" which takes the same payload as the current "/druid/v2/sql" endpoint and allows users to fetch results in an async manner.

…pache#14527) One of the most requested features in druid is to have an ability to download big result sets. As part of apache#14416 , we added an ability for MSQ to be queried via a query friendly endpoint. This PR builds upon that work and adds the ability for MSQ to write select results to durable storage. We write the results to the durable storage location <prefix>/results/<queryId> in the druid frame format. This is exposed to users by /v2/sql/statements/:queryId/results.

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

Adding Interactive API's for MSQ engine

4729279

cryptoe mentioned this pull request Jun 13, 2023

Community roadmap 2023 #14157

Open

cryptoe marked this pull request as draft June 13, 2023 15:01

github-advanced-security AI found potential problems Jun 13, 2023

View reviewed changes

vogievetsky added the Needs web console change Backend API changes that would benefit from frontend support in the web console label Jun 14, 2023

Adding more tests.

9371ebf

cryptoe changed the title ~~[Draft] Adding Interactive API's for MSQ engine~~ Adding Interactive API's for MSQ engine Jun 19, 2023

cryptoe marked this pull request as ready for review June 19, 2023 06:24

abhishekagarwal87 reviewed Jun 19, 2023

View reviewed changes

cryptoe added 5 commits June 20, 2023 08:26

Moving towards the new overlord client.

ed59f6d

Adding more sanity post test

1. Adding native types to response.

e37ca02

2. Adding rows and size to response. 3. Adding dataSource to response. 4. Adding exceptionDetails to response.

Fixing checkstyle

8006f6d

Merge remote-tracking branch 'apache/master' into async_api

e3b7966

# Conflicts: # sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java

Merge with main branch.

4bdd18a

github-advanced-security AI found potential problems Jun 22, 2023

View reviewed changes

abhishekagarwal87 reviewed Jun 23, 2023

View reviewed changes

cryptoe added 3 commits June 24, 2023 06:44

Cleaning up phase 1.

c6d3df3

Merge things

3cb11a9

1. Adding resource helper

7634124

2. Moving to DruidExceptions

github-actions Bot added the Area - Documentation label Jun 26, 2023

Merge things

06a7fb4

github-actions Bot removed the Area - Documentation label Jun 26, 2023

Changing SqlStatementResult to use ErrorResponse

b423b38

github-advanced-security AI found potential problems Jun 27, 2023

View reviewed changes

Comment thread ...core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/entity/SqlStatementResult.java Fixed

adarshsanjeev reviewed Jun 27, 2023

View reviewed changes

cryptoe added 4 commits June 27, 2023 23:49

Removing exception details from QueryException

5ab7771

Adding more tests for forbidden exception.

8055386

Code coverage things. Review comments.

Spotbugs.

164337d

Error Message test case fix.

56ceec1

adarshsanjeev reviewed Jun 28, 2023

View reviewed changes

Removing extra status call.

15778a8

adarshsanjeev approved these changes Jun 28, 2023

View reviewed changes

cryptoe commented Jun 28, 2023

View reviewed changes

abhishekagarwal87 approved these changes Jun 28, 2023

View reviewed changes

cryptoe added the Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 label Jun 28, 2023

cryptoe merged commit cb3a9d2 into apache:master Jun 28, 2023

cryptoe mentioned this pull request Jul 5, 2023

Adding Ability for MSQ to write select results to durable storage. #14527

Merged

10 tasks

vogievetsky mentioned this pull request Jul 7, 2023

Web console: catchup to all the backend changes #14540

Merged

AmatyaAvadhanula mentioned this pull request Jul 17, 2023

(Backport #14540) Web console: catchup to all the backend changes #14596

Merged

abhishekagarwal87 added this to the 27.0 milestone Jul 19, 2023

AmatyaAvadhanula mentioned this pull request Aug 6, 2023

[DRAFT] 27.0.0 release notes #14761

Closed

vogievetsky removed the Needs web console change Backend API changes that would benefit from frontend support in the web console label Aug 8, 2023

cryptoe mentioned this pull request Aug 20, 2023

Adjusting SqlStatementResource and SqlTaskResource to set request attribute via a new method. #14878

Merged


		ListenableFuture<Map<String, Object>> taskReportAsMap(String taskId);

		ListenableFuture<TaskPayloadResponse> taskPayload(String taskId);

		final long last = SqlStatementResourceHelper.getLastIndex(numberOfRows, start);


		getStatementStatus(queryId, authenticationResult.getIdentity(), false);

Conversation

cryptoe commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

POST SQL statement

Query execution failures

Status

Results

Close Query

TODO

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cryptoe Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adarshsanjeev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cryptoe Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adarshsanjeev left a comment

Choose a reason for hiding this comment

cryptoe commented Jun 13, 2023 •

edited

Loading

cryptoe Jun 28, 2023 •

edited

Loading

cryptoe Jun 28, 2023 •

edited

Loading

cryptoe Aug 20, 2023 •

edited

Loading