Flight SQL Ratification Based On Community Feedback #6 (apache#94)

* Refactored FlightSql Statement Constant names

* Defined non-nullable parameters for FlightSql proto

* Resolved minimal checkstyle issues

* Added further documentation for catalog and schema

* Refactored FlightSql proto comments to include more information

* Added Field/FieldType notNullable methods

* Refactored FlightSqlClient and FlightSqlExample to leverage Field notNullable method

* Removed opaque query warning from FlightSql proto

* Added the optional tag for the returned schema of getTables to proto

Author: vfraga

format/FlightSql.proto

@@ -31,7 +31,7 @@ package arrow.flight.protocol.sql;
  *
  * The returned schema will be:
  * <
- *  info_name: uint32,
+ *  info_name: uint32 not null,
  *  value: dense_union<string_value: string, int_value: int32, bigint_value: int64, int32_bitmask: int32>
  * >
  * where there is one row per requested piece of metadata information.
@@ -156,7 +156,7 @@ enum SqlInfo {
  *
  * The returned schema will be:
  * <
- *  catalog_name: utf8
+ *  catalog_name: utf8 not null
  * >
  * The returned data should be ordered by catalog_name.
  */
@@ -173,16 +173,17 @@ message CommandGetCatalogs {
  * The returned schema will be:
  * <
  *  catalog_name: utf8,
- *  schema_name: utf8
+ *  schema_name: utf8 not null
  * >
  * The returned data should be ordered by catalog_name, then schema_name.
  */
 message CommandGetSchemas {
   option (experimental) = true;
 
   /*
-   * Specifies the Catalog to search for schemas.
-   * If omitted, then all catalogs are searched.
+   * Specifies the Catalog to search for the tables.
+   * An empty string retrieves those without a catalog.
+   * If omitted the catalog name should not be used to narrow the search.
    */
   google.protobuf.StringValue catalog = 1;
 
@@ -206,18 +207,20 @@ message CommandGetSchemas {
  * <
  *  catalog_name: utf8,
  *  schema_name: utf8,
- *  table_name: utf8,
- *  table_type: utf8,
- *  table_schema: bytes (schema of the table as described in Schema.fbs::Schema, it is serialized as an IPC message.)
+ *  table_name: utf8 not null,
+ *  table_type: utf8 not null,
+ *  [optional] table_schema: bytes not null (schema of the table as described in Schema.fbs::Schema,
+ *                                           it is serialized as an IPC message.)
  * >
- * The returned data should be ordered by catalog_name, schema_name, table_name, then table_type.
+ * The returned data should be ordered by catalog_name, schema_name, table_name, then table_type, followed by table_schema if requested.
  */
 message CommandGetTables {
   option (experimental) = true;
 
   /*
    * Specifies the Catalog to search for the tables.
-   * If omitted, then all catalogs are searched.
+   * An empty string retrieves those without a catalog.
+   * If omitted the catalog name should not be used to narrow the search.
    */
   google.protobuf.StringValue catalog = 1;
 
@@ -254,7 +257,7 @@ message CommandGetTables {
  *
  * The returned schema will be:
  * <
- *  table_type: utf8
+ *  table_type: utf8 not null
  * >
  * The returned data should be ordered by table_type.
  */
@@ -282,14 +285,22 @@ message CommandGetTableTypes {
 message CommandGetPrimaryKeys {
   option (experimental) = true;
 
-  // Specifies the catalog to search for the table.
+  /*
+   * Specifies the catalog to search for the table.
+   * An empty string retrieves those without a catalog.
+   * If omitted the catalog name should not be used to narrow the search.
+   */
   google.protobuf.StringValue catalog = 1;
 
-  // Specifies the schema to search for the table.
+  /*
+   * Specifies the schema to search for the table.
+   * An empty string retrieves those without a schema.
+   * If omitted the schema name should not be used to narrow the search.
+   */
   google.protobuf.StringValue schema = 2;
 
   // Specifies the table to get the primary keys for.
-  google.protobuf.StringValue table = 3;
+  string table = 3;
 }
 
 /*
@@ -303,28 +314,36 @@ message CommandGetPrimaryKeys {
  * <
  *  pk_catalog_name: utf8,
  *  pk_schema_name: utf8,
- *  pk_table_name: utf8,
- *  pk_column_name: utf8,
+ *  pk_table_name: utf8 not null,
+ *  pk_column_name: utf8 not null,
  *  fk_catalog_name: utf8,
  *  fk_schema_name: utf8,
- *  fk_table_name: utf8,
- *  fk_column_name: utf8,
- *  key_sequence: int,
+ *  fk_table_name: utf8 not null,
+ *  fk_column_name: utf8 not null,
+ *  key_sequence: int not null,
  *  fk_key_name: utf8,
  *  pk_key_name: utf8,
- *  update_rule: uint1,
- *  delete_rule: uint1
+ *  update_rule: uint1 not null,
+ *  delete_rule: uint1 not null
  * >
  * The returned data should be ordered by fk_catalog_name, fk_schema_name, fk_table_name, fk_key_name, then key_sequence.
  * update_rule and delete_rule returns a byte that is equivalent to actions declared on UpdateDeleteRules enum.
  */
 message CommandGetExportedKeys {
   option (experimental) = true;
 
-  // Specifies the catalog to search for the foreign key table.
+  /*
+   * Specifies the catalog to search for the foreign key table.
+   * An empty string retrieves those without a catalog.
+   * If omitted the catalog name should not be used to narrow the search.
+   */
   google.protobuf.StringValue catalog = 1;
 
-  // Specifies the schema to search for the foreign key table.
+  /*
+   * Specifies the schema to search for the foreign key table.
+   * An empty string retrieves those without a schema.
+   * If omitted the schema name should not be used to narrow the search.
+   */
   google.protobuf.StringValue schema = 2;
 
   // Specifies the foreign key table to get the foreign keys for.
@@ -349,17 +368,17 @@ enum UpdateDeleteRules {
  * <
  *  pk_catalog_name: utf8,
  *  pk_schema_name: utf8,
- *  pk_table_name: utf8,
- *  pk_column_name: utf8,
+ *  pk_table_name: utf8 not null,
+ *  pk_column_name: utf8 not null,
  *  fk_catalog_name: utf8,
  *  fk_schema_name: utf8,
- *  fk_table_name: utf8,
- *  fk_column_name: utf8,
- *  key_sequence: int,
+ *  fk_table_name: utf8 not null,
+ *  fk_column_name: utf8 not null,
+ *  key_sequence: int not null,
  *  fk_key_name: utf8,
  *  pk_key_name: utf8,
- *  update_rule: uint1,
- *  delete_rule: uint1
+ *  update_rule: uint1 not null,
+ *  delete_rule: uint1 not null
  * >
  * The returned data should be ordered by pk_catalog_name, pk_schema_name, pk_table_name, pk_key_name, then key_sequence.
  * update_rule and delete_rule returns a byte that is equivalent to actions:
@@ -372,10 +391,18 @@ enum UpdateDeleteRules {
 message CommandGetImportedKeys {
   option (experimental) = true;
 
-  // Specifies the catalog to search for the primary key table.
+  /*
+   * Specifies the catalog to search for the primary key table.
+   * An empty string retrieves those without a catalog.
+   * If omitted the catalog name should not be used to narrow the search.
+   */
   google.protobuf.StringValue catalog = 1;
 
-  // Specifies the schema to search for the primary key table.
+  /*
+   * Specifies the schema to search for the primary key table.
+   * An empty string retrieves those without a schema.
+   * If omitted the schema name should not be used to narrow the search.
+   */
   google.protobuf.StringValue schema = 2;
 
   // Specifies the primary key table to get the foreign keys for.
@@ -391,12 +418,15 @@ message ActionCreatePreparedStatementRequest {
   option (experimental) = true;
 
   // The valid SQL string to create a prepared statement for.
-  // The query should be treated as an opaque value, that is, clients should not attempt to parse this.
   string query = 1;
 }
 
 /*
  * Wrap the result of a "GetPreparedStatement" action.
+ *
+ * The resultant PreparedStatement can be closed either:
+ * - Manually, through the "ClosePreparedStatement" action;
+ * - Automatically, by a server timeout.
  */
 message ActionCreatePreparedStatementResult {
   option (experimental) = true;
@@ -437,7 +467,6 @@ message CommandStatementQuery {
   option (experimental) = true;
 
   // The SQL syntax.
-  // The query should be treated as an opaque value, that is, clients should not attempt to parse this.
   string query = 1;
 }
 
@@ -473,7 +502,6 @@ message CommandStatementUpdate {
   option (experimental) = true;
 
   // The SQL syntax.
-  // The query should be treated as an opaque value, that is, clients should not attempt to parse this.
   string query = 1;
 }
 

java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlClient.java

@@ -266,9 +266,9 @@ public FlightInfo getPrimaryKeys(final String catalog, final String schema,
       builder.setSchema(StringValue.newBuilder().setValue(schema).build());
     }
 
-    if (table != null) {
-      builder.setTable(StringValue.newBuilder().setValue(table).build());
-    }
+    Objects.requireNonNull(table);
+    builder.setTable(table).build();
+
     final FlightDescriptor descriptor = FlightDescriptor.command(Any.pack(builder.build()).toByteArray());
     return client.getInfo(descriptor, options);
   }
@@ -376,7 +376,7 @@ public static class PreparedStatement implements AutoCloseable {
     public PreparedStatement(final FlightClient client, final String sql, final CallOption... options) {
       this.client = client;
       final Action action = new Action(
-          FlightSqlUtils.FLIGHT_SQL_CREATEPREPAREDSTATEMENT.getType(),
+          FlightSqlUtils.FLIGHT_SQL_CREATE_PREPARED_STATEMENT.getType(),
           Any.pack(ActionCreatePreparedStatementRequest
                   .newBuilder()
                   .setQuery(sql)
@@ -530,7 +530,7 @@ public void close(final CallOption... options) {
       }
       isClosed = true;
       final Action action = new Action(
-          FlightSqlUtils.FLIGHT_SQL_CLOSEPREPAREDSTATEMENT.getType(),
+          FlightSqlUtils.FLIGHT_SQL_CLOSE_PREPARED_STATEMENT.getType(),
           Any.pack(ActionClosePreparedStatementRequest.newBuilder()
                   .setPreparedStatementHandle(preparedStatementResult.getPreparedStatementHandle())
                   .build())

java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlProducer.java

@@ -250,11 +250,11 @@ default void listActions(CallContext context, StreamListener<ActionType> listene
   @Override
   default void doAction(CallContext context, Action action, StreamListener<Result> listener) {
     final String actionType = action.getType();
-    if (actionType.equals(FlightSqlUtils.FLIGHT_SQL_CREATEPREPAREDSTATEMENT.getType())) {
+    if (actionType.equals(FlightSqlUtils.FLIGHT_SQL_CREATE_PREPARED_STATEMENT.getType())) {
       final ActionCreatePreparedStatementRequest request = FlightSqlUtils.unpackAndParseOrThrow(action.getBody(),
           ActionCreatePreparedStatementRequest.class);
       createPreparedStatement(request, context, listener);
-    } else if (actionType.equals(FlightSqlUtils.FLIGHT_SQL_CLOSEPREPAREDSTATEMENT.getType())) {
+    } else if (actionType.equals(FlightSqlUtils.FLIGHT_SQL_CLOSE_PREPARED_STATEMENT.getType())) {
       final ActionClosePreparedStatementRequest request = FlightSqlUtils.unpackAndParseOrThrow(action.getBody(),
           ActionClosePreparedStatementRequest.class);
       closePreparedStatement(request, context, listener);
@@ -566,38 +566,38 @@ final class Schemas {
     public static final Schema GET_TABLES_SCHEMA = new Schema(Arrays.asList(
         Field.nullable("catalog_name", MinorType.VARCHAR.getType()),
         Field.nullable("schema_name", MinorType.VARCHAR.getType()),
-        Field.nullable("table_name", MinorType.VARCHAR.getType()),
-        Field.nullable("table_type", MinorType.VARCHAR.getType()),
-        Field.nullable("table_schema", MinorType.VARBINARY.getType())));
+        Field.notNullable("table_name", MinorType.VARCHAR.getType()),
+        Field.notNullable("table_type", MinorType.VARCHAR.getType()),
+        Field.notNullable("table_schema", MinorType.VARBINARY.getType())));
     public static final Schema GET_TABLES_SCHEMA_NO_SCHEMA = new Schema(Arrays.asList(
         Field.nullable("catalog_name", MinorType.VARCHAR.getType()),
         Field.nullable("schema_name", MinorType.VARCHAR.getType()),
-        Field.nullable("table_name", MinorType.VARCHAR.getType()),
-        Field.nullable("table_type", MinorType.VARCHAR.getType())));
+        Field.notNullable("table_name", MinorType.VARCHAR.getType()),
+        Field.notNullable("table_type", MinorType.VARCHAR.getType())));
     public static final Schema GET_CATALOGS_SCHEMA = new Schema(
-        Collections.singletonList(new Field("catalog_name", FieldType.nullable(MinorType.VARCHAR.getType()), null)));
+        Collections.singletonList(Field.notNullable("catalog_name", MinorType.VARCHAR.getType())));
     public static final Schema GET_TABLE_TYPES_SCHEMA =
-        new Schema(Collections.singletonList(Field.nullable("table_type", MinorType.VARCHAR.getType())));
+        new Schema(Collections.singletonList(Field.notNullable("table_type", MinorType.VARCHAR.getType())));
     public static final Schema GET_SCHEMAS_SCHEMA = new Schema(
         Arrays.asList(Field.nullable("catalog_name", MinorType.VARCHAR.getType()),
-            Field.nullable("schema_name", MinorType.VARCHAR.getType())));
+            Field.notNullable("schema_name", MinorType.VARCHAR.getType())));
     public static final Schema GET_IMPORTED_AND_EXPORTED_KEYS_SCHEMA = new Schema(Arrays.asList(
         Field.nullable("pk_catalog_name", MinorType.VARCHAR.getType()),
         Field.nullable("pk_schema_name", MinorType.VARCHAR.getType()),
-        Field.nullable("pk_table_name", MinorType.VARCHAR.getType()),
-        Field.nullable("pk_column_name", MinorType.VARCHAR.getType()),
+        Field.notNullable("pk_table_name", MinorType.VARCHAR.getType()),
+        Field.notNullable("pk_column_name", MinorType.VARCHAR.getType()),
         Field.nullable("fk_catalog_name", MinorType.VARCHAR.getType()),
         Field.nullable("fk_schema_name", MinorType.VARCHAR.getType()),
-        Field.nullable("fk_table_name", MinorType.VARCHAR.getType()),
-        Field.nullable("fk_column_name", MinorType.VARCHAR.getType()),
-        Field.nullable("key_sequence", MinorType.INT.getType()),
+        Field.notNullable("fk_table_name", MinorType.VARCHAR.getType()),
+        Field.notNullable("fk_column_name", MinorType.VARCHAR.getType()),
+        Field.notNullable("key_sequence", MinorType.INT.getType()),
         Field.nullable("fk_key_name", MinorType.VARCHAR.getType()),
         Field.nullable("pk_key_name", MinorType.VARCHAR.getType()),
-        Field.nullable("update_rule", new ArrowType.Int(8, false)),
-        Field.nullable("delete_rule", new ArrowType.Int(8, false))));
+        Field.notNullable("update_rule", new ArrowType.Int(8, false)),
+        Field.notNullable("delete_rule", new ArrowType.Int(8, false))));
     public static final Schema GET_SQL_INFO_SCHEMA =
         new Schema(Arrays.asList(
-            Field.nullable("info_name", new ArrowType.Int(32, false)),
+            Field.notNullable("info_name", new ArrowType.Int(32, false)),
             new Field("value",
                 // dense_union<string_value: string, int_value: int32, bigint_value: int64, int32_bitmask: int32>
                 new FieldType(true, new Union(UnionMode.Dense, new int[] {0, 1, 2, 3}), /*dictionary=*/null),
@@ -609,9 +609,9 @@ final class Schemas {
     public static final Schema GET_PRIMARY_KEYS_SCHEMA = new Schema(Arrays.asList(
         Field.nullable("catalog_name", MinorType.VARCHAR.getType()),
         Field.nullable("schema_name", MinorType.VARCHAR.getType()),
-        Field.nullable("table_name", MinorType.VARCHAR.getType()),
-        Field.nullable("column_name", MinorType.VARCHAR.getType()),
-        Field.nullable("key_sequence", MinorType.INT.getType()),
+        Field.notNullable("table_name", MinorType.VARCHAR.getType()),
+        Field.notNullable("column_name", MinorType.VARCHAR.getType()),
+        Field.notNullable("key_sequence", MinorType.INT.getType()),
         Field.nullable("key_name", MinorType.VARCHAR.getType())));
 
     private Schemas() {

java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlUtils.java

@@ -31,19 +31,19 @@
  * Utilities to work with Flight SQL semantics.
  */
 public final class FlightSqlUtils {
-  public static final ActionType FLIGHT_SQL_CREATEPREPAREDSTATEMENT = new ActionType("CreatePreparedStatement",
+  public static final ActionType FLIGHT_SQL_CREATE_PREPARED_STATEMENT = new ActionType("CreatePreparedStatement",
       "Creates a reusable prepared statement resource on the server. \n" +
           "Request Message: ActionCreatePreparedStatementRequest\n" +
           "Response Message: ActionCreatePreparedStatementResult");
 
-  public static final ActionType FLIGHT_SQL_CLOSEPREPAREDSTATEMENT = new ActionType("ClosePreparedStatement",
+  public static final ActionType FLIGHT_SQL_CLOSE_PREPARED_STATEMENT = new ActionType("ClosePreparedStatement",
       "Closes a reusable prepared statement resource on the server. \n" +
           "Request Message: ActionClosePreparedStatementRequest\n" +
           "Response Message: N/A");
 
   public static final List<ActionType> FLIGHT_SQL_ACTIONS = ImmutableList.of(
-      FLIGHT_SQL_CREATEPREPAREDSTATEMENT,
-      FLIGHT_SQL_CLOSEPREPAREDSTATEMENT
+      FLIGHT_SQL_CREATE_PREPARED_STATEMENT,
+      FLIGHT_SQL_CLOSE_PREPARED_STATEMENT
   );
 
   /**