You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "erratic-pattern (via GitHub)" <gi...@apache.org> on 2024/02/26 16:15:10 UTC
[PR] feat(flightsql): update spec to support stateless prepared statements [arrow]
erratic-pattern opened a new pull request, #40243:
URL: https://github.com/apache/arrow/pull/40243
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). 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](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) 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}
-->
### Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
### What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
### Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
### Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #40243:
URL: https://github.com/apache/arrow/pull/40243#discussion_r1510013638
##########
format/FlightSql.proto:
##########
@@ -1797,6 +1797,15 @@ message DoPutUpdateResult {
int64 record_count = 1;
}
+// An optional response returned when `DoPut` is called with `CommandPreparedStatementQuery`
+message DoPutPreparedStatementResult {
+ 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
Review Comment:
```suggestion
// All subsequent requests for this prepared statement must use this new handle, if specified
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "erratic-pattern (via GitHub)" <gi...@apache.org>.
erratic-pattern commented on code in PR #40243:
URL: https://github.com/apache/arrow/pull/40243#discussion_r1511382303
##########
format/FlightSql.proto:
##########
@@ -1797,6 +1797,15 @@ message DoPutUpdateResult {
int64 record_count = 1;
}
+// An optional response returned when `DoPut` is called with `CommandPreparedStatementQuery`
+message DoPutPreparedStatementResult {
+ 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
Review Comment:
> 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 can still be supported even in up-to-date clients and servers.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #40243:
URL: https://github.com/apache/arrow/pull/40243#discussion_r1514300710
##########
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:
Why "if specified"?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "erratic-pattern (via GitHub)" <gi...@apache.org>.
erratic-pattern commented on code in PR #40243:
URL: https://github.com/apache/arrow/pull/40243#discussion_r1527007991
##########
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
I wanted to avoid documenting information about individual fields in the message, since the message might one day contain many fields.
I think I could simplify the wording on this to just indicate that older servers dont use this, clients should support that, and just use the default blank values for fields in that case.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "erratic-pattern (via GitHub)" <gi...@apache.org>.
erratic-pattern commented on code in PR #40243:
URL: https://github.com/apache/arrow/pull/40243#discussion_r1510021392
##########
format/FlightSql.proto:
##########
@@ -1797,6 +1797,15 @@ message DoPutUpdateResult {
int64 record_count = 1;
}
+// An optional response returned when `DoPut` is called with `CommandPreparedStatementQuery`
+message DoPutPreparedStatementResult {
+ 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
Review Comment:
> 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
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #40243:
URL: https://github.com/apache/arrow/pull/40243#issuecomment-2018339062
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 2b0427559d94fb25ce61672c1ed17fc245d0a546.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/23063714848) has more details.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #40243:
URL: https://github.com/apache/arrow/pull/40243#issuecomment-2017741448
Thank you everyone for your feedback and participation in the process
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #40243:
URL: https://github.com/apache/arrow/pull/40243
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #40243:
URL: https://github.com/apache/arrow/pull/40243#discussion_r1514301137
##########
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:
What does "appropriately" imply? Return an error?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #40243:
URL: https://github.com/apache/arrow/pull/40243#issuecomment-1989350387
The "app" in this case is Flight SQL. (Someday I would like to fuse Flight SQL/Flight RPC to avoid these odd distinctions, and so that we aren't multiplexing a dozen different calls into the same endpoint...)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "erratic-pattern (via GitHub)" <gi...@apache.org>.
erratic-pattern commented on code in PR #40243:
URL: https://github.com/apache/arrow/pull/40243#discussion_r1510021392
##########
format/FlightSql.proto:
##########
@@ -1797,6 +1797,15 @@ message DoPutUpdateResult {
int64 record_count = 1;
}
+// An optional response returned when `DoPut` is called with `CommandPreparedStatementQuery`
+message DoPutPreparedStatementResult {
+ 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
Review Comment:
> 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 server's 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
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #40243:
URL: https://github.com/apache/arrow/pull/40243#discussion_r1514293835
##########
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:
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?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #40243:
URL: https://github.com/apache/arrow/pull/40243#discussion_r1511122588
##########
format/FlightSql.proto:
##########
@@ -1797,6 +1797,15 @@ message DoPutUpdateResult {
int64 record_count = 1;
}
+// An optional response returned when `DoPut` is called with `CommandPreparedStatementQuery`
+message DoPutPreparedStatementResult {
+ 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
Review Comment:
It would be good to copy/summarize the justification here, if we think it's worth keeping, instead of/along with referencing the ticket
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #40243:
URL: https://github.com/apache/arrow/pull/40243#discussion_r1510208251
##########
docs/source/format/FlightSql.rst:
##########
@@ -141,6 +141,9 @@ 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.
Review Comment:
```suggestion
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.
```
##########
format/FlightSql.proto:
##########
@@ -1797,6 +1797,15 @@ message DoPutUpdateResult {
int64 record_count = 1;
}
+// An optional response returned when `DoPut` is called with `CommandPreparedStatementQuery`
+message DoPutPreparedStatementResult {
+ 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
Review Comment:
I think it might help to give a hint here about what this field is used for
```suggestion
// 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
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "joellubi (via GitHub)" <gi...@apache.org>.
joellubi commented on PR #40243:
URL: https://github.com/apache/arrow/pull/40243#issuecomment-1989319426
Will we need to extend the schema of the `PutResult` message in order to encapsulate the `DoPutPreparedStatementResult` payload?
This is the spec right now:
```proto
service FlightService {
rpc DoPut(stream FlightData) returns (stream PutResult) {}
}
message PutResult {
bytes app_metadata = 1;
}
```
I don't think the response would be considered `app_metadata`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "erratic-pattern (via GitHub)" <gi...@apache.org>.
erratic-pattern commented on PR #40243:
URL: https://github.com/apache/arrow/pull/40243#issuecomment-2016874528
This proposal was approved on the Arrow dev mailing list. [See thread here](https://lists.apache.org/thread/zoyv6d4fzco4h48b66wqwmfc6w31757x)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(flightsql): update spec to support stateless prepared statements [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40243:
URL: https://github.com/apache/arrow/pull/40243#issuecomment-1964551459
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). 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](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) 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:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - Contributing Overview](https://arrow.apache.org/docs/developers/overview.html)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #40243:
URL: https://github.com/apache/arrow/pull/40243#discussion_r1514298945
##########
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:
We should not point to GH issue discussions as an explanation of why this exists. The spec should ideally be self-contained.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #40243:
URL: https://github.com/apache/arrow/pull/40243#discussion_r1517742005
##########
format/FlightSql.proto:
##########
@@ -1797,6 +1797,15 @@ message DoPutUpdateResult {
int64 record_count = 1;
}
+// An optional response returned when `DoPut` is called with `CommandPreparedStatementQuery`
+message DoPutPreparedStatementResult {
+ 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
Review Comment:
In general, I think we should have a larger discussion about how to communicate / handle compatibility in the FlightSQL protocol -- I filed https://github.com/apache/arrow/issues/40424 to discuss the over arching issue, so hopefully we can focus discussions on this ticket to this particular proposal
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "erratic-pattern (via GitHub)" <gi...@apache.org>.
erratic-pattern commented on code in PR #40243:
URL: https://github.com/apache/arrow/pull/40243#discussion_r1511382303
##########
format/FlightSql.proto:
##########
@@ -1797,6 +1797,15 @@ message DoPutUpdateResult {
int64 record_count = 1;
}
+// An optional response returned when `DoPut` is called with `CommandPreparedStatementQuery`
+message DoPutPreparedStatementResult {
+ 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
Review Comment:
> 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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "joellubi (via GitHub)" <gi...@apache.org>.
joellubi commented on PR #40243:
URL: https://github.com/apache/arrow/pull/40243#issuecomment-1989392929
With that framing it makes sense, thanks @lidavidm.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document stateless prepared statements [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #40243:
URL: https://github.com/apache/arrow/pull/40243#discussion_r1534987835
##########
docs/source/format/FlightSql.rst:
##########
@@ -141,6 +141,21 @@ 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 provide an updated handle in the response.
+ 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.
+
+ 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
Review Comment:
```suggestion
The subsequent call itself may return an updated handle which again should
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37720: [Format][Docs][FlightSQL] Document FlightSQL stateless prepared statements [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40243:
URL: https://github.com/apache/arrow/pull/40243#issuecomment-1973797539
:warning: GitHub issue #37720 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org