Implement FlightSQL spec change to support stateless prepared statements #5433

erratic-pattern · 2024-02-26T16:10:22Z

implements client changes for stateless management of FlightSQL prepared statement handles based on the design proposal described in apache/arrow#37720

tustvold · 2024-02-29T02:36:24Z

Marking as draft as my understanding of the status of apache/arrow#37720 is that it is a protocol proposal that has yet to go through a mailing list approval process.

Feel free to unmark as a draft if I am mistaken here.

erratic-pattern · 2024-02-29T18:18:27Z

Marking as draft as my understanding of the status of apache/arrow#37720 is that it is a protocol proposal that has yet to go through a mailing list approval process.

Feel free to unmark as a draft if I am mistaken here.

That's fair. I just marked it as ready to indicate that it's ready for review, but it does still need to undergo the formal spec change process. These PRs are intended to serve as conversation starters around specific implementation details of the spec change.

tustvold · 2024-02-29T20:25:01Z

👍 I just wanted to avoid this accidentally getting merged early

alamb · 2024-03-03T08:08:24Z

Mailing list discussion thread: https://lists.apache.org/thread/3kb82ypx99q96g84qv555l6x8r0bppyq

alamb

Thank you @erratic-pattern -- I think this is looking quite good. I had some questions about the API design, but otherwise 👍

alamb · 2024-03-03T08:14:16Z

arrow-flight/src/sql/client.rs

+                .map_err(status_to_arrow_error)?
+                .unwrap();
+            // Attempt to update the stored handle with any updated handle in the DoPut result.
+            // Not all servers support this, so ignore any errors when attempting to decode.


I think this might silently ignore legitimate errors and I think it would be a good way to distinguish between a server not sending a response vs sending an invalid response.

If the server doesn't support sending back an updated handle, the put_results should be empty, right? So perhaps we could check for an empty response rather than ignoring the error?

I've updated the logic here to not ignore legitimate errors and instead only look for empty responses instead.

alamb · 2024-03-03T08:16:27Z

arrow-flight/examples/flight_sql_server.rs

@@ -617,7 +618,7 @@ impl FlightSqlService for FlightSqlServiceImpl {
        &self,
        _query: CommandPreparedStatementQuery,
        _request: Request<PeekableFlightDataStream>,
-    ) -> Result<Response<<Self as FlightService>::DoPutStream>, Status> {
+    ) -> Result<Option<DoPutPreparedStatementResult>, Status> {


Isn't it the field on DoPutPreparedStatementResult that is optional? It seems like this API would be cleaner and follow the rest of the API if it returned a DoPutPreparedStatementResult (and the client can handle / translate a missing message into a DoPutPreparedStatementResult)?

Suggested change

) -> Result<Option<DoPutPreparedStatementResult>, Status> {

) -> Result<DoPutPreparedStatementResult, Status> {

Or maybe it is important to distinguish between the case where the server didn't send back any message from the case where the server sent back a message with an empty prepared handle?

This is a decision of whether or not we want to expressly forbid up-to-date servers from implementing the older version of the spec, even though clients still support it. I went with the more conservative decision of allowing existing implementations to continue doing what they were doing before without any major changes aside from changing their return value to Ok(None). That way they continue with the legacy behavior without being "locked out" of future crate updates.

A counterargument to this would be that it should be trivial for servers to simply return the existing handle so there's no need for this extra layer of backwards compatibility.

arrow-flight/src/sql/server.rs

alamb · 2024-03-03T08:18:12Z

arrow-flight/tests/flight_sql_client_cli.rs

-            cmd.prepared_statement_handle,
-            PREPARED_STATEMENT_HANDLE.as_bytes()
-        );
+        if self.stateless_prepared_statements {


I'd like to do this kind of testing (simulate both stateful and stateless server behavior) in the Go implementation as well, but I am less confident around the Arrow Go test suite and general Go testing practices.

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

erratic-pattern · 2024-03-10T18:34:18Z

TODO: update protobuf docs with latest changes to apache/arrow#40243 once the documentation in that PR has finalized

erratic-pattern · 2024-03-24T17:10:37Z

The proposal for this specification change was approved on the Arrow dev mailing list. See thread here I've moved this PR out of draft status.

…ments (#40243) documents changes for stateless management of FlightSQL prepared statement handles based on the design proposal described in #37720 * GitHub Issue: #37720 PRs for language implementations: * Rust: apache/arrow-rs#5433 * Go: #40311 Mailing list discussion: https://lists.apache.org/thread/3kb82ypx99q96g84qv555l6x8r0bppyq --------- Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

alamb

Thanks again @erratic-pattern

Can you double check that the change to FlightSql.proto in this repo is the same as was merged into the arrow repo in apache/arrow#40243 ?

alamb · 2024-03-21T14:05:08Z

arrow-flight/src/sql/client.rs

@@ -519,17 +521,37 @@ impl PreparedStatement<Channel> {
                .await
                .map_err(flight_error_to_arrow_error)?;

-            self.flight_sql_client
+            // Attempt to update the stored handle with any updated handle in the DoPut result.
+            // Not all servers support this, so ignore any errors when attempting to decode.


The comment says errors are ignored, but the code doesn't seem to ignore errors. I wonder if I am misreading this or if the comment or code should be updated 🤔

The comment says errors are ignored, but the code doesn't seem to ignore errors. I wonder if I am misreading this or if the comment or code should be updated 🤔

comments never lie ;)

updated this to explain that we ignore the lack of a response from legacy servers, rather than any error.

format/FlightSql.proto

erratic-pattern · 2024-03-26T16:58:27Z

I've updated the protobuf docs to match the docs that were merged into arrow here apache/arrow#40243

Let me know if there's anything else left to address.

alamb

LGTM -- thank you very much @erratic-pattern

…nts (apache#5433) * feat: stateless FlightSQL prepared statements * update protobuf and improve legacy behavior * Update arrow-flight/src/sql/server.rs Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> * make DoPutPreparedStatenentResult mandatory * update DoPutPreparedStatementResult docs to match arrow repo * update comment about legacy server behavior in DoPut --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels Feb 26, 2024

erratic-pattern marked this pull request as ready for review February 28, 2024 20:47

erratic-pattern mentioned this pull request Feb 28, 2024

[FlightSQL] Stateless prepared statement with parameter support apache/arrow#37720

Closed

tustvold marked this pull request as draft February 29, 2024 02:36

erratic-pattern changed the title ~~feat: stateless FlightSQL prepared statements~~ Implement FlightSQL spec change to support stateless prepared statements Mar 1, 2024

erratic-pattern mentioned this pull request Mar 1, 2024

GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements apache/arrow#40243

Merged

alamb added the api-change Changes to the arrow API label Mar 3, 2024

alamb reviewed Mar 3, 2024

View reviewed changes

erratic-pattern mentioned this pull request Mar 4, 2024

GH-37720: [Go][FlightSQL] Add prepared statement handle to DoPut result apache/arrow#40311

Merged

erratic-pattern and others added 3 commits March 10, 2024 14:04

feat: stateless FlightSQL prepared statements

1d1b800

update protobuf and improve legacy behavior

1e6dfe0

Update arrow-flight/src/sql/server.rs

8ba176e

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

erratic-pattern force-pushed the adam/flightsql-stateless-prepared-statements branch from adf8361 to e8067f7 Compare March 10, 2024 18:28

make DoPutPreparedStatenentResult mandatory

3dcbeda

erratic-pattern force-pushed the adam/flightsql-stateless-prepared-statements branch from e8067f7 to 3dcbeda Compare March 10, 2024 18:28

This was referenced Mar 11, 2024

Add stateless prepared statements #5497

Closed

Add stateless prepared statements polygon-io/arrow-rs#1

Open

alamb marked this pull request as ready for review March 21, 2024 14:02

alamb marked this pull request as draft March 21, 2024 14:02

erratic-pattern marked this pull request as ready for review March 24, 2024 17:07

alamb approved these changes Mar 25, 2024

View reviewed changes

erratic-pattern commented Mar 26, 2024

View reviewed changes

format/FlightSql.proto Show resolved Hide resolved

erratic-pattern added 2 commits March 26, 2024 12:50

update DoPutPreparedStatementResult docs to match arrow repo

b738348

update comment about legacy server behavior in DoPut

041ea6d

alamb approved these changes Mar 26, 2024

View reviewed changes

alamb merged commit 9a5ea83 into apache:master Mar 26, 2024
12 checks passed

alamb mentioned this pull request Mar 26, 2024

[DISCUSS] Reducing cadence of major arrow-rs releases introducing patch releases #5368

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement FlightSQL spec change to support stateless prepared statements #5433

Implement FlightSQL spec change to support stateless prepared statements #5433

erratic-pattern commented Feb 26, 2024 •

edited

Loading

tustvold commented Feb 29, 2024

erratic-pattern commented Feb 29, 2024

tustvold commented Feb 29, 2024

alamb commented Mar 3, 2024

alamb left a comment

alamb Mar 3, 2024

erratic-pattern Mar 4, 2024

alamb Mar 3, 2024

erratic-pattern Mar 4, 2024 •

edited

Loading

alamb Mar 3, 2024

erratic-pattern Mar 4, 2024

erratic-pattern commented Mar 10, 2024

erratic-pattern commented Mar 24, 2024

alamb left a comment

alamb Mar 21, 2024

erratic-pattern Mar 26, 2024

erratic-pattern commented Mar 26, 2024

alamb left a comment

	) -> Result<Option<DoPutPreparedStatementResult>, Status> {
	) -> Result<DoPutPreparedStatementResult, Status> {

Implement FlightSQL spec change to support stateless prepared statements #5433

Implement FlightSQL spec change to support stateless prepared statements #5433

Conversation

erratic-pattern commented Feb 26, 2024 • edited Loading

tustvold commented Feb 29, 2024

erratic-pattern commented Feb 29, 2024

tustvold commented Feb 29, 2024

alamb commented Mar 3, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Mar 3, 2024

Choose a reason for hiding this comment

erratic-pattern Mar 4, 2024

Choose a reason for hiding this comment

alamb Mar 3, 2024

Choose a reason for hiding this comment

erratic-pattern Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

alamb Mar 3, 2024

Choose a reason for hiding this comment

erratic-pattern Mar 4, 2024

Choose a reason for hiding this comment

erratic-pattern commented Mar 10, 2024

erratic-pattern commented Mar 24, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Mar 21, 2024

Choose a reason for hiding this comment

erratic-pattern Mar 26, 2024

Choose a reason for hiding this comment

erratic-pattern commented Mar 26, 2024

alamb left a comment

Choose a reason for hiding this comment

erratic-pattern commented Feb 26, 2024 •

edited

Loading

erratic-pattern Mar 4, 2024 •

edited

Loading