Allow Presto clients to receive query results in binary format #20932

mbasmanova · 2023-09-21T11:16:46Z

Introduce 'binaryResults=true' query parameter to request query results returned in binary format https://prestodb.io/docs/current/develop/serialized-page.html

With binaryResults=true, the result json will contain 'binaryData' field of type array with one or
more pages of the results. The 'data' field will not be included in the result JSON.

See #20886

== NO RELEASE NOTE ==

tdcmeehan · 2023-09-21T16:03:20Z

Please consider adding either a query or path parameter to toggle the client results, as clients may set arbitrary headers, and without migration of clients to support this format, it will cause them to break unexpectedly (the data field will be null, which will indicate query completion).

mbasmanova · 2023-09-21T16:15:21Z

Please consider adding either a query or path parameter to toggle the client results, as clients may set arbitrary headers, and without migration of clients to support this format, it will cause them to break unexpectedly (the data field will be null, which will indicate query completion).

@tdcmeehan Tim, thank you for reviewing. I updated to use query parameter binaryResults=true instead of the header. Would you, please, take another look?

tdcmeehan

In general, whereas data is Object because we don't know the underlying types (primitives, lists, etc.), don't we know that binaryData is String since it's Base64 encoded?

tdcmeehan · 2023-09-21T16:19:13Z

presto-main/src/test/java/com/facebook/presto/server/TestServer.java

+                data.addAll(queryResults.getBinaryData());
+            }
+        }
+        assertNull(queryResults.getError());


Perhaps print out the error in case this test becomes flaky

tdcmeehan · 2023-09-21T16:22:11Z

presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java

@@ -442,25 +447,48 @@ private synchronized QueryResults getNextResult(long token, UriInfo uriInfo, Str
        // last page is removed.  If another thread observes this state before the response is cached
        // the pages will be lost.
        Iterable<List<Object>> data = null;
+        List<Object> binaryData = null;


Nit: don't we know that the parameter of the List is String?

mbasmanova · 2023-09-21T16:29:20Z

@tdcmeehan Good points, Tim. Updated. Please, take another look.

tdcmeehan

LGTM % a few more nits, feel free to merge after addressing!

tdcmeehan · 2023-09-21T17:09:22Z

presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java

-                if (serializedPage == null) {
-                    break;
+            if (binaryResults) {
+                binaryData = new ArrayList<>();


Nit: use immutable list (consistent with below branch)?

tdcmeehan · 2023-09-21T17:13:23Z

presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java

+                    DynamicSliceOutput sliceOutput = new DynamicSliceOutput(1000);
+                    PagesSerdeUtil.writeSerializedPage(sliceOutput, serializedPage);
+
+                    String encodedPage = Base64.getEncoder().encodeToString(sliceOutput.slice().byteArray());


Nit: Add a constant?

Suggested change

String encodedPage = Base64.getEncoder().encodeToString(sliceOutput.slice().byteArray());

String encodedPage = BASE64_ENCODER.encodeToString(sliceOutput.slice().byteArray());

tdcmeehan · 2023-09-21T17:14:01Z

presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java

+                    bytes += serializedPage.getSizeInBytes();
+
+                    DynamicSliceOutput sliceOutput = new DynamicSliceOutput(1000);
+                    PagesSerdeUtil.writeSerializedPage(sliceOutput, serializedPage);


Nit: static import?

Suggested change

PagesSerdeUtil.writeSerializedPage(sliceOutput, serializedPage);

writeSerializedPage(sliceOutput, serializedPage);

mbasmanova · 2023-09-21T17:30:57Z

@tdcmeehan Tim, thank you for the review and offline discussion. Addressed all comments and updated the PR.

github-actions · 2023-09-21T18:20:00Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff dd5b168...d218176.

Notify	File(s)
@steveburnett	presto-docs/src/main/sphinx/develop/client-protocol.rst

steveburnett

LGTM! (docs)

agrawaldevesh · 2023-09-23T06:00:25Z

presto-main/src/main/java/com/facebook/presto/server/protocol/QueuedStatementResource.java

                .scheme(getScheme(xForwardedProto, uriInfo))
-                .replacePath("/v1/statement/queued/")
+                .replacePath("/v1/statement/queued")


Just a Q : Why was the trailing slash removed ?

agrawaldevesh · 2023-09-23T06:01:36Z

presto-main/src/main/java/com/facebook/presto/server/protocol/Query.java

-                if (serializedPage == null) {
-                    break;
+            if (binaryResults) {
+                ImmutableList.Builder<String> pages = ImmutableList.builder();


Nit: How do you feel about merging these two of the loops ?

Also is it expensive to recreate DynamicSliceOutput(1000) each time in the loop ? No idea, so just asking :-)

agrawaldevesh · 2023-09-23T06:03:48Z

presto-main/src/test/java/com/facebook/presto/server/TestServer.java

+            }
+        }
+
+        if (queryResults.getError() != null) {


nit: Is there a Util class where we can create this helper method that is an improvement on assertNull(queryResults.getError()) that is used in a couple of places now (this whole check and fail descriptively thing) :-)

mbasmanova requested a review from a team as a code owner September 21, 2023 11:16

mbasmanova requested a review from presto-oss September 21, 2023 11:16

mbasmanova force-pushed the binary-results branch from 0bc1bed to 743288d Compare September 21, 2023 11:17

mbasmanova changed the title ~~Allow Presto Client to return results in binary format~~ Allow Presto clients to receive query results in binary format Sep 21, 2023

mbasmanova force-pushed the binary-results branch 3 times, most recently from 5bca17d to 9955f8c Compare September 21, 2023 14:36

mbasmanova force-pushed the binary-results branch from 9955f8c to 5ba0235 Compare September 21, 2023 16:13

mbasmanova force-pushed the binary-results branch from 5ba0235 to c022068 Compare September 21, 2023 16:23

tdcmeehan reviewed Sep 21, 2023

View reviewed changes

mbasmanova force-pushed the binary-results branch from c022068 to a1e7cfd Compare September 21, 2023 16:29

tdcmeehan approved these changes Sep 21, 2023

View reviewed changes

mbasmanova force-pushed the binary-results branch from a1e7cfd to c64d903 Compare September 21, 2023 17:30

mbasmanova force-pushed the binary-results branch 2 times, most recently from c03ecc5 to 9550d77 Compare September 21, 2023 18:05

Allow Presto clients to receive query results in binary format

d218176

mbasmanova force-pushed the binary-results branch from 9550d77 to d218176 Compare September 21, 2023 18:11

steveburnett approved these changes Sep 21, 2023

View reviewed changes

mbasmanova merged commit da04bac into prestodb:master Sep 22, 2023
55 checks passed

mbasmanova deleted the binary-results branch September 22, 2023 17:14

mbasmanova mentioned this pull request Sep 22, 2023

Allow clients to receive query results in PrestoPage format #20886

Closed

agrawaldevesh reviewed Sep 23, 2023

View reviewed changes

jagill mentioned this pull request Apr 24, 2024

binaryData: Supplied checksum does not match checksum calculated from serialized slice #22598

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Presto clients to receive query results in binary format #20932

Allow Presto clients to receive query results in binary format #20932

mbasmanova commented Sep 21, 2023 •

edited

tdcmeehan commented Sep 21, 2023

mbasmanova commented Sep 21, 2023

tdcmeehan left a comment

tdcmeehan Sep 21, 2023

tdcmeehan Sep 21, 2023

mbasmanova commented Sep 21, 2023

tdcmeehan left a comment

tdcmeehan Sep 21, 2023

tdcmeehan Sep 21, 2023

tdcmeehan Sep 21, 2023

mbasmanova commented Sep 21, 2023

github-actions bot commented Sep 21, 2023 •

edited

steveburnett left a comment

agrawaldevesh Sep 23, 2023

agrawaldevesh Sep 23, 2023

agrawaldevesh Sep 23, 2023

	String encodedPage = Base64.getEncoder().encodeToString(sliceOutput.slice().byteArray());
	String encodedPage = BASE64_ENCODER.encodeToString(sliceOutput.slice().byteArray());

	PagesSerdeUtil.writeSerializedPage(sliceOutput, serializedPage);
	writeSerializedPage(sliceOutput, serializedPage);

Allow Presto clients to receive query results in binary format #20932

Allow Presto clients to receive query results in binary format #20932

Conversation

mbasmanova commented Sep 21, 2023 • edited

tdcmeehan commented Sep 21, 2023

mbasmanova commented Sep 21, 2023

tdcmeehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbasmanova commented Sep 21, 2023

tdcmeehan 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

mbasmanova commented Sep 21, 2023

github-actions bot commented Sep 21, 2023 • edited

steveburnett 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

mbasmanova commented Sep 21, 2023 •

edited

github-actions bot commented Sep 21, 2023 •

edited