alamb commented on code in PR #40243:
URL: https://github.com/apache/arrow/pull/40243#discussion_r1517757475
##########
format/FlightSql.proto:
##########
@@ -1797,6 +1797,27 @@ message DoPutUpdateResult {
int64 record_count = 1;
}
+/* An *optional* response returned when `DoPut` is called with
`CommandPreparedStatementQuery`.
+ *
+ * *Note on legacy behavior*: previous versions of the protocol did not return
any result for
+ * this command, and that behavior should still be supported by clients. See
documentation
+ * of individual fields for more details on expected client behavior in this
case.
Review Comment:
Maybe we could phrase this in terms of what happens in the case where
clients or servers don't support this new message
For example:
"If servers send return a `DoPutPreparedStatementResult`, and the client
does not use the updated handle in subsequent requests, the subsequent requests
may error"
##########
format/FlightSql.proto:
##########
@@ -1797,6 +1797,27 @@ message DoPutUpdateResult {
int64 record_count = 1;
}
+/* An *optional* response returned when `DoPut` is called with
`CommandPreparedStatementQuery`.
+ *
+ * *Note on legacy behavior*: previous versions of the protocol did not return
any result for
+ * this command, and that behavior should still be supported by clients. See
documentation
+ * of individual fields for more details on expected client behavior in this
case.
+ */
+message DoPutPreparedStatementResult {
+ option (experimental) = true;
+
+ // Represents a (potentially updated) opaque handle for the prepared
statement on the server.
+ // Because the handle could potentially be updated, any previous handles for
this prepared
+ // statement should be considered invalid, and 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.
Review Comment:
```suggestion
// The updated handle allows implementing query parameters with stateless
services.
```
##########
docs/source/format/FlightSql.rst:
##########
@@ -141,6 +141,13 @@ the ``type`` should be ``ClosePreparedStatement``).
Execute a previously created prepared statement and get the results.
When used with DoPut: binds parameter values to the prepared statement.
+ 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
Review Comment:
> . 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.
##########
format/FlightSql.proto:
##########
@@ -1797,6 +1797,27 @@ message DoPutUpdateResult {
int64 record_count = 1;
}
+/* An *optional* response returned when `DoPut` is called with
`CommandPreparedStatementQuery`.
+ *
+ * *Note on legacy behavior*: previous versions of the protocol did not return
any result for
+ * this command, and that behavior should still be supported by clients. See
documentation
+ * of individual fields for more details on expected client behavior in this
case.
+ */
+message DoPutPreparedStatementResult {
+ option (experimental) = true;
+
+ // Represents a (potentially updated) opaque handle for the prepared
statement on the server.
+ // Because the handle could potentially be updated, any previous handles for
this prepared
+ // statement should be considered invalid, and 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.
Review Comment:
I agree. I think we can remove the reference.
If we wanted we could copy some of the background on stateless services from
https://github.com/apache/arrow/issues/37720 to this comments here
##########
docs/source/format/FlightSql.rst:
##########
@@ -141,6 +141,13 @@ the ``type`` should be ``ClosePreparedStatement``).
Execute a previously created prepared statement and get the results.
When used with DoPut: binds parameter values to the prepared statement.
+ 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
+ for detecting the case where the client does not use the updated handle on
+ subsequent requests (older clients may ignore this field) and responding
+ appropriately.
Review Comment:
Yes, I think it means the server should return an error. Perhaps we can
reword like
> requests (older clients may ignore this field) and returning a clear error.
##########
format/FlightSql.proto:
##########
@@ -1797,6 +1797,27 @@ message DoPutUpdateResult {
int64 record_count = 1;
}
+/* An *optional* response returned when `DoPut` is called with
`CommandPreparedStatementQuery`.
+ *
+ * *Note on legacy behavior*: previous versions of the protocol did not return
any result for
+ * this command, and that behavior should still be supported by clients. See
documentation
+ * of individual fields for more details on expected client behavior in this
case.
+ */
+message DoPutPreparedStatementResult {
+ option (experimental) = true;
+
+ // Represents a (potentially updated) opaque handle for the prepared
statement on the server.
+ // Because the handle could potentially be updated, any previous handles for
this prepared
+ // statement should be considered invalid, and all subsequent requests for
this prepared
+ // statement must use this new handle, if specified.
Review Comment:
I think we are trying to point out that it is valid for a server (for
example, all currently existing servers) to not return a
`DoPutPreparedStatementResult`. Perhaps that concept is already captured in the
"An *optional* response returned when" statement above and we should remove the
phrase here
```suggestion
// statement must use this new handle.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
