You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/07/19 09:31:30 UTC

[GitHub] [arrow] pitrou commented on a diff in pull request #13492: RFC: [FlightRPC][WIP] Substrait, transaction, cancellation for Flight SQL

pitrou commented on code in PR #13492:
URL: https://github.com/apache/arrow/pull/13492#discussion_r924271806


##########
format/FlightSql.proto:
##########
@@ -89,6 +90,31 @@ enum SqlInfo {
    */
   FLIGHT_SQL_SERVER_READ_ONLY = 3;
 
+  /*
+   * Retrieves a boolean value indicating whether the Flight SQL Server supports executing Substrait plans.

Review Comment:
   So "Flight SQL" is generic enough that it might support things other than actual SQL?



##########
format/FlightSql.proto:
##########
@@ -89,6 +90,31 @@ enum SqlInfo {
    */
   FLIGHT_SQL_SERVER_READ_ONLY = 3;
 
+  /*
+   * Retrieves a boolean value indicating whether the Flight SQL Server supports executing Substrait plans.
+   */
+  FLIGHT_SQL_SUBSTRAIT = 4;
+
+  /*
+   * Retrieves an int32 indicating whether the Flight SQL Server supports explicit transaction RPCs.
+   *
+   * The possible values are listed in `FlightSqlTransactionSupport`.
+   */
+  FLIGHT_SQL_TRANSACTION = 5;
+
+  /*
+   * Retrieves an int32 indicating the timeout for prepared statement handles.

Review Comment:
   Er... which unit is that? Seconds? Can we make it a real/float instead, if protobuf allows that?



##########
format/FlightSql.proto:
##########
@@ -1450,8 +1500,55 @@ message ActionClosePreparedStatementRequest {
   bytes prepared_statement_handle = 1;
 }
 
+/*
+ * Request message for the "BeginTransaction" action.
+ * Begins a transaction or creates a savepoint within a transaction.
+ */
+message ActionBeginTransactionRequest {
+  // The transaction to which a savepoint belongs, if applicable.
+  //
+  // To begin a transaction, leave this field empty.

Review Comment:
   Hmm... should there be two separate commands for starting a transaction and a savepoint, so that they can take different parameters?



##########
format/FlightSql.proto:
##########
@@ -760,6 +786,20 @@ enum SqlInfo {
   SQL_STORED_FUNCTIONS_USING_CALL_SYNTAX_SUPPORTED = 576;
 }
 
+// The level of support for Flight SQL transaction RPCs.
+enum FlightSqlTransactionSupport {
+  // Unknown/not indicated
+  FLIGHT_SQL_TRANSACTION_SUPPORT_UNKNOWN = 0;
+  // No support
+  FLIGHT_SQL_TRANSACTION_SUPPORT_NONE = 1;
+  // Transactions, but not savepoints.
+  // A savepoint is a mark within a transaction that can be individually
+  // rolled back to. Not all databases support savepoints.

Review Comment:
   Just for my understanding, savepoints are for two-phase commits, right?



##########
format/FlightSql.proto:
##########
@@ -1475,6 +1572,35 @@ message CommandStatementQuery {
 
   // The SQL syntax.
   string query = 1;
+  // Include the query as part of this transaction (by default queries are auto-committed).
+  bytes transaction_id = 2;
+}
+
+/*
+ * Represents a Substrait plan. Used in the command member of FlightDescriptor
+ * for the following RPC calls:
+ *  - GetSchema: return the Arrow schema of the query.
+ *    Fields on this schema may contain the following metadata:
+ *    - ARROW:FLIGHT:SQL:CATALOG_NAME      - Table's catalog name
+ *    - ARROW:FLIGHT:SQL:DB_SCHEMA_NAME    - Database schema name
+ *    - ARROW:FLIGHT:SQL:TABLE_NAME        - Table name
+ *    - ARROW:FLIGHT:SQL:TYPE_NAME         - The data source-specific name for the data type of the column.
+ *    - ARROW:FLIGHT:SQL:PRECISION         - Column precision/size
+ *    - ARROW:FLIGHT:SQL:SCALE             - Column scale/decimal digits if applicable
+ *    - ARROW:FLIGHT:SQL:IS_AUTO_INCREMENT - "1" indicates if the column is auto incremented, "0" otherwise.
+ *    - ARROW:FLIGHT:SQL:IS_CASE_SENSITIVE - "1" indicates if the column is case sensitive, "0" otherwise.
+ *    - ARROW:FLIGHT:SQL:IS_READ_ONLY      - "1" indicates if the column is read only, "0" otherwise.
+ *    - ARROW:FLIGHT:SQL:IS_SEARCHABLE     - "1" indicates if the column is searchable via WHERE clause, "0" otherwise.
+ *  - GetFlightInfo: execute the query.
+ *  - DoPut: execute the query.
+ */
+message CommandStatementSubstraitPlan {
+  option (experimental) = true;
+
+  // A serialized substrait.Plan
+  bytes plan = 1;
+  // Include the query as part of this transaction (by default queries are auto-committed).

Review Comment:
   Does the "default" happen if `transaction_id` is left unset?



##########
format/FlightSql.proto:
##########
@@ -1475,6 +1572,35 @@ message CommandStatementQuery {
 
   // The SQL syntax.
   string query = 1;
+  // Include the query as part of this transaction (by default queries are auto-committed).
+  bytes transaction_id = 2;
+}
+
+/*
+ * Represents a Substrait plan. Used in the command member of FlightDescriptor
+ * for the following RPC calls:
+ *  - GetSchema: return the Arrow schema of the query.
+ *    Fields on this schema may contain the following metadata:
+ *    - ARROW:FLIGHT:SQL:CATALOG_NAME      - Table's catalog name
+ *    - ARROW:FLIGHT:SQL:DB_SCHEMA_NAME    - Database schema name
+ *    - ARROW:FLIGHT:SQL:TABLE_NAME        - Table name
+ *    - ARROW:FLIGHT:SQL:TYPE_NAME         - The data source-specific name for the data type of the column.
+ *    - ARROW:FLIGHT:SQL:PRECISION         - Column precision/size
+ *    - ARROW:FLIGHT:SQL:SCALE             - Column scale/decimal digits if applicable
+ *    - ARROW:FLIGHT:SQL:IS_AUTO_INCREMENT - "1" indicates if the column is auto incremented, "0" otherwise.
+ *    - ARROW:FLIGHT:SQL:IS_CASE_SENSITIVE - "1" indicates if the column is case sensitive, "0" otherwise.
+ *    - ARROW:FLIGHT:SQL:IS_READ_ONLY      - "1" indicates if the column is read only, "0" otherwise.
+ *    - ARROW:FLIGHT:SQL:IS_SEARCHABLE     - "1" indicates if the column is searchable via WHERE clause, "0" otherwise.
+ *  - GetFlightInfo: execute the query.
+ *  - DoPut: execute the query.
+ */
+message CommandStatementSubstraitPlan {
+  option (experimental) = true;
+
+  // A serialized substrait.Plan
+  bytes plan = 1;
+  // Include the query as part of this transaction (by default queries are auto-committed).

Review Comment:
   If so, perhaps replace "by default" with "if unset, " for clarity?



-- 
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