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

erratic-pattern · 2024-02-26T16:15:08Z

documents changes for stateless management of FlightSQL prepared statement handles based on the design proposal described in #37720

GitHub Issue: [FlightSQL] Stateless prepared statement with parameter support #37720

PRs for language implementations:

Rust: Implement FlightSQL spec change to support stateless prepared statements arrow-rs#5433
Go: GH-37720: [Go][FlightSQL] Add prepared statement handle to DoPut result #40311

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

github-actions · 2024-02-26T16:15:31Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

…s prepared statements

github-actions · 2024-03-01T19:33:29Z

⚠️ GitHub issue #37720 has been automatically assigned in GitHub to PR creator.

lidavidm

Do we need to discuss backwards compatibility at all? (Presumably it's on the server to detect that the client didn't use the updated handle?)

lidavidm · 2024-03-02T18:01:10Z

format/FlightSql.proto

+  option (experimental) = true;
+
+  // (potentially updated) opaque handle for the prepared statement on the server.
+  // All subsequent requests for his prepared statement must use this new handle, if specified


Suggested change

// All subsequent requests for his prepared statement must use this new handle, if specified

// All subsequent requests for this prepared statement must use this new handle, if specified

Do we need to discuss backwards compatibility at all? (Presumably it's on the server to detect that the client didn't use the updated handle?)

The only backwards compatibility safeguard in the current proposal is that the client must assume the returned handle could be missing if the server is following the previous spec. I am open to other suggestions.

What should happen here depends on whether the server is stateful or stateless.

Stateful: This should be inherently backwards and forwards compatible by design. There is no need for an updated handle here, because the server is already managing the necessary state. The server can simply execute the statement with the original handle, or any updated handle, since it should have all the necessary state for the prepared statement.

Stateless: There is no way to properly support prepared statements if the client does not return the updated handle. There is no backwards compatibility here, and so the only thing to do here is to fail gracefully

It may be worthwhile to document this somewhere, but I'm not sure the best place for it.

It would be nice if there could be a way for the server to know whether or not a client supports the updated protocol, as that would provide a more straightforward path for servers to provide graceful failure. But I'm not sure of the best way to do that. There may be implementation-specific tricks involving checking a log or a timestamp embedded in the handle to determine if a handle was updated but I think that's out of the scope of the spec

In general, I think we should have a larger discussion about how to communicate / handle compatibility in the FlightSQL protocol -- I filed #40424 to discuss the over arching issue, so hopefully we can focus discussions on this ticket to this particular proposal

Co-authored-by: David Li <li.davidm96@gmail.com>

alamb

Thank you very much @erratic-pattern - this looks great

Do we need to discuss backwards compatibility at all? (Presumably it's on the server to detect that the client didn't use the updated handle?)

Thank you @lidavidm that is a great idea. I left some potential text for this

This also made me wonder if we do (or should) version the FlightSQL protocol (so for example, we could describe "this client supports version 1.2" or something)

We should wait until a formal vote is held on the mailing list to merge this PR but from my perspective this PR is good to go

docs/source/format/FlightSql.rst

alamb · 2024-03-03T08:03:05Z

format/FlightSql.proto

+  option (experimental) = true;
+
+  // (potentially updated) opaque handle for the prepared statement on the server.
+  // All subsequent requests for this prepared statement must use this new handle, if specified


I think it might help to give a hint here about what this field is used for

Suggested change

// All subsequent requests for this prepared statement must use this new handle, if specified

// All subsequent requests for this prepared statement must use this new handle, if specified.

// The updated handle allows implementing query parameters with stateless services

// as described in https://github.com/apache/arrow/issues/37720

It would be good to copy/summarize the justification here, if we think it's worth keeping, instead of/along with referencing the ticket

It would be good to copy/summarize the justification here, if we think it's worth keeping, instead of/along with referencing the ticket

I've updated the docs to provide some background on this change, as well as explicitly documenting legacy behavior. also made the updated handle explicitly optional to help indicate that the older stateful pattern should still be supported even in up-to-date clients and servers.

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

pitrou · 2024-03-06T11:20:51Z

docs/source/format/FlightSql.rst

+    The server may optionally respond with an updated handle. The client
+    should use this updated handle for all subsequent requests for this
+    prepared statement. The updated handle allows implementing query 
+    parameters with stateless services. Note that the server is responsible 


This is very difficult to understand. How does an updated handle allow implementing query parameters? This should be more descriptive (does the handle embody the bound parameters?).

Also, "the client should use this updated handle" means the client cannot bind the original prepared statement to other parameters?

. How does an updated handle allow implementing query parameters? This should be more descriptive (does the handle embody the bound parameters?).

Maybe we can say something like this:

Optionally updating the handle allows the client to supply all state required to execute the query in an ActionPreparedStatementExecute message. For example, stateless servers can encode the bound parameter values into the new handle, and the client will send that new handle with parameters back to the server.

The problem is as currently written, the CommandPrepareStatementQuery message contains parameter values, but does not return anything to the client prior to the client sending ActionPreparedStatementExecute. Thus the only way to implement prepared statements with bind parameters today is to store the value of the parameters on the server between the call to CommandPrepareStatementQuery and ActionPreparedStatementExecute.

Also, "the client should use this updated handle" means the client cannot bind the original prepared statement to other parameters?

That is an excellent question. I think the intent is that the handle could be updated with new parameter values. I agree as worded this is confusing and should be clarified

Note that a handle returned from a DoPut call with CommandPreparedStatementQuery can itself be passed to a subsequent DoPut call with CommandPreparedStatementQuery to bind a new set of parameters. The subsequent call itself may return an updated handled which again should be used for subsequent requests.

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

alamb

With the updated changes this PR looks good to me

…ateless-prepared-statement-params-spec

docs/source/format/FlightSql.rst

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

erratic-pattern · 2024-03-24T17:09:46Z

This proposal was approved on the Arrow dev mailing list. See thread here

alamb · 2024-03-25T11:04:20Z

Thank you everyone for your feedback and participation in the process

conbench-apache-arrow · 2024-03-25T15:55:19Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 2b04275.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…lt (#40311) ### Rationale for this change See discussion on #37720 and mailing list: https://lists.apache.org/thread/3kb82ypx99q96g84qv555l6x8r0bppyq ### What changes are included in this PR? Changes the Go FlightSQL client and server implementations to support returning an updated prepared statement handle to the client as part of the `DoPut(PreparedStatement)` RPC call. ### Are these changes tested? ### Are there any user-facing changes? See parent issue and docs PR #40243 for details of user facing changes. **This PR includes breaking changes to public APIs.** * GitHub Issue: #37720 Lead-authored-by: Adam Curtis <adam.curtis.dev@gmail.com> Co-authored-by: David Li <li.davidm96@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>

…t result (apache#40311) ### Rationale for this change See discussion on apache#37720 and mailing list: https://lists.apache.org/thread/3kb82ypx99q96g84qv555l6x8r0bppyq ### What changes are included in this PR? Changes the Go FlightSQL client and server implementations to support returning an updated prepared statement handle to the client as part of the `DoPut(PreparedStatement)` RPC call. ### Are these changes tested? ### Are there any user-facing changes? See parent issue and docs PR apache#40243 for details of user facing changes. **This PR includes breaking changes to public APIs.** * GitHub Issue: apache#37720 Lead-authored-by: Adam Curtis <adam.curtis.dev@gmail.com> Co-authored-by: David Li <li.davidm96@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>

github-actions bot added Component: Documentation awaiting review Awaiting review labels Feb 26, 2024

erratic-pattern changed the title ~~feat(flightsql): update spec to support stateless prepared statements~~ doc(flightsql): document stateless prepared statements Feb 26, 2024

erratic-pattern force-pushed the adam/flightsql/stateless-prepared-statement-params-spec branch from 333b2cb to df571f1 Compare February 26, 2024 16:31

erratic-pattern mentioned this pull request Feb 28, 2024

[FlightSQL] Stateless prepared statement with parameter support #37720

Closed

erratic-pattern force-pushed the adam/flightsql/stateless-prepared-statement-params-spec branch from df571f1 to 40c3c21 Compare March 1, 2024 19:16

apacheGH-37720: [Format][Docs][FlightSQL] Document FlightSQL stateles…

3061d9d

…s prepared statements

erratic-pattern force-pushed the adam/flightsql/stateless-prepared-statement-params-spec branch from 40c3c21 to 3061d9d Compare March 1, 2024 19:22

erratic-pattern changed the title ~~doc(flightsql): document stateless prepared statements~~ GH-37720: [Go][FlightSQL] Implement stateless prepared statements Mar 1, 2024

erratic-pattern changed the title ~~GH-37720: [Go][FlightSQL] Implement stateless prepared statements~~ GH-37720: [Format][Docs][FlightSQL] Document FlightSQL stateless prepared statements Mar 1, 2024

erratic-pattern mentioned this pull request Mar 1, 2024

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

Merged

erratic-pattern changed the title ~~GH-37720: [Format][Docs][FlightSQL] Document FlightSQL stateless prepared statements~~ GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements Mar 1, 2024

lidavidm approved these changes Mar 2, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Mar 2, 2024

Fix typo in protobuf docs

cfce010

Co-authored-by: David Li <li.davidm96@gmail.com>

alamb approved these changes Mar 3, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Mar 4, 2024

Update docs/source/format/FlightSql.rst

dc96ef7

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

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 4, 2024

erratic-pattern and others added 2 commits March 4, 2024 09:55

Update format/FlightSql.proto

d4a30c1

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

clarify usage of the updated statement handle

0f4d1f4

erratic-pattern force-pushed the adam/flightsql/stateless-prepared-statement-params-spec branch from b52d0b4 to 0f4d1f4 Compare March 4, 2024 15:47

erratic-pattern mentioned this pull request Mar 4, 2024

Implement FlightSQL spec change to support stateless prepared statements apache/arrow-rs#5433

Merged

make comment style consistent in flightsql proto

bb2d155

pitrou reviewed Mar 6, 2024

View reviewed changes

github-actions bot added the awaiting change review Awaiting change review label Mar 16, 2024

erratic-pattern and others added 3 commits March 15, 2024 20:48

Update format/FlightSql.proto

e073001

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

add additional clarifications about usage of stateless parameters

d4dd713

clarify legacy behavior for DoPutPreparedResult protobuf message

8ddc3c3

alamb approved these changes Mar 18, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Mar 18, 2024

erratic-pattern force-pushed the adam/flightsql/stateless-prepared-statement-params-spec branch from 8ddc3c3 to 4894e23 Compare March 20, 2024 00:59

Merge branch 'main' of github.com:apache/arrow into adam/flightsql/st…

b5ad6c5

…ateless-prepared-statement-params-spec

erratic-pattern force-pushed the adam/flightsql/stateless-prepared-statement-params-spec branch from 4894e23 to b5ad6c5 Compare March 20, 2024 01:22

alamb approved these changes Mar 21, 2024

View reviewed changes

kou reviewed Mar 22, 2024

View reviewed changes

docs/source/format/FlightSql.rst Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Mar 22, 2024

Update docs/source/format/FlightSql.rst

6cffe2f

Co-authored-by: Sutou Kouhei <kou@cozmixng.org>

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 22, 2024

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

alamb merged commit 2b04275 into apache:main Mar 25, 2024
8 checks passed

lidavidm mentioned this pull request Mar 25, 2024

go/adbc/driver/flightsql: support stateless prepared statements apache/arrow-adbc#1657

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

erratic-pattern commented Feb 26, 2024 •

edited by alamb

Loading

github-actions bot commented Feb 26, 2024

github-actions bot commented Mar 1, 2024

lidavidm left a comment

lidavidm Mar 2, 2024

erratic-pattern Mar 2, 2024 •

edited

Loading

alamb Mar 8, 2024

alamb left a comment

alamb Mar 3, 2024

lidavidm Mar 4, 2024

erratic-pattern Mar 4, 2024 •

edited

Loading

pitrou Mar 6, 2024

alamb Mar 8, 2024

alamb left a comment

erratic-pattern commented Mar 24, 2024

alamb commented Mar 25, 2024

conbench-apache-arrow bot commented Mar 25, 2024

	// All subsequent requests for his prepared statement must use this new handle, if specified
	// All subsequent requests for this prepared statement must use this new handle, if specified

-  // All subsequent requests for this prepared statement must use this new handle, if specified
+  // All subsequent requests for this prepared statement must use this new handle, if specified.
+  // The updated handle allows implementing query parameters with stateless services
+  // as described in https://github.com/apache/arrow/issues/37720

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

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

Conversation

erratic-pattern commented Feb 26, 2024 • edited by alamb Loading

github-actions bot commented Feb 26, 2024

github-actions bot commented Mar 1, 2024

lidavidm left a comment

Choose a reason for hiding this comment

lidavidm Mar 2, 2024

Choose a reason for hiding this comment

erratic-pattern Mar 2, 2024 • edited Loading

Choose a reason for hiding this comment

alamb Mar 8, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Mar 3, 2024

Choose a reason for hiding this comment

lidavidm Mar 4, 2024

Choose a reason for hiding this comment

erratic-pattern Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

pitrou Mar 6, 2024

Choose a reason for hiding this comment

alamb Mar 8, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

erratic-pattern commented Mar 24, 2024

alamb commented Mar 25, 2024

conbench-apache-arrow bot commented Mar 25, 2024

erratic-pattern commented Feb 26, 2024 •

edited by alamb

Loading

erratic-pattern Mar 2, 2024 •

edited

Loading

erratic-pattern Mar 4, 2024 •

edited

Loading