Comments

extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java

@@ -648,26 +648,30 @@ interface TicketHandlerReleasable extends TicketHandler {
     }
 
     private Table executeSqlQuery(String sql) {
-        // See SQLTODO(catalog-reader-implementation)
         final ExecutionContext executionContext = ExecutionContext.getContext();
         final QueryScope queryScope = executionContext.getQueryScope();
-
         // We aren't managing the liveness of Tables that come verbatim (authorization un-transformed) from the query
-        // scope. In the case where they either are already not live, or become not live by the time the operation logic
-        // is executed, an appropriate exception will be thrown. While this is a liveness race, it isn't technically
-        // much different than a liveness race possible via ScopeTicketResolver.resolve.
+        // scope (we are ensuring that any transformed, or operation created, tables don't escape to a higher-layer's
+        // liveness scope). In the case where they either are already not live, or become not live by the time the
+        // operation logic is executed, an appropriate exception will be thrown. While this is a liveness race, it isn't
+        // technically much different than a liveness race possible via ScopeTicketResolver.resolve.
         //
         // The proper way to do this would be to re-model the table execution logic of GrpcTableOperation (gRPC) into a
         // QST form, whereby table dependencies are presented as properly-scoped, liveness-managed Exports for the
         // duration of the operation.
         try (final SafeCloseable ignored = LivenessScopeStack.open()) {
+            // See SQLTODO(catalog-reader-implementation)
+            // Unfortunately, we must do authorization.transform on all the query scope tables up-front; to make this
+            // on-demand, we need to implement a Calcite Catalog Reader (non-trivial). Technically, parseSql only needs
+            // to know the definitions of the tables; we could consider a table-specific authorization interface that
+            // presents the specialization Authorization.transformedDefinition(Table) to make this cheaper in a lot of
+            // cases.
             final Map<String, Table> queryScopeTables =
                     queryScope.toMap(o -> queryScopeAuthorizedTableMapper(queryScope, o), (n, t) -> t != null);
             final TableSpec tableSpec =
                     Sql.parseSql(sql, queryScopeTables, TableCreatorScopeTickets::ticketTable, null);
             final TableCreator<Table> tableCreator =
                     new TableCreatorScopeTickets(TableCreatorImpl.INSTANCE, queryScopeTables);
-
             // We could consider doing finer-grained sharedLock in the future; right now, taking it for the whole
             // operation if any of the TicketTable sources are refreshing.
             final List<Table> refreshingTables = new ArrayList<>();
@@ -682,13 +686,11 @@ private Table executeSqlQuery(String sql) {
                     refreshingTables.add(sourceTable);
                 }
             }
-
             final UpdateGraph updateGraph = refreshingTables.isEmpty()
                     ? null
                     : NotificationQueue.Dependency.getUpdateGraph(null, refreshingTables.toArray(new Table[0]));
-
-            // Note: this is doing io.deephaven.server.session.TicketResolver.Authorization.transform, but not
-            // io.deephaven.auth.ServiceAuthWiring
+            // Note: Authorization.transform has already been performed, but we are _not_ doing
+            // io.deephaven.auth.ServiceAuthWiring checks.
             // TODO(deephaven-core#6307): Declarative server-side table execution logic that preserves authorization
             // logic
             try (final SafeCloseable ignored1 = updateGraph == null ? null : updateGraph.sharedLock().lockCloseable()) {