From 65775e9f9afa1de254f7b4db64b2f954ef0db51c Mon Sep 17 00:00:00 2001 From: Devin Smith Date: Wed, 11 Dec 2024 08:16:19 -0800 Subject: [PATCH] Comments --- .../server/flightsql/FlightSqlResolver.java | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java b/extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java index 689311458ec..3cd854f695b 100644 --- a/extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java +++ b/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 queryScopeTables = queryScope.toMap(o -> queryScopeAuthorizedTableMapper(queryScope, o), (n, t) -> t != null); final TableSpec tableSpec = Sql.parseSql(sql, queryScopeTables, TableCreatorScopeTickets::ticketTable, null); final TableCreator 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
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()) {