lidavidm commented on code in PR #13492:
URL: https://github.com/apache/arrow/pull/13492#discussion_r953012756
##########
format/FlightSql.proto:
##########
@@ -1418,7 +1442,17 @@ message ActionCreatePreparedStatementRequest {
}
/*
- * Wrap the result of a "GetPreparedStatement" action.
+ * Request message for the "CreatePreparedSubstraitPlan" action on a Flight
SQL enabled backend.
+ */
+message ActionCreatePreparedSubstraitPlan {
+ option (experimental) = true;
+
+ // The serialized substrait.Plan to create a prepared statement for.
+ bytes plan = 1;
Review Comment:
I'd like to keep code dependencies on other Protobuf messages out because
Windows/Protobuf has issues with those when they're in different DLLs (as they
are with Flight/Flight SQL, and as they would be here). As seen below with the
CancelQuery message I already ran into linking issues and I think they're
insurmountable unless protoc itself is modified.
##########
format/FlightSql.proto:
##########
@@ -1418,7 +1442,17 @@ message ActionCreatePreparedStatementRequest {
}
/*
- * Wrap the result of a "GetPreparedStatement" action.
+ * Request message for the "CreatePreparedSubstraitPlan" action on a Flight
SQL enabled backend.
+ */
+message ActionCreatePreparedSubstraitPlan {
+ option (experimental) = true;
+
+ // The serialized substrait.Plan to create a prepared statement for.
+ bytes plan = 1;
Review Comment:
On the wire, the actual encoding is the same either way.
##########
format/FlightSql.proto:
##########
@@ -761,6 +798,20 @@ enum SqlInfo {
SQL_STORED_FUNCTIONS_USING_CALL_SYNTAX_SUPPORTED = 576;
}
+// The level of support for Flight SQL transaction RPCs.
+enum SqlSupportedTransaction {
+ // Unknown/not indicated
+ SQL_SUPPORTED_TRANSACTION_UNKNOWN = 0;
+ // No support
+ SQL_SUPPORTED_TRANSACTION_NONE = 1;
Review Comment:
Not really - I guess in that case let's just fold them together since
there's no point. (I suppose normally in protobuf you'd distinguish the two,
but that doesn't apply here.)
##########
format/FlightSql.proto:
##########
@@ -1550,6 +1755,48 @@ message DoPutUpdateResult {
int64 record_count = 1;
}
+/*
+ * Request message for the "CancelQuery" action.
+ *
+ * Explicitly cancel a running query.
+ *
+ * This lets a single client explicitly cancel work, no matter how many clients
+ * are involved/whether the query is distributed or not, given server support.
+ * The transaction/statement is not rolled back; it is the application's job to
+ * commit or rollback as appropriate. This only indicates the client no longer
+ * wishes to read the remainder of the query results or continue submitting
+ * data.
+ *
+ * This command is idempotent.
+ */
+message ActionCancelQueryRequest {
+ option (experimental) = true;
+
+ // The result of the GetFlightInfo RPC that initated the query.
+ // XXX(ARROW-16902): this must be a serialized FlightInfo, but is
+ // rendered as bytes because Protobuf does not really support one
+ // DLL using Protobuf definitions from another DLL.
+ bytes info = 1;
Review Comment:
It should be a FlightInfo because I assume the server needs the information
of _all_ endpoints in order to fully cancel a query. Also in the event that we
do update FlightInfo with an application metadata field, it would automatically
get passed back to the server
##########
format/FlightSql.proto:
##########
@@ -1550,6 +1755,48 @@ message DoPutUpdateResult {
int64 record_count = 1;
}
+/*
+ * Request message for the "CancelQuery" action.
+ *
+ * Explicitly cancel a running query.
+ *
+ * This lets a single client explicitly cancel work, no matter how many clients
+ * are involved/whether the query is distributed or not, given server support.
+ * The transaction/statement is not rolled back; it is the application's job to
+ * commit or rollback as appropriate. This only indicates the client no longer
+ * wishes to read the remainder of the query results or continue submitting
+ * data.
+ *
+ * This command is idempotent.
+ */
+message ActionCancelQueryRequest {
+ option (experimental) = true;
+
+ // The result of the GetFlightInfo RPC that initated the query.
+ // XXX(ARROW-16902): this must be a serialized FlightInfo, but is
+ // rendered as bytes because Protobuf does not really support one
+ // DLL using Protobuf definitions from another DLL.
+ bytes info = 1;
+}
+
+/*
+ * The result of cancelling a query.
+ *
+ * The result should be wrapped in a google.protobuf.Any message.
+ */
+message ActionCancelQueryResult {
+ option (experimental) = true;
+
+ enum CancelResult {
+ CANCEL_RESULT_UNSPECIFIED = 0;
+ CANCEL_RESULT_CANCELLED = 1;
+ CANCEL_RESULT_CANCELLING = 2;
+ CANCEL_RESULT_NOT_CANCELLABLE = 3;
Review Comment:
That was the intent. I'll document the variants.
--
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]
