From afcddf72188a2d212ce48fbec163818e312a5601 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Wed, 20 Dec 2023 11:21:40 -0600 Subject: [PATCH 01/23] Extract variable api from ScriptSession, let ScriptSession guard reads This patch moves from assuming that the ScriptSession owns the ExecContext for locking (with UpdateGraph) and variable access (with QueryScope). Previously, the groovy script session used a lock to interact with its query scope, but python did not under the incorrect assumption that the GIL would guard those reads. Instead, a read/write lock is used to ensure that reads block while an operation is running that could affect values, but avoiding reads blocking each other. Partial #4040 --- .../python/PythonDeephavenSession.java | 8 - .../java/io/deephaven/engine/sql/Sql.java | 15 +- .../engine/util/AbstractScriptSession.java | 200 ++++++++++++------ .../engine/util/DelegatingScriptSession.java | 33 --- .../engine/util/GroovyDeephavenSession.java | 5 - .../util/NoLanguageDeephavenSession.java | 6 - .../deephaven/engine/util/ScriptSession.java | 56 ----- .../engine/util/VariableProvider.java | 6 +- .../deephaven/client/ObjectServiceTest.java | 12 +- .../console/ConsoleServiceGrpcImpl.java | 4 +- .../server/console/ScopeTicketResolver.java | 49 ++--- .../completer/PythonAutoCompleteObserver.java | 11 +- .../server/uri/QueryScopeResolver.java | 12 +- .../ApplicationServiceGrpcImplTest.java | 2 +- .../test/FlightMessageRoundTripTest.java | 34 +-- 15 files changed, 207 insertions(+), 246 deletions(-) diff --git a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java index 59c03d1551d..9b6ba40e9ab 100644 --- a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java +++ b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java @@ -23,7 +23,6 @@ import io.deephaven.plugin.type.ObjectTypeLookup.NoOp; import io.deephaven.util.SafeCloseable; import io.deephaven.util.annotations.ScriptApi; -import io.deephaven.util.annotations.VisibleForTesting; import io.deephaven.util.thread.NamingThreadFactory; import io.deephaven.util.thread.ThreadInitializationFactory; import org.jetbrains.annotations.NotNull; @@ -144,13 +143,6 @@ public Thread newThread(@NotNull Runnable r) { } } - @Override - @VisibleForTesting - public QueryScope newQueryScope() { - // depend on the GIL instead of local synchronization - return new UnsynchronizedScriptSessionQueryScope(this); - } - /** * Finds the specified script; and runs it as a file, or if it is a stream writes it to a temporary file in order to * run it. diff --git a/engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java b/engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java index 9fd5af47723..23657503c64 100644 --- a/engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java +++ b/engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java @@ -3,12 +3,11 @@ import io.deephaven.base.log.LogOutput; import io.deephaven.base.log.LogOutput.ObjFormatter; import io.deephaven.engine.context.ExecutionContext; +import io.deephaven.engine.context.QueryScope; import io.deephaven.engine.table.ColumnDefinition; import io.deephaven.engine.table.Table; import io.deephaven.engine.table.TableDefinition; import io.deephaven.engine.table.impl.TableCreatorImpl; -import io.deephaven.engine.util.AbstractScriptSession.ScriptSessionQueryScope; -import io.deephaven.engine.util.ScriptSession; import io.deephaven.internal.log.LoggerFactory; import io.deephaven.io.logger.Logger; import io.deephaven.qst.column.header.ColumnHeader; @@ -83,18 +82,16 @@ private static Map currentScriptSessionNamedTables() { final Map scope = new HashMap<>(); // getVariables() is inefficient // See SQLTODO(catalog-reader-implementation) - for (Entry e : currentScriptSession().getVariables().entrySet()) { - if (e.getValue() instanceof Table) { - scope.put(e.getKey(), (Table) e.getValue()); + QueryScope queryScope = ExecutionContext.getContext().getQueryScope(); + for (String name : queryScope.getParamNames()) { + Object paramValue = queryScope.readParamValue(name); + if (paramValue instanceof Table) { + scope.put(name, (Table) paramValue); } } return scope; } - private static ScriptSession currentScriptSession() { - return ((ScriptSessionQueryScope) ExecutionContext.getContext().getQueryScope()).scriptSession(); - } - private static TableHeader adapt(TableDefinition tableDef) { final Builder builder = TableHeader.builder(); for (ColumnDefinition cd : tableDef.getColumns()) { diff --git a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java index 9bd1679b939..8bbb4945bcc 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java @@ -31,9 +31,12 @@ import java.io.IOException; import java.nio.file.Path; import java.util.LinkedHashSet; +import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.UUID; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; import static io.deephaven.engine.table.Table.NON_DISPLAY_TABLE; @@ -42,7 +45,7 @@ * evaluateScript which handles liveness and diffs in a consistent way. */ public abstract class AbstractScriptSession extends LivenessScope - implements ScriptSession, VariableProvider { + implements ScriptSession { private static final Path CLASS_CACHE_LOCATION = CacheDir.get().resolve("script-session-classes"); @@ -68,6 +71,8 @@ private static void createOrClearDirectory(final File directory) { private final ObjectTypeLookup objectTypeLookup; private final Listener changeListener; + private final ReadWriteLock variableAccessLock = new ReentrantReadWriteLock(); + private S lastSnapshot; protected AbstractScriptSession( @@ -141,6 +146,7 @@ public synchronized final Changes evaluateScript(final String script, @Nullable final Changes diff; // retain any objects which are created in the executed code, we'll release them when the script session // closes + variableAccessLock.writeLock().lock(); try (final S initialSnapshot = takeSnapshot(); final SafeCloseable ignored = LivenessScopeStack.open(this, false)) { @@ -157,6 +163,8 @@ public synchronized final Changes evaluateScript(final String script, @Nullable observeScopeChanges(); // Use the "last" snapshot created as a side effect of observeScopeChanges() as our "to" diff = createDiff(initialSnapshot, lastSnapshot, evaluateErr); + } finally { + variableAccessLock.writeLock().unlock(); } return diff; @@ -247,10 +255,11 @@ protected void destroy() { /** * @return a query scope for this session; only invoked during construction */ - protected abstract QueryScope newQueryScope(); + protected QueryScope newQueryScope() { + return new ScriptSessionQueryScope(); + } - @Override - public Class getVariableType(final String var) { + private Class getVariableType(final String var) { final Object result = getVariable(var, null); if (result == null) { return null; @@ -262,47 +271,149 @@ public Class getVariableType(final String var) { } - @Override public TableDefinition getTableDefinition(final String var) { Object o = getVariable(var, null); return o instanceof Table ? ((Table) o).getDefinition() : null; } + + /** + * Retrieve a variable from the script session's bindings. + *

+ * Please use {@link ScriptSession#getVariable(String, Object)} if you expect the variable may not exist. + * + * @param name the variable to retrieve + * @return the variable + * @throws QueryScope.MissingVariableException if the variable does not exist + */ + @NotNull + protected abstract Object getVariable(String name) throws QueryScope.MissingVariableException; + + /** + * Retrieve a variable from the script session's bindings. If the variable is not present, return defaultValue. + *

+ * If the variable is present, but is not of type (T), a ClassCastException may result. + * + * @param name the variable to retrieve + * @param defaultValue the value to use when no value is present in the session's scope + * @param the type of the variable + * @return the value of the variable, or defaultValue if not present + */ + protected abstract T getVariable(String name, T defaultValue); + + /** + * Retrieves all of the variables present in the session's scope (e.g., Groovy binding, Python globals()). + * + * @return an unmodifiable map with variable names as the keys, and the Objects as the result + */ + protected abstract Map getVariables(); + + /** + * Retrieves all of the variable names present in the session's scope + * + * @return an unmodifiable set of variable names + */ + protected abstract Set getVariableNames(); + + /** + * Check if the scope has the given variable name + * + * @param name the variable name + * @return True iff the scope has the given variable name + */ + protected abstract boolean hasVariableName(String name); + + /** + * Inserts a value into the script's scope. + * + * @param name the variable name to set + * @param value the new value of the variable + */ + protected abstract void setVariable(String name, @Nullable Object value); + @Override public VariableProvider getVariableProvider() { - return this; + return new VariableProvider() { + @Override + public Set getVariableNames() { + variableAccessLock.readLock().lock(); + try { + return AbstractScriptSession.this.getVariableNames(); + } finally { + variableAccessLock.readLock().unlock(); + } + } + + @Override + public Class getVariableType(String var) { + variableAccessLock.readLock().lock(); + try { + return AbstractScriptSession.this.getVariableType(var); + } finally { + variableAccessLock.readLock().unlock(); + } + } + + @Override + public T getVariable(String var, T defaultValue) { + variableAccessLock.readLock().lock(); + try { + return AbstractScriptSession.this.getVariable(var, defaultValue); + } finally { + variableAccessLock.readLock().unlock(); + } + } + + @Override + public TableDefinition getTableDefinition(String var) { + variableAccessLock.readLock().lock(); + try { + return AbstractScriptSession.this.getTableDefinition(var); + } finally { + variableAccessLock.readLock().unlock(); + } + } + + @Override + public boolean hasVariableName(String name) { + variableAccessLock.readLock().lock(); + try { + return AbstractScriptSession.this.hasVariableName(name); + } finally { + variableAccessLock.readLock().unlock(); + } + } + + @Override + public void setVariable(String name, T value) { + variableAccessLock.writeLock().lock(); + try { + AbstractScriptSession.this.setVariable(name, value); + } finally { + variableAccessLock.writeLock().unlock(); + } + } + }; } // ----------------------------------------------------------------------------------------------------------------- // ScriptSession-based QueryScope implementation, with no remote scope or object reflection support // ----------------------------------------------------------------------------------------------------------------- - public abstract static class ScriptSessionQueryScope extends QueryScope { - final ScriptSession scriptSession; - - public ScriptSessionQueryScope(ScriptSession scriptSession) { - this.scriptSession = scriptSession; - } - + public class ScriptSessionQueryScope extends QueryScope { @Override public void putObjectFields(Object object) { throw new UnsupportedOperationException(); } public ScriptSession scriptSession() { - return scriptSession; - } - } - - public static class UnsynchronizedScriptSessionQueryScope extends ScriptSessionQueryScope { - public UnsynchronizedScriptSessionQueryScope(@NotNull final ScriptSession scriptSession) { - super(scriptSession); + return AbstractScriptSession.this; } @Override public Set getParamNames() { final Set result = new LinkedHashSet<>(); - for (final String name : scriptSession.getVariableNames()) { + for (final String name : getVariableProvider().getVariableNames()) { if (NameValidator.isValidQueryParameterName(name)) { result.add(name); } @@ -312,7 +423,7 @@ public Set getParamNames() { @Override public boolean hasParamName(String name) { - return NameValidator.isValidQueryParameterName(name) && scriptSession.hasVariableName(name); + return NameValidator.isValidQueryParameterName(name) && getVariableProvider().hasVariableName(name); } @Override @@ -322,7 +433,7 @@ protected QueryScopeParam createParam(final String name) throw new QueryScope.MissingVariableException("Name " + name + " is invalid"); } // noinspection unchecked - return new QueryScopeParam<>(name, (T) scriptSession.getVariable(name)); + return new QueryScopeParam<>(name, (T) getVariableProvider().getVariable(name, null)); } @Override @@ -331,7 +442,7 @@ public T readParamValue(final String name) throws QueryScope.MissingVariable throw new QueryScope.MissingVariableException("Name " + name + " is invalid"); } // noinspection unchecked - return (T) scriptSession.getVariable(name); + return (T) getVariableProvider().getVariable(name, null); } @Override @@ -339,49 +450,12 @@ public T readParamValue(final String name, final T defaultValue) { if (!NameValidator.isValidQueryParameterName(name)) { return defaultValue; } - return scriptSession.getVariable(name, defaultValue); + return getVariableProvider().getVariable(name, defaultValue); } @Override public void putParam(final String name, final T value) { - scriptSession.setVariable(NameValidator.validateQueryParameterName(name), value); - } - } - - public static class SynchronizedScriptSessionQueryScope extends UnsynchronizedScriptSessionQueryScope { - public SynchronizedScriptSessionQueryScope(@NotNull final ScriptSession scriptSession) { - super(scriptSession); - } - - @Override - public synchronized Set getParamNames() { - return super.getParamNames(); - } - - @Override - public synchronized boolean hasParamName(String name) { - return super.hasParamName(name); - } - - @Override - protected synchronized QueryScopeParam createParam(final String name) - throws QueryScope.MissingVariableException { - return super.createParam(name); - } - - @Override - public synchronized T readParamValue(final String name) throws QueryScope.MissingVariableException { - return super.readParamValue(name); - } - - @Override - public synchronized T readParamValue(final String name, final T defaultValue) { - return super.readParamValue(name, defaultValue); - } - - @Override - public synchronized void putParam(final String name, final T value) { - super.putParam(name, value); + getVariableProvider().setVariable(NameValidator.validateQueryParameterName(name), value); } } } diff --git a/engine/table/src/main/java/io/deephaven/engine/util/DelegatingScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/DelegatingScriptSession.java index f67afedb5b9..7f7e84e8771 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/DelegatingScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/DelegatingScriptSession.java @@ -3,7 +3,6 @@ */ package io.deephaven.engine.util; -import io.deephaven.engine.context.QueryScope; import io.deephaven.engine.liveness.LivenessReferent; import io.deephaven.engine.context.ExecutionContext; import org.jetbrains.annotations.NotNull; @@ -12,7 +11,6 @@ import java.lang.ref.WeakReference; import java.nio.file.Path; import java.util.HashSet; -import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.stream.Stream; @@ -63,17 +61,6 @@ public void observeScopeChanges() { delegate.observeScopeChanges(); } - @NotNull - @Override - public Object getVariable(String name) throws QueryScope.MissingVariableException { - return delegate.getVariable(name); - } - - @Override - public T getVariable(String name, T defaultValue) { - return delegate.getVariable(name, defaultValue); - } - @Override public VariableProvider getVariableProvider() { return delegate.getVariableProvider(); @@ -89,26 +76,6 @@ public Changes evaluateScript(Path scriptPath) { return contextualizeChanges(delegate.evaluateScript(scriptPath)); } - @Override - public Map getVariables() { - return delegate.getVariables(); - } - - @Override - public Set getVariableNames() { - return delegate.getVariableNames(); - } - - @Override - public boolean hasVariableName(String name) { - return delegate.hasVariableName(name); - } - - @Override - public void setVariable(String name, Object value) { - delegate.setVariable(name, value); - } - @Override public String scriptType() { return delegate.scriptType(); diff --git a/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java b/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java index c071ce0a9c7..ff60cf4b6e5 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java @@ -248,11 +248,6 @@ private String generateScriptName() { return script + "_" + (++counter) + ".groovy"; } - @Override - public QueryScope newQueryScope() { - return new SynchronizedScriptSessionQueryScope(this); - } - public static InputStream findScript(String relativePath) throws IOException { return new ScriptFinder(DEFAULT_SCRIPT_PATH).findScript(relativePath); } diff --git a/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java b/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java index 7c7a28c838e..d62454e2e43 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java @@ -13,7 +13,6 @@ import java.util.LinkedHashMap; import java.util.Map; import java.util.Set; -import java.util.function.Supplier; /** * ScriptSession implementation that simply allows variables to be exported. This is not intended for use in user @@ -38,11 +37,6 @@ public NoLanguageDeephavenSession(final UpdateGraph updateGraph, variables = new LinkedHashMap<>(); } - @Override - public QueryScope newQueryScope() { - return new SynchronizedScriptSessionQueryScope(this); - } - @NotNull @Override public Object getVariable(String name) throws QueryScope.MissingVariableException { diff --git a/engine/table/src/main/java/io/deephaven/engine/util/ScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/ScriptSession.java index 1b7dea829b1..6f20a1e7faf 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/ScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/ScriptSession.java @@ -7,43 +7,17 @@ import io.deephaven.engine.context.QueryScope; import io.deephaven.engine.liveness.LivenessNode; import io.deephaven.engine.liveness.ReleasableLivenessManager; -import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import java.nio.file.Path; import java.util.LinkedHashMap; import java.util.Map; -import java.util.Set; /** * Interface for interactive console script sessions. */ public interface ScriptSession extends ReleasableLivenessManager, LivenessNode { - /** - * Retrieve a variable from the script session's bindings. - *

- * Please use {@link ScriptSession#getVariable(String, Object)} if you expect the variable may not exist. - * - * @param name the variable to retrieve - * @return the variable - * @throws QueryScope.MissingVariableException if the variable does not exist - */ - @NotNull - Object getVariable(String name) throws QueryScope.MissingVariableException; - - /** - * Retrieve a variable from the script session's bindings. If the variable is not present, return defaultValue. - * - * If the variable is present, but is not of type (T), a ClassCastException may result. - * - * @param name the variable to retrieve - * @param defaultValue the value to use when no value is present in the session's scope - * @param the type of the variable - * @return the value of the variable, or defaultValue if not present - */ - T getVariable(String name, T defaultValue); - /** * A {@link VariableProvider} instance, for services like autocomplete which may want a limited "just the variables" * view of our session state. @@ -122,36 +96,6 @@ default Changes evaluateScript(String script) { */ Changes evaluateScript(Path scriptPath); - /** - * Retrieves all of the variables present in the session's scope (e.g., Groovy binding, Python globals()). - * - * @return an unmodifiable map with variable names as the keys, and the Objects as the result - */ - Map getVariables(); - - /** - * Retrieves all of the variable names present in the session's scope - * - * @return an unmodifiable set of variable names - */ - Set getVariableNames(); - - /** - * Check if the scope has the given variable name - * - * @param name the variable name - * @return True iff the scope has the given variable name - */ - boolean hasVariableName(String name); - - /** - * Inserts a value into the script's scope. - * - * @param name the variable name to set - * @param value the new value of the variable - */ - void setVariable(String name, @Nullable Object value); - /** * @return a textual description of this script session's language for use in messages. */ diff --git a/engine/table/src/main/java/io/deephaven/engine/util/VariableProvider.java b/engine/table/src/main/java/io/deephaven/engine/util/VariableProvider.java index 8a7bde003c4..adf3771e07c 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/VariableProvider.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/VariableProvider.java @@ -8,7 +8,7 @@ import java.util.Set; /** - * + * Provides access to the variables in a script session. */ public interface VariableProvider { Set getVariableNames(); @@ -18,4 +18,8 @@ public interface VariableProvider { T getVariable(String var, T defaultValue); TableDefinition getTableDefinition(String var); + + boolean hasVariableName(String name); + + void setVariable(String name, T value); } diff --git a/java-client/session-dagger/src/test/java/io/deephaven/client/ObjectServiceTest.java b/java-client/session-dagger/src/test/java/io/deephaven/client/ObjectServiceTest.java index 53a27e14a03..8da982379f9 100644 --- a/java-client/session-dagger/src/test/java/io/deephaven/client/ObjectServiceTest.java +++ b/java-client/session-dagger/src/test/java/io/deephaven/client/ObjectServiceTest.java @@ -94,7 +94,7 @@ public MyObjects(Table table, Figure customObject) { @Test public void fetchable() throws ExecutionException, InterruptedException, TimeoutException, InvalidProtocolBufferException, TableHandleException { - getScriptSession().setVariable("my_objects", myObjects()); + getScriptSession().getVariableProvider().setVariable("my_objects", myObjects()); final TypedTicket tt = new TypedTicket(MyObjectsObjectType.NAME, new ScopeId("my_objects")); try ( final Fetchable fetchable = session.fetchable(tt).get(5, TimeUnit.SECONDS); @@ -106,7 +106,7 @@ public void fetchable() throws ExecutionException, InterruptedException, Timeout @Test public void fetch() throws ExecutionException, InterruptedException, TimeoutException, InvalidProtocolBufferException, TableHandleException { - getScriptSession().setVariable("my_objects", myObjects()); + getScriptSession().getVariableProvider().setVariable("my_objects", myObjects()); final TypedTicket tt = new TypedTicket(MyObjectsObjectType.NAME, new ScopeId("my_objects")); try (final ServerData dataAndExports = session.fetch(tt).get(5, TimeUnit.SECONDS)) { checkMyObject(dataAndExports); @@ -116,8 +116,8 @@ public void fetch() throws ExecutionException, InterruptedException, TimeoutExce @Test public void bidirectional() throws InterruptedException, ExecutionException, TimeoutException { final ScriptSession scriptSession = getScriptSession(); - scriptSession.setVariable("my_echo", EchoObjectType.INSTANCE); - scriptSession.setVariable("my_objects", myObjects()); + scriptSession.getVariableProvider().setVariable("my_echo", EchoObjectType.INSTANCE); + scriptSession.getVariableProvider().setVariable("my_objects", myObjects()); final TypedTicket echo = new TypedTicket(EchoObjectType.NAME, new ScopeId("my_echo")); final TypedTicket myObjects = new TypedTicket(MyObjectsObjectType.NAME, new ScopeId("my_objects")); try ( @@ -132,8 +132,8 @@ public void bidirectional() throws InterruptedException, ExecutionException, Tim @Test public void messageStream() throws InterruptedException { final ScriptSession scriptSession = getScriptSession(); - scriptSession.setVariable("my_echo", EchoObjectType.INSTANCE); - scriptSession.setVariable("my_objects", myObjects()); + scriptSession.getVariableProvider().setVariable("my_echo", EchoObjectType.INSTANCE); + scriptSession.getVariableProvider().setVariable("my_objects", myObjects()); final TypedTicket echo = new TypedTicket(EchoObjectType.NAME, new ScopeId("my_echo")); final TypedTicket myObjects = new TypedTicket(MyObjectsObjectType.NAME, new ScopeId("my_objects")); final EchoHandler echoHandler = new EchoHandler(); diff --git a/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java b/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java index 3fec26cab7b..17246213590 100644 --- a/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java +++ b/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java @@ -282,7 +282,7 @@ public void bindTableToVariable( ScriptSession scriptSession = exportedConsole != null ? exportedConsole.get() : scriptSessionProvider.get(); Table table = exportedTable.get(); - scriptSession.setVariable(request.getVariableName(), table); + scriptSession.getVariableProvider().setVariable(request.getVariableName(), table); if (DynamicNode.notDynamicOrIsRefreshing(table)) { scriptSession.manage(table); } @@ -305,7 +305,7 @@ public StreamObserver autoCompleteStream( final ScriptSession scriptSession = scriptSessionProvider.get(); scriptSession.evaluateScript( "from deephaven_internal.auto_completer import jedi_settings ; jedi_settings.set_scope(globals())"); - settings[0] = (PyObject) scriptSession.getVariable("jedi_settings"); + settings[0] = scriptSession.getVariableProvider().getVariable("jedi_settings", null); } catch (Exception err) { log.error().append("Error trying to enable jedi autocomplete").append(err).endl(); } diff --git a/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java b/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java index 585b5534b67..c23bceb9ba4 100644 --- a/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java +++ b/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java @@ -6,6 +6,7 @@ import com.google.protobuf.ByteStringAccess; import com.google.rpc.Code; import io.deephaven.base.string.EncodingInfo; +import io.deephaven.engine.context.ExecutionContext; import io.deephaven.engine.context.QueryScope; import io.deephaven.engine.liveness.LivenessReferent; import io.deephaven.engine.table.Table; @@ -58,29 +59,28 @@ public SessionState.ExportObject flightInfoFor( // there is no mechanism to wait for a scope variable to resolve; require that the scope variable exists now final String scopeName = nameForDescriptor(descriptor, logId); - final ScriptSession gss = scriptSessionProvider.get(); - final Flight.FlightInfo flightInfo = - gss.getExecutionContext().getUpdateGraph().sharedLock().computeLocked(() -> { - Object scopeVar = gss.getVariable(scopeName, null); - if (scopeVar == null) { - throw Exceptions.statusRuntimeException(Code.NOT_FOUND, - "Could not resolve '" + logId + ": no variable exists with name '" + scopeName + "'"); - } - if (scopeVar instanceof Table) { - scopeVar = authorization.transform(scopeVar); - return TicketRouter.getFlightInfo((Table) scopeVar, descriptor, flightTicketForName(scopeName)); - } + Object scopeVar = scriptSessionProvider.get().getVariableProvider().getVariable(scopeName, null); + if (scopeVar == null) { + throw Exceptions.statusRuntimeException(Code.NOT_FOUND, + "Could not resolve '" + logId + ": no variable exists with name '" + scopeName + "'"); + } + if (!(scopeVar instanceof Table)) { + throw Exceptions.statusRuntimeException(Code.NOT_FOUND, + "Could not resolve '" + logId + "': no variable exists with name '" + scopeName + "'"); + } - throw Exceptions.statusRuntimeException(Code.NOT_FOUND, - "Could not resolve '" + logId + "': no variable exists with name '" + scopeName + "'"); - }); + Table transformed = authorization.transform((Table) scopeVar); + Flight.FlightInfo flightInfo = + TicketRouter.getFlightInfo(transformed, descriptor, flightTicketForName(scopeName)); return SessionState.wrapAsExport(flightInfo); } @Override public void forAllFlightInfo(@Nullable final SessionState session, final Consumer visitor) { - scriptSessionProvider.get().getVariables().forEach((varName, varObj) -> { + QueryScope queryScope = ExecutionContext.getContext().getQueryScope(); + queryScope.getParamNames().forEach((varName) -> { + Object varObj = queryScope.readParamValue(varName); if (varObj instanceof Table) { visitor.accept(TicketRouter.getFlightInfo((Table) varObj, descriptorForName(varName), flightTicketForName(varName))); @@ -104,15 +104,12 @@ private SessionState.ExportObject resolve( @Nullable final SessionState session, final String scopeName, final String logId) { final ScriptSession gss = scriptSessionProvider.get(); // fetch the variable from the scope right now - T export = gss.getExecutionContext().getUpdateGraph().sharedLock().computeLocked(() -> { - T scopeVar = null; - try { - // noinspection unchecked - scopeVar = (T) gss.unwrapObject(gss.getVariable(scopeName)); - } catch (QueryScope.MissingVariableException ignored) { - } - return scopeVar; - }); + T export = null; + try { + // noinspection unchecked + export = (T) gss.unwrapObject(gss.getVariableProvider().getVariable(scopeName, null)); + } catch (QueryScope.MissingVariableException ignored) { + } export = authorization.transform(export); @@ -162,7 +159,7 @@ private SessionState.ExportBuilder publish( if (value instanceof LivenessReferent && DynamicNode.notDynamicOrIsRefreshing(value)) { gss.manage((LivenessReferent) value); } - gss.setVariable(varName, value); + gss.getVariableProvider().setVariable(varName, value); if (onPublish != null) { onPublish.run(); } diff --git a/server/src/main/java/io/deephaven/server/console/completer/PythonAutoCompleteObserver.java b/server/src/main/java/io/deephaven/server/console/completer/PythonAutoCompleteObserver.java index 2a26905e474..93b55ffa477 100644 --- a/server/src/main/java/io/deephaven/server/console/completer/PythonAutoCompleteObserver.java +++ b/server/src/main/java/io/deephaven/server/console/completer/PythonAutoCompleteObserver.java @@ -49,7 +49,8 @@ public void onNext(AutoCompleteRequest value) { switch (value.getRequestCase()) { case OPEN_DOCUMENT: { final TextDocumentItem doc = value.getOpenDocument().getTextDocument(); - PyObject completer = (PyObject) scriptSession.get().getVariable("jedi_settings"); + PyObject completer = + (PyObject) scriptSession.get().getVariableProvider().getVariable("jedi_settings", null); completer.callMethod("open_doc", doc.getText(), doc.getUri(), doc.getVersion()); break; } @@ -57,7 +58,8 @@ public void onNext(AutoCompleteRequest value) { ChangeDocumentRequest request = value.getChangeDocument(); final VersionedTextDocumentIdentifier text = request.getTextDocument(); - PyObject completer = (PyObject) scriptSession.get().getVariable("jedi_settings"); + PyObject completer = + (PyObject) scriptSession.get().getVariableProvider().getVariable("jedi_settings", null); String uri = text.getUri(); int version = text.getVersion(); String document = completer.callMethod("get_doc", text.getUri()).getStringValue(); @@ -75,7 +77,8 @@ public void onNext(AutoCompleteRequest value) { break; } case CLOSE_DOCUMENT: { - PyObject completer = (PyObject) scriptSession.get().getVariable("jedi_settings"); + PyObject completer = + (PyObject) scriptSession.get().getVariableProvider().getVariable("jedi_settings", null); CloseDocumentRequest request = value.getCloseDocument(); completer.callMethod("close_doc", request.getTextDocument().getUri()); break; @@ -110,7 +113,7 @@ private void handleAutocompleteRequest(AutoCompleteRequest request, request.getRequestId() > 0 ? request.getRequestId() : request.getGetCompletionItems().getRequestId(); try { final ScriptSession scriptSession = exportedConsole.get(); - PyObject completer = (PyObject) scriptSession.getVariable("jedi_settings"); + PyObject completer = (PyObject) scriptSession.getVariableProvider().getVariable("jedi_settings", null); boolean canJedi = completer.callMethod("is_enabled").getBooleanValue(); if (!canJedi) { log.trace().append("Ignoring completion request because jedi is disabled").endl(); diff --git a/server/src/main/java/io/deephaven/server/uri/QueryScopeResolver.java b/server/src/main/java/io/deephaven/server/uri/QueryScopeResolver.java index 0a89d98b5c8..6b9839a753b 100644 --- a/server/src/main/java/io/deephaven/server/uri/QueryScopeResolver.java +++ b/server/src/main/java/io/deephaven/server/uri/QueryScopeResolver.java @@ -3,17 +3,15 @@ */ package io.deephaven.server.uri; -import io.deephaven.engine.util.ScriptSession; +import io.deephaven.engine.context.ExecutionContext; import io.deephaven.uri.DeephavenUri; import io.deephaven.uri.QueryScopeUri; import io.deephaven.uri.resolver.UriResolver; import io.deephaven.uri.resolver.UriResolversInstance; import javax.inject.Inject; -import javax.inject.Provider; import java.net.URI; import java.util.Collections; -import java.util.Objects; import java.util.Set; /** @@ -30,12 +28,8 @@ public static QueryScopeResolver get() { return UriResolversInstance.get().find(QueryScopeResolver.class).get(); } - private final Provider globalSessionProvider; - @Inject - public QueryScopeResolver(Provider globalSessionProvider) { - this.globalSessionProvider = Objects.requireNonNull(globalSessionProvider); - } + public QueryScopeResolver() {} @Override public Set schemes() { @@ -57,6 +51,6 @@ public Object resolve(QueryScopeUri uri) { } public Object resolve(String variableName) { - return globalSessionProvider.get().getVariable(variableName, null); + return ExecutionContext.getContext().getQueryScope().readParamValue(variableName, null); } } diff --git a/server/src/test/java/io/deephaven/server/appmode/ApplicationServiceGrpcImplTest.java b/server/src/test/java/io/deephaven/server/appmode/ApplicationServiceGrpcImplTest.java index b11c16fece8..35ba754d59f 100644 --- a/server/src/test/java/io/deephaven/server/appmode/ApplicationServiceGrpcImplTest.java +++ b/server/src/test/java/io/deephaven/server/appmode/ApplicationServiceGrpcImplTest.java @@ -89,7 +89,7 @@ public void onListFieldsSubscribeFailedObserver() { // trigger a change ScriptSession scriptSession = new NoLanguageDeephavenSession( ExecutionContext.getDefaultContext().getUpdateGraph(), ThreadInitializationFactory.NO_OP); - scriptSession.setVariable("key", "hello world"); + scriptSession.getVariableProvider().setVariable("key", "hello world"); ScriptSession.Changes changes = new ScriptSession.Changes(); changes.created.put("key", "Object"); applicationServiceGrpcImpl.onScopeChanges(scriptSession, changes); diff --git a/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java b/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java index 860eed84cb5..8003572998c 100644 --- a/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java +++ b/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java @@ -365,7 +365,7 @@ public void testLoginHandshakeBasicAuth() { .allocator(new RootAllocator()) .build(); - scriptSession.setVariable("test", TableTools.emptyTable(10).update("I=i")); + scriptSession.getVariableProvider().setVariable("test", TableTools.emptyTable(10).update("I=i")); // do get cannot be invoked by unauthenticated user final Ticket ticket = new Ticket("s/test".getBytes(StandardCharsets.UTF_8)); @@ -386,7 +386,7 @@ public void testLoginHeaderBasicAuth() { .allocator(new RootAllocator()) .build(); - scriptSession.setVariable("test", TableTools.emptyTable(10).update("I=i")); + scriptSession.getVariableProvider().setVariable("test", TableTools.emptyTable(10).update("I=i")); // do get cannot be invoked by unauthenticated user final Ticket ticket = new Ticket("s/test".getBytes(StandardCharsets.UTF_8)); @@ -407,7 +407,7 @@ public void testLoginHeaderBasicAuth() { @Test public void testLoginHeaderCustomBearer() { closeClient(); - scriptSession.setVariable("test", TableTools.emptyTable(10).update("I=i")); + scriptSession.getVariableProvider().setVariable("test", TableTools.emptyTable(10).update("I=i")); // add the bearer token override final MutableBoolean tokenChanged = new MutableBoolean(); @@ -447,7 +447,7 @@ public void testLoginHandshakeAnonymous() { .allocator(new RootAllocator()) .build(); - scriptSession.setVariable("test", TableTools.emptyTable(10).update("I=i")); + scriptSession.getVariableProvider().setVariable("test", TableTools.emptyTable(10).update("I=i")); // do get cannot be invoked by unauthenticated user final Ticket ticket = new Ticket("s/test".getBytes(StandardCharsets.UTF_8)); @@ -482,7 +482,7 @@ public void testLoginHeaderAnonymous() { final String ANONYMOUS = "Anonymous"; closeClient(); - scriptSession.setVariable("test", TableTools.emptyTable(10).update("I=i")); + scriptSession.getVariableProvider().setVariable("test", TableTools.emptyTable(10).update("I=i")); final MutableBoolean tokenChanged = new MutableBoolean(); flightClient = FlightClient.builder().location(serverLocation) @@ -651,8 +651,8 @@ public void testFlightInfo() { () -> TableTools.timeTable(1_000_000).update("I = i")); // stuff table into the scope - scriptSession.setVariable(staticTableName, table); - scriptSession.setVariable(tickingTableName, tickingTable); + scriptSession.getVariableProvider().setVariable(staticTableName, table); + scriptSession.getVariableProvider().setVariable(tickingTableName, tickingTable); // test fetch info from scoped ticket assertInfoMatchesTable(flightClient.getInfo(arrowFlightDescriptorForName(staticTableName)), table); @@ -683,8 +683,8 @@ public void testGetSchema() { try (final SafeCloseable ignored = LivenessScopeStack.open(scriptSession, false)) { // stuff table into the scope - scriptSession.setVariable(staticTableName, table); - scriptSession.setVariable(tickingTableName, tickingTable); + scriptSession.getVariableProvider().setVariable(staticTableName, table); + scriptSession.getVariableProvider().setVariable(tickingTableName, tickingTable); // test fetch info from scoped ticket assertSchemaMatchesTable(flightClient.getSchema(arrowFlightDescriptorForName(staticTableName)).getSchema(), @@ -714,7 +714,7 @@ public void testDoExchangeSnapshot() throws Exception { try (final SafeCloseable ignored = LivenessScopeStack.open(scriptSession, false)) { // stuff table into the scope - scriptSession.setVariable(staticTableName, table); + scriptSession.getVariableProvider().setVariable(staticTableName, table); // build up a snapshot request byte[] magic = new byte[] {100, 112, 104, 110}; // equivalent to '0x6E687064' (ASCII "dphn") @@ -790,7 +790,7 @@ public void testDoExchangeProtocol() throws Exception { try (final SafeCloseable ignored = LivenessScopeStack.open(scriptSession, false)) { // stuff table into the scope - scriptSession.setVariable(staticTableName, table); + scriptSession.getVariableProvider().setVariable(staticTableName, table); // java-flight requires us to send a message, but cannot add app metadata, send a dummy message byte[] empty = new byte[] {}; @@ -837,7 +837,7 @@ public T transform(T source) { } }; - scriptSession.setVariable(tableName, table); + scriptSession.getVariableProvider().setVariable(tableName, table); // export from query scope to our session; this transforms the table assertEquals(0, numTransforms.intValue()); @@ -849,7 +849,7 @@ public T transform(T source) { assertEquals(1, numTransforms.intValue()); // check that the table was transformed - Object result = scriptSession.getVariable(resultTableName); + Object result = scriptSession.getVariableProvider().getVariable(resultTableName, null); assertTrue(result instanceof Table); assertEquals(1, ((Table) result).getColumnSources().size()); assertEquals(2, table.getColumnSources().size()); @@ -861,7 +861,7 @@ public void testSimpleServiceAuthWiring() throws Exception { final String tableName = "testSimpleServiceAuthWiringTest"; final String resultTableName = tableName + "Result"; final Table table = TableTools.emptyTable(10).update("I = -i", "J = -i"); - scriptSession.setVariable(tableName, table); + scriptSession.getVariableProvider().setVariable(tableName, table); // export from query scope to our session; this transforms the table try (final TableHandle handle = clientSession.session().execute(TicketTable.fromQueryScopeField(tableName))) { @@ -894,7 +894,7 @@ public void testSimpleContextualAuthWiring() throws Exception { // stuff table into the scope final String tableName = "testSimpleContextualAuthWiringTest"; final Table table = TableTools.emptyTable(10).update("I = -i", "J = -i"); - scriptSession.setVariable(tableName, table); + scriptSession.getVariableProvider().setVariable(tableName, table); // export from query scope to our session; this transforms the table try (final TableHandle handle = clientSession.session().execute(TicketTable.fromQueryScopeField(tableName))) { @@ -1017,7 +1017,7 @@ private void assertRoundTripDataEqual(Table deephavenTable) throws Exception { public void testBarrageMessageAppendingMarshaller() { final int size = 100; final Table source = TableTools.emptyTable(size).update("I = ii", "J = `str_` + i"); - scriptSession.setVariable("test", source); + scriptSession.getVariableProvider().setVariable("test", source); // fetch schema over flight final SchemaResult schema = flightClient.getSchema(arrowFlightDescriptorForName("test")); @@ -1068,7 +1068,7 @@ public void testColumnsAsListFeature() throws Exception { final Table appendOnly = TableTools.timeTable("PT1s") .update("I = ii % 3", "J = `str_` + i"); final Table withMods = appendOnly.lastBy("I"); - scriptSession.setVariable("test", withMods); + scriptSession.getVariableProvider().setVariable("test", withMods); } final BarrageSubscriptionOptions options = BarrageSubscriptionOptions.builder() From 4e2beee8becf1834bbb15f2478b0a872f5dd7843 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Wed, 20 Dec 2023 15:28:44 -0600 Subject: [PATCH 02/23] Further shift from script session to query scope, including liveness --- .../engine/context/EmptyQueryScope.java | 5 --- .../engine/context/PoisonedQueryScope.java | 5 --- .../deephaven/engine/context/QueryScope.java | 34 +++++++-------- .../engine/util/AbstractScriptSession.java | 34 ++++++++------- .../engine/util/DelegatingScriptSession.java | 5 --- .../deephaven/engine/util/ScriptSession.java | 2 +- .../console/ConsoleServiceGrpcImpl.java | 3 +- .../server/console/ScopeTicketResolver.java | 24 ++++------- .../server/appmode/ApplicationTest.java | 10 +---- .../test/FlightMessageRoundTripTest.java | 41 +++++++++---------- 10 files changed, 64 insertions(+), 99 deletions(-) diff --git a/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java index f382e9b7d5f..368a7d38926 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java @@ -40,9 +40,4 @@ public T readParamValue(String name, T defaultValue) { public void putParam(String name, T value) { throw new IllegalStateException("EmptyQueryScope cannot create parameters"); } - - @Override - public void putObjectFields(Object object) { - throw new IllegalStateException("EmptyQueryScope cannot create parameters"); - } } diff --git a/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java index dda0ad0a035..66cefae2452 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java @@ -46,9 +46,4 @@ public T readParamValue(String name, T defaultValue) { public void putParam(String name, T value) { fail(); } - - @Override - public void putObjectFields(Object object) { - fail(); - } } diff --git a/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java index cccf14b22bd..21370076c8c 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java @@ -3,6 +3,9 @@ */ package io.deephaven.engine.context; +import io.deephaven.engine.liveness.LivenessReferent; +import io.deephaven.engine.liveness.LivenessScope; +import io.deephaven.engine.updategraph.DynamicNode; import io.deephaven.time.DateTimeUtils; import io.deephaven.hash.KeyedObjectHashMap; import io.deephaven.hash.KeyedObjectKey; @@ -21,7 +24,7 @@ /** * Variable scope used to resolve parameter values during query execution. */ -public abstract class QueryScope implements LogOutputAppendable { +public abstract class QueryScope extends LivenessScope implements LogOutputAppendable { /** * Adds a parameter to the default instance {@link QueryScope}, or updates the value of an existing parameter. @@ -34,15 +37,6 @@ public static void addParam(final String name, final T value) { ExecutionContext.getContext().getQueryScope().putParam(name, value); } - /** - * Adds an object's declared fields to the scope. - * - * @param object object whose fields will be added. - */ - public static void addObjectFields(final Object object) { - ExecutionContext.getContext().getQueryScope().putObjectFields(object); - } - /** * Gets a parameter from the default instance {@link QueryScope}. * @@ -194,12 +188,15 @@ public final QueryScopeParam[] getParams(final Collection names) throws public abstract void putParam(final String name, final T value); /** - * Add an object's public members (referenced reflectively, not a shallow copy!) to this scope if supported. - * Note: This is an optional method. + * Asks the session to remove any wrapping that exists on scoped objects so that clients can fetch them. Defaults to + * returning the object itself. * - * @param object object to add public members from. + * @param object the scoped object + * @return an obj which can be consumed by a client */ - public abstract void putObjectFields(final Object object); + public Object unwrapObject(Object object) { + return object; + } // ----------------------------------------------------------------------------------------------------------------- // LogOutputAppendable implementation @@ -276,17 +273,14 @@ public T readParamValue(final String name, final T defaultValue) { @Override public void putParam(final String name, final T value) { + if (value instanceof LivenessReferent && DynamicNode.notDynamicOrIsRefreshing(value)) { + manage((LivenessReferent) value); + } NameValidator.validateQueryParameterName(name); // TODO: Can I get rid of this applyValueConversions? It's too inconsistent to feel safe. valueRetrievers.put(name, new SimpleValueRetriever<>(name, applyValueConversions(value))); } - public void putObjectFields(final Object object) { - for (final Field field : object.getClass().getDeclaredFields()) { - valueRetrievers.put(field.getName(), new ReflectiveValueRetriever(object, field)); - } - } - private static abstract class ValueRetriever { private final String name; diff --git a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java index 8bbb4945bcc..842f0b747b7 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java @@ -10,6 +10,8 @@ import io.deephaven.configuration.CacheDir; import io.deephaven.engine.context.QueryCompiler; import io.deephaven.engine.context.ExecutionContext; +import io.deephaven.engine.liveness.LivenessArtifact; +import io.deephaven.engine.liveness.LivenessReferent; import io.deephaven.engine.liveness.LivenessScope; import io.deephaven.engine.liveness.LivenessScopeStack; import io.deephaven.engine.table.PartitionedTable; @@ -19,6 +21,7 @@ import io.deephaven.engine.context.QueryScopeParam; import io.deephaven.engine.table.hierarchical.HierarchicalTable; import io.deephaven.engine.table.impl.OperationInitializationThreadPool; +import io.deephaven.engine.updategraph.DynamicNode; import io.deephaven.engine.updategraph.UpdateGraph; import io.deephaven.plugin.type.ObjectType; import io.deephaven.plugin.type.ObjectTypeLookup; @@ -44,7 +47,7 @@ * This class exists to make all script sessions to be liveness artifacts, and provide a default implementation for * evaluateScript which handles liveness and diffs in a consistent way. */ -public abstract class AbstractScriptSession extends LivenessScope +public abstract class AbstractScriptSession extends LivenessArtifact implements ScriptSession { private static final Path CLASS_CACHE_LOCATION = CacheDir.get().resolve("script-session-classes"); @@ -74,6 +77,7 @@ private static void createOrClearDirectory(final File directory) { private final ReadWriteLock variableAccessLock = new ReentrantReadWriteLock(); private S lastSnapshot; + private final QueryScope queryScope; protected AbstractScriptSession( UpdateGraph updateGraph, @@ -88,7 +92,8 @@ protected AbstractScriptSession( classCacheDirectory = CLASS_CACHE_LOCATION.resolve(UuidCreator.toString(scriptCacheId)).toFile(); createOrClearDirectory(classCacheDirectory); - final QueryScope queryScope = newQueryScope(); + queryScope = new ScriptSessionQueryScope(); + manage(queryScope); final QueryCompiler compilerContext = QueryCompiler.create(classCacheDirectory, Thread.currentThread().getContextClassLoader()); @@ -148,7 +153,7 @@ public synchronized final Changes evaluateScript(final String script, @Nullable // closes variableAccessLock.writeLock().lock(); try (final S initialSnapshot = takeSnapshot(); - final SafeCloseable ignored = LivenessScopeStack.open(this, false)) { + final SafeCloseable ignored = LivenessScopeStack.open(queryScope, false)) { try { // Actually evaluate the script; use the enclosing auth context, since AbstractScriptSession's @@ -253,10 +258,10 @@ protected void destroy() { protected abstract void evaluate(String command, @Nullable String scriptName); /** - * @return a query scope for this session; only invoked during construction + * @return a query scope that wraps */ - protected QueryScope newQueryScope() { - return new ScriptSessionQueryScope(); + public QueryScope getQueryScope() { + return queryScope; } private Class getVariableType(final String var) { @@ -401,15 +406,6 @@ public void setVariable(String name, T value) { // ----------------------------------------------------------------------------------------------------------------- public class ScriptSessionQueryScope extends QueryScope { - @Override - public void putObjectFields(Object object) { - throw new UnsupportedOperationException(); - } - - public ScriptSession scriptSession() { - return AbstractScriptSession.this; - } - @Override public Set getParamNames() { final Set result = new LinkedHashSet<>(); @@ -455,7 +451,15 @@ public T readParamValue(final String name, final T defaultValue) { @Override public void putParam(final String name, final T value) { + if (value instanceof LivenessReferent && DynamicNode.notDynamicOrIsRefreshing(value)) { + manage((LivenessReferent) value); + } getVariableProvider().setVariable(NameValidator.validateQueryParameterName(name), value); } + + @Override + public Object unwrapObject(Object object) { + return AbstractScriptSession.this.unwrapObject(object); + } } } diff --git a/engine/table/src/main/java/io/deephaven/engine/util/DelegatingScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/DelegatingScriptSession.java index 7f7e84e8771..590bc838ed4 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/DelegatingScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/DelegatingScriptSession.java @@ -110,9 +110,4 @@ public void dropReference() { public WeakReference getWeakReference() { return delegate.getWeakReference(); } - - @Override - public void release() { - delegate.release(); - } } diff --git a/engine/table/src/main/java/io/deephaven/engine/util/ScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/ScriptSession.java index 6f20a1e7faf..09cc8e89726 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/ScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/ScriptSession.java @@ -16,7 +16,7 @@ /** * Interface for interactive console script sessions. */ -public interface ScriptSession extends ReleasableLivenessManager, LivenessNode { +public interface ScriptSession extends LivenessNode { /** * A {@link VariableProvider} instance, for services like autocomplete which may want a limited "just the variables" diff --git a/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java b/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java index 17246213590..df5cb3a84c3 100644 --- a/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java +++ b/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java @@ -7,6 +7,7 @@ import com.google.rpc.Code; import io.deephaven.base.LockFreeArrayQueue; import io.deephaven.configuration.Configuration; +import io.deephaven.engine.context.ExecutionContext; import io.deephaven.engine.table.Table; import io.deephaven.engine.table.impl.perf.QueryPerformanceNugget; import io.deephaven.engine.table.impl.perf.QueryPerformanceRecorder; @@ -282,7 +283,7 @@ public void bindTableToVariable( ScriptSession scriptSession = exportedConsole != null ? exportedConsole.get() : scriptSessionProvider.get(); Table table = exportedTable.get(); - scriptSession.getVariableProvider().setVariable(request.getVariableName(), table); + ExecutionContext.getContext().getQueryScope().putParam(request.getVariableName(), table); if (DynamicNode.notDynamicOrIsRefreshing(table)) { scriptSession.manage(table); } diff --git a/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java b/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java index c23bceb9ba4..2c61b2cb4cf 100644 --- a/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java +++ b/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java @@ -8,10 +8,7 @@ import io.deephaven.base.string.EncodingInfo; import io.deephaven.engine.context.ExecutionContext; import io.deephaven.engine.context.QueryScope; -import io.deephaven.engine.liveness.LivenessReferent; import io.deephaven.engine.table.Table; -import io.deephaven.engine.updategraph.DynamicNode; -import io.deephaven.engine.util.ScriptSession; import io.deephaven.proto.backplane.grpc.Ticket; import io.deephaven.proto.flight.util.TicketRouterHelper; import io.deephaven.proto.util.ByteHelper; @@ -25,7 +22,6 @@ import org.jetbrains.annotations.Nullable; import javax.inject.Inject; -import javax.inject.Provider; import javax.inject.Singleton; import java.nio.ByteBuffer; import java.nio.charset.CharacterCodingException; @@ -38,14 +34,10 @@ @Singleton public class ScopeTicketResolver extends TicketResolverBase { - private final Provider scriptSessionProvider; - @Inject public ScopeTicketResolver( - final AuthorizationProvider authProvider, - final Provider globalSessionProvider) { + final AuthorizationProvider authProvider) { super(authProvider, (byte) TICKET_PREFIX, FLIGHT_DESCRIPTOR_ROUTE); - this.scriptSessionProvider = globalSessionProvider; } @Override @@ -59,7 +51,8 @@ public SessionState.ExportObject flightInfoFor( // there is no mechanism to wait for a scope variable to resolve; require that the scope variable exists now final String scopeName = nameForDescriptor(descriptor, logId); - Object scopeVar = scriptSessionProvider.get().getVariableProvider().getVariable(scopeName, null); + QueryScope queryScope = ExecutionContext.getContext().getQueryScope(); + Object scopeVar = queryScope.readParamValue(scopeName, null); if (scopeVar == null) { throw Exceptions.statusRuntimeException(Code.NOT_FOUND, "Could not resolve '" + logId + ": no variable exists with name '" + scopeName + "'"); @@ -102,12 +95,12 @@ public SessionState.ExportObject resolve( private SessionState.ExportObject resolve( @Nullable final SessionState session, final String scopeName, final String logId) { - final ScriptSession gss = scriptSessionProvider.get(); // fetch the variable from the scope right now T export = null; try { + QueryScope queryScope = ExecutionContext.getContext().getQueryScope(); // noinspection unchecked - export = (T) gss.unwrapObject(gss.getVariableProvider().getVariable(scopeName, null)); + export = (T) queryScope.unwrapObject(queryScope.readParamValue(scopeName)); } catch (QueryScope.MissingVariableException ignored) { } @@ -154,12 +147,9 @@ private SessionState.ExportBuilder publish( .requiresSerialQueue() .require(resultExport) .submit(() -> { - final ScriptSession gss = scriptSessionProvider.get(); T value = resultExport.get(); - if (value instanceof LivenessReferent && DynamicNode.notDynamicOrIsRefreshing(value)) { - gss.manage((LivenessReferent) value); - } - gss.getVariableProvider().setVariable(varName, value); + ExecutionContext.getContext().getQueryScope().putParam(varName, value); + if (onPublish != null) { onPublish.run(); } diff --git a/server/src/test/java/io/deephaven/server/appmode/ApplicationTest.java b/server/src/test/java/io/deephaven/server/appmode/ApplicationTest.java index 8214cd3e313..73e40a82107 100644 --- a/server/src/test/java/io/deephaven/server/appmode/ApplicationTest.java +++ b/server/src/test/java/io/deephaven/server/appmode/ApplicationTest.java @@ -29,15 +29,7 @@ public class ApplicationTest { @Rule public final EngineCleanup base = new EngineCleanup(); - private AbstractScriptSession session = null; - - @After - public void tearDown() { - if (session != null) { - session.release(); - session = null; - } - } + private AbstractScriptSession session = null; @Test public void app00() { diff --git a/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java b/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java index 8003572998c..f9716a31d93 100644 --- a/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java +++ b/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java @@ -302,7 +302,6 @@ public void teardown() throws InterruptedException { clientChannel.shutdownNow(); sessionService.closeAllSessions(); - scriptSession.release(); executionContext.close(); closeClient(); @@ -365,7 +364,7 @@ public void testLoginHandshakeBasicAuth() { .allocator(new RootAllocator()) .build(); - scriptSession.getVariableProvider().setVariable("test", TableTools.emptyTable(10).update("I=i")); + ExecutionContext.getContext().getQueryScope().putParam("test", TableTools.emptyTable(10).update("I=i")); // do get cannot be invoked by unauthenticated user final Ticket ticket = new Ticket("s/test".getBytes(StandardCharsets.UTF_8)); @@ -386,7 +385,7 @@ public void testLoginHeaderBasicAuth() { .allocator(new RootAllocator()) .build(); - scriptSession.getVariableProvider().setVariable("test", TableTools.emptyTable(10).update("I=i")); + ExecutionContext.getContext().getQueryScope().putParam("test", TableTools.emptyTable(10).update("I=i")); // do get cannot be invoked by unauthenticated user final Ticket ticket = new Ticket("s/test".getBytes(StandardCharsets.UTF_8)); @@ -407,7 +406,7 @@ public void testLoginHeaderBasicAuth() { @Test public void testLoginHeaderCustomBearer() { closeClient(); - scriptSession.getVariableProvider().setVariable("test", TableTools.emptyTable(10).update("I=i")); + ExecutionContext.getContext().getQueryScope().putParam("test", TableTools.emptyTable(10).update("I=i")); // add the bearer token override final MutableBoolean tokenChanged = new MutableBoolean(); @@ -447,7 +446,7 @@ public void testLoginHandshakeAnonymous() { .allocator(new RootAllocator()) .build(); - scriptSession.getVariableProvider().setVariable("test", TableTools.emptyTable(10).update("I=i")); + ExecutionContext.getContext().getQueryScope().putParam("test", TableTools.emptyTable(10).update("I=i")); // do get cannot be invoked by unauthenticated user final Ticket ticket = new Ticket("s/test".getBytes(StandardCharsets.UTF_8)); @@ -651,8 +650,8 @@ public void testFlightInfo() { () -> TableTools.timeTable(1_000_000).update("I = i")); // stuff table into the scope - scriptSession.getVariableProvider().setVariable(staticTableName, table); - scriptSession.getVariableProvider().setVariable(tickingTableName, tickingTable); + ExecutionContext.getContext().getQueryScope().putParam(staticTableName, table); + ExecutionContext.getContext().getQueryScope().putParam(tickingTableName, tickingTable); // test fetch info from scoped ticket assertInfoMatchesTable(flightClient.getInfo(arrowFlightDescriptorForName(staticTableName)), table); @@ -681,10 +680,10 @@ public void testGetSchema() { final Table tickingTable = ExecutionContext.getContext().getUpdateGraph().sharedLock().computeLocked( () -> TableTools.timeTable(1_000_000).update("I = i")); - try (final SafeCloseable ignored = LivenessScopeStack.open(scriptSession, false)) { + try (final SafeCloseable ignored = LivenessScopeStack.open(scriptSession.getQueryScope(), false)) { // stuff table into the scope - scriptSession.getVariableProvider().setVariable(staticTableName, table); - scriptSession.getVariableProvider().setVariable(tickingTableName, tickingTable); + ExecutionContext.getContext().getQueryScope().putParam(staticTableName, table); + ExecutionContext.getContext().getQueryScope().putParam(tickingTableName, tickingTable); // test fetch info from scoped ticket assertSchemaMatchesTable(flightClient.getSchema(arrowFlightDescriptorForName(staticTableName)).getSchema(), @@ -712,9 +711,9 @@ public void testDoExchangeSnapshot() throws Exception { final String staticTableName = "flightInfoTest"; final Table table = TableTools.emptyTable(10).update("I = i", "J = i + 0.01"); - try (final SafeCloseable ignored = LivenessScopeStack.open(scriptSession, false)) { + try (final SafeCloseable ignored = LivenessScopeStack.open(scriptSession.getQueryScope(), false)) { // stuff table into the scope - scriptSession.getVariableProvider().setVariable(staticTableName, table); + ExecutionContext.getContext().getQueryScope().putParam(staticTableName, table); // build up a snapshot request byte[] magic = new byte[] {100, 112, 104, 110}; // equivalent to '0x6E687064' (ASCII "dphn") @@ -788,9 +787,9 @@ public void testDoExchangeProtocol() throws Exception { final String staticTableName = "flightInfoTest"; final Table table = TableTools.emptyTable(10).update("I = i", "J = i + 0.01"); - try (final SafeCloseable ignored = LivenessScopeStack.open(scriptSession, false)) { + try (final SafeCloseable ignored = LivenessScopeStack.open(scriptSession.getQueryScope(), false)) { // stuff table into the scope - scriptSession.getVariableProvider().setVariable(staticTableName, table); + ExecutionContext.getContext().getQueryScope().putParam(staticTableName, table); // java-flight requires us to send a message, but cannot add app metadata, send a dummy message byte[] empty = new byte[] {}; @@ -837,7 +836,7 @@ public T transform(T source) { } }; - scriptSession.getVariableProvider().setVariable(tableName, table); + ExecutionContext.getContext().getQueryScope().putParam(tableName, table); // export from query scope to our session; this transforms the table assertEquals(0, numTransforms.intValue()); @@ -849,8 +848,8 @@ public T transform(T source) { assertEquals(1, numTransforms.intValue()); // check that the table was transformed - Object result = scriptSession.getVariableProvider().getVariable(resultTableName, null); - assertTrue(result instanceof Table); + Object result = ExecutionContext.getContext().getQueryScope().readParamValue(resultTableName, null); + assertTrue(result + "", result instanceof Table); assertEquals(1, ((Table) result).getColumnSources().size()); assertEquals(2, table.getColumnSources().size()); } @@ -861,7 +860,7 @@ public void testSimpleServiceAuthWiring() throws Exception { final String tableName = "testSimpleServiceAuthWiringTest"; final String resultTableName = tableName + "Result"; final Table table = TableTools.emptyTable(10).update("I = -i", "J = -i"); - scriptSession.getVariableProvider().setVariable(tableName, table); + ExecutionContext.getContext().getQueryScope().putParam(tableName, table); // export from query scope to our session; this transforms the table try (final TableHandle handle = clientSession.session().execute(TicketTable.fromQueryScopeField(tableName))) { @@ -894,7 +893,7 @@ public void testSimpleContextualAuthWiring() throws Exception { // stuff table into the scope final String tableName = "testSimpleContextualAuthWiringTest"; final Table table = TableTools.emptyTable(10).update("I = -i", "J = -i"); - scriptSession.getVariableProvider().setVariable(tableName, table); + ExecutionContext.getContext().getQueryScope().putParam(tableName, table); // export from query scope to our session; this transforms the table try (final TableHandle handle = clientSession.session().execute(TicketTable.fromQueryScopeField(tableName))) { @@ -1017,7 +1016,7 @@ private void assertRoundTripDataEqual(Table deephavenTable) throws Exception { public void testBarrageMessageAppendingMarshaller() { final int size = 100; final Table source = TableTools.emptyTable(size).update("I = ii", "J = `str_` + i"); - scriptSession.getVariableProvider().setVariable("test", source); + ExecutionContext.getContext().getQueryScope().putParam("test", source); // fetch schema over flight final SchemaResult schema = flightClient.getSchema(arrowFlightDescriptorForName("test")); @@ -1068,7 +1067,7 @@ public void testColumnsAsListFeature() throws Exception { final Table appendOnly = TableTools.timeTable("PT1s") .update("I = ii % 3", "J = `str_` + i"); final Table withMods = appendOnly.lastBy("I"); - scriptSession.getVariableProvider().setVariable("test", withMods); + ExecutionContext.getContext().getQueryScope().putParam("test", withMods); } final BarrageSubscriptionOptions options = BarrageSubscriptionOptions.builder() From 7041746598dbba9d3c25743e22aa37f958749960 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Wed, 20 Dec 2023 16:32:47 -0600 Subject: [PATCH 03/23] Restore method needed for python local scope hack --- .../java/io/deephaven/engine/util/AbstractScriptSession.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java index 842f0b747b7..8d142b15820 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java @@ -406,6 +406,10 @@ public void setVariable(String name, T value) { // ----------------------------------------------------------------------------------------------------------------- public class ScriptSessionQueryScope extends QueryScope { + public ScriptSession scriptSession() { + return AbstractScriptSession.this; + } + @Override public Set getParamNames() { final Set result = new LinkedHashSet<>(); From 06ce7f077104e5e60cdde9df25b5572131e260b2 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Wed, 20 Dec 2023 19:07:35 -0600 Subject: [PATCH 04/23] Fix test setup, correct unwrapping in sql --- .../main/java/io/deephaven/engine/sql/Sql.java | 2 +- .../io/deephaven/client/ObjectServiceTest.java | 15 +++++++-------- .../server/runner/DeephavenApiServerTestBase.java | 4 ++-- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java b/engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java index 23657503c64..e59add6bc08 100644 --- a/engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java +++ b/engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java @@ -84,7 +84,7 @@ private static Map currentScriptSessionNamedTables() { // See SQLTODO(catalog-reader-implementation) QueryScope queryScope = ExecutionContext.getContext().getQueryScope(); for (String name : queryScope.getParamNames()) { - Object paramValue = queryScope.readParamValue(name); + Object paramValue = queryScope.unwrapObject(queryScope.readParamValue(name)); if (paramValue instanceof Table) { scope.put(name, (Table) paramValue); } diff --git a/java-client/session-dagger/src/test/java/io/deephaven/client/ObjectServiceTest.java b/java-client/session-dagger/src/test/java/io/deephaven/client/ObjectServiceTest.java index 8da982379f9..03a3401b840 100644 --- a/java-client/session-dagger/src/test/java/io/deephaven/client/ObjectServiceTest.java +++ b/java-client/session-dagger/src/test/java/io/deephaven/client/ObjectServiceTest.java @@ -16,6 +16,7 @@ import io.deephaven.client.impl.TableObject; import io.deephaven.client.impl.TypedTicket; import io.deephaven.client.impl.UnknownObject; +import io.deephaven.engine.context.ExecutionContext; import io.deephaven.engine.table.Table; import io.deephaven.engine.util.ScriptSession; import io.deephaven.engine.util.TableTools; @@ -94,7 +95,7 @@ public MyObjects(Table table, Figure customObject) { @Test public void fetchable() throws ExecutionException, InterruptedException, TimeoutException, InvalidProtocolBufferException, TableHandleException { - getScriptSession().getVariableProvider().setVariable("my_objects", myObjects()); + ExecutionContext.getContext().getQueryScope().putParam("my_objects", myObjects()); final TypedTicket tt = new TypedTicket(MyObjectsObjectType.NAME, new ScopeId("my_objects")); try ( final Fetchable fetchable = session.fetchable(tt).get(5, TimeUnit.SECONDS); @@ -106,7 +107,7 @@ public void fetchable() throws ExecutionException, InterruptedException, Timeout @Test public void fetch() throws ExecutionException, InterruptedException, TimeoutException, InvalidProtocolBufferException, TableHandleException { - getScriptSession().getVariableProvider().setVariable("my_objects", myObjects()); + ExecutionContext.getContext().getQueryScope().putParam("my_objects", myObjects()); final TypedTicket tt = new TypedTicket(MyObjectsObjectType.NAME, new ScopeId("my_objects")); try (final ServerData dataAndExports = session.fetch(tt).get(5, TimeUnit.SECONDS)) { checkMyObject(dataAndExports); @@ -115,9 +116,8 @@ public void fetch() throws ExecutionException, InterruptedException, TimeoutExce @Test public void bidirectional() throws InterruptedException, ExecutionException, TimeoutException { - final ScriptSession scriptSession = getScriptSession(); - scriptSession.getVariableProvider().setVariable("my_echo", EchoObjectType.INSTANCE); - scriptSession.getVariableProvider().setVariable("my_objects", myObjects()); + ExecutionContext.getContext().getQueryScope().putParam("my_echo", EchoObjectType.INSTANCE); + ExecutionContext.getContext().getQueryScope().putParam("my_objects", myObjects()); final TypedTicket echo = new TypedTicket(EchoObjectType.NAME, new ScopeId("my_echo")); final TypedTicket myObjects = new TypedTicket(MyObjectsObjectType.NAME, new ScopeId("my_objects")); try ( @@ -131,9 +131,8 @@ public void bidirectional() throws InterruptedException, ExecutionException, Tim @Test public void messageStream() throws InterruptedException { - final ScriptSession scriptSession = getScriptSession(); - scriptSession.getVariableProvider().setVariable("my_echo", EchoObjectType.INSTANCE); - scriptSession.getVariableProvider().setVariable("my_objects", myObjects()); + ExecutionContext.getContext().getQueryScope().putParam("my_echo", EchoObjectType.INSTANCE); + ExecutionContext.getContext().getQueryScope().putParam("my_objects", myObjects()); final TypedTicket echo = new TypedTicket(EchoObjectType.NAME, new ScopeId("my_echo")); final TypedTicket myObjects = new TypedTicket(MyObjectsObjectType.NAME, new ScopeId("my_objects")); final EchoHandler echoHandler = new EchoHandler(); diff --git a/server/src/test/java/io/deephaven/server/runner/DeephavenApiServerTestBase.java b/server/src/test/java/io/deephaven/server/runner/DeephavenApiServerTestBase.java index a064a1ef415..68bc260d813 100644 --- a/server/src/test/java/io/deephaven/server/runner/DeephavenApiServerTestBase.java +++ b/server/src/test/java/io/deephaven/server/runner/DeephavenApiServerTestBase.java @@ -86,7 +86,8 @@ interface Builder { @Rule public final GrpcCleanupRule grpcCleanup = new GrpcCleanupRule(); - private ExecutionContext executionContext; + @Inject + ExecutionContext executionContext; private SafeCloseable executionContextCloseable; private LogBuffer logBuffer; @@ -129,7 +130,6 @@ public void setUp() throws Exception { .injectFields(this); final PeriodicUpdateGraph updateGraph = server.getUpdateGraph().cast(); - executionContext = TestExecutionContext.createForUnitTests().withUpdateGraph(updateGraph); executionContextCloseable = executionContext.open(); if (updateGraph.isUnitTestModeAllowed()) { updateGraph.enableUnitTestMode(); From 7b57fa217d8ad008b8807f0e0b86b85e04aff241 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Thu, 21 Dec 2023 10:54:27 -0600 Subject: [PATCH 05/23] Partial review feedback --- .../python/PythonDeephavenSession.java | 27 +++------- .../table/impl/lang/QueryLanguageParser.java | 4 +- .../engine/util/AbstractScriptSession.java | 53 ++++++++----------- .../engine/util/DelegatingScriptSession.java | 6 +++ .../engine/util/GroovyDeephavenSession.java | 32 +++-------- .../util/NoLanguageDeephavenSession.java | 34 ++++-------- .../deephaven/engine/util/ScriptSession.java | 6 +++ .../engine/table/impl/FuzzerTest.java | 8 +-- .../scripts/TestGroovyDeephavenSession.java | 34 ++++++------ .../console/ConsoleServiceGrpcImpl.java | 11 ++-- .../completer/PythonAutoCompleteObserver.java | 4 +- .../server/appmode/ApplicationTest.java | 8 +++ .../test/FlightMessageRoundTripTest.java | 46 ++++++++-------- 13 files changed, 118 insertions(+), 155 deletions(-) diff --git a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java index 9b6ba40e9ab..f3a97999a17 100644 --- a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java +++ b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java @@ -169,20 +169,14 @@ private void runScript(String script) throws IOException { } } - @NotNull + @SuppressWarnings("unchecked") @Override - public Object getVariable(String name) throws QueryScope.MissingVariableException { - return scope + protected T getVariable(String name) throws QueryScope.MissingVariableException { + return (T) scope .getValue(name) .orElseThrow(() -> new QueryScope.MissingVariableException("No variable for: " + name)); } - @Override - public T getVariable(String name, T defaultValue) { - return scope - .getValueUnchecked(name) - .orElse(defaultValue); - } @SuppressWarnings("unused") @ScriptApi @@ -210,13 +204,6 @@ protected void evaluate(String command, String scriptName) { } } - @Override - public Map getVariables() { - final Map outMap = new LinkedHashMap<>(); - scope.getEntriesMap().forEach((key, value) -> outMap.put(key, maybeUnwrap(value))); - return outMap; - } - protected static class PythonSnapshot implements Snapshot, SafeCloseable { private final PyDictWrapper dict; @@ -281,17 +268,17 @@ private Object maybeUnwrap(PyObject o) { } @Override - public Set getVariableNames() { - return Collections.unmodifiableSet(scope.getKeys().collect(Collectors.toSet())); + protected Set getVariableNames() { + return scope.getKeys().collect(Collectors.toUnmodifiableSet()); } @Override - public boolean hasVariableName(String name) { + protected boolean hasVariableName(String name) { return scope.containsKey(name); } @Override - public synchronized void setVariable(String name, @Nullable Object newValue) { + protected synchronized void setVariable(String name, @Nullable Object newValue) { final PyDictWrapper globals = scope.mainGlobals(); if (newValue == null) { try { diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java index 71addc2fdaa..d10b147f18b 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java @@ -2710,10 +2710,10 @@ private void prepareVectorizationArgs(MethodCallExpr n, QueryScope queryScope, E addLiteralArg(expressions[i], argTypes[i], pyCallableWrapper); } else if (expressions[i] instanceof NameExpr) { String name = expressions[i].asNameExpr().getNameAsString(); - try { + if (queryScope.hasParamName(name)) { Object param = queryScope.readParamValue(name); pyCallableWrapper.addChunkArgument(new ConstantChunkArgument(param, argTypes[i])); - } catch (QueryScope.MissingVariableException ex) { + } else { // A column name or one of the special variables pyCallableWrapper.addChunkArgument(new ColumnChunkArgument(name, argTypes[i])); } diff --git a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java index 8d142b15820..f6268f6af0b 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java @@ -33,6 +33,7 @@ import java.io.File; import java.io.IOException; import java.nio.file.Path; +import java.util.Collections; import java.util.LinkedHashSet; import java.util.Map; import java.util.Optional; @@ -265,7 +266,10 @@ public QueryScope getQueryScope() { } private Class getVariableType(final String var) { - final Object result = getVariable(var, null); + if (hasVariableName(var)) { + return null; + } + final Object result = getVariable(var); if (result == null) { return null; } else if (result instanceof Table) { @@ -277,46 +281,27 @@ private Class getVariableType(final String var) { public TableDefinition getTableDefinition(final String var) { - Object o = getVariable(var, null); + if (!hasVariableName(var)) { + return null; + } + Object o = getVariable(var); return o instanceof Table ? ((Table) o).getDefinition() : null; } /** * Retrieve a variable from the script session's bindings. - *

- * Please use {@link ScriptSession#getVariable(String, Object)} if you expect the variable may not exist. * * @param name the variable to retrieve * @return the variable * @throws QueryScope.MissingVariableException if the variable does not exist */ - @NotNull - protected abstract Object getVariable(String name) throws QueryScope.MissingVariableException; - - /** - * Retrieve a variable from the script session's bindings. If the variable is not present, return defaultValue. - *

- * If the variable is present, but is not of type (T), a ClassCastException may result. - * - * @param name the variable to retrieve - * @param defaultValue the value to use when no value is present in the session's scope - * @param the type of the variable - * @return the value of the variable, or defaultValue if not present - */ - protected abstract T getVariable(String name, T defaultValue); - - /** - * Retrieves all of the variables present in the session's scope (e.g., Groovy binding, Python globals()). - * - * @return an unmodifiable map with variable names as the keys, and the Objects as the result - */ - protected abstract Map getVariables(); + protected abstract T getVariable(String name) throws QueryScope.MissingVariableException; /** * Retrieves all of the variable names present in the session's scope * - * @return an unmodifiable set of variable names + * @return an immutable set of variable names */ protected abstract Set getVariableNames(); @@ -363,7 +348,10 @@ public Class getVariableType(String var) { public T getVariable(String var, T defaultValue) { variableAccessLock.readLock().lock(); try { - return AbstractScriptSession.this.getVariable(var, defaultValue); + if (!hasVariableName(var)) { + return defaultValue; + } + return AbstractScriptSession.this.getVariable(var); } finally { variableAccessLock.readLock().unlock(); } @@ -418,7 +406,7 @@ public Set getParamNames() { result.add(name); } } - return result; + return Collections.unmodifiableSet(result); } @Override @@ -432,8 +420,7 @@ protected QueryScopeParam createParam(final String name) if (!NameValidator.isValidQueryParameterName(name)) { throw new QueryScope.MissingVariableException("Name " + name + " is invalid"); } - // noinspection unchecked - return new QueryScopeParam<>(name, (T) getVariableProvider().getVariable(name, null)); + return new QueryScopeParam<>(name, readParamValue(name)); } @Override @@ -441,6 +428,9 @@ public T readParamValue(final String name) throws QueryScope.MissingVariable if (!NameValidator.isValidQueryParameterName(name)) { throw new QueryScope.MissingVariableException("Name " + name + " is invalid"); } + if (!getVariableProvider().hasVariableName(name)) { + throw new MissingVariableException("Missing variable " + name); + } // noinspection unchecked return (T) getVariableProvider().getVariable(name, null); } @@ -455,10 +445,11 @@ public T readParamValue(final String name, final T defaultValue) { @Override public void putParam(final String name, final T value) { + NameValidator.validateQueryParameterName(name); if (value instanceof LivenessReferent && DynamicNode.notDynamicOrIsRefreshing(value)) { manage((LivenessReferent) value); } - getVariableProvider().setVariable(NameValidator.validateQueryParameterName(name), value); + getVariableProvider().setVariable(name, value); } @Override diff --git a/engine/table/src/main/java/io/deephaven/engine/util/DelegatingScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/DelegatingScriptSession.java index 590bc838ed4..dfbe94b98f1 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/DelegatingScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/DelegatingScriptSession.java @@ -3,6 +3,7 @@ */ package io.deephaven.engine.util; +import io.deephaven.engine.context.QueryScope; import io.deephaven.engine.liveness.LivenessReferent; import io.deephaven.engine.context.ExecutionContext; import org.jetbrains.annotations.NotNull; @@ -66,6 +67,11 @@ public VariableProvider getVariableProvider() { return delegate.getVariableProvider(); } + @Override + public QueryScope getQueryScope() { + return delegate.getQueryScope(); + } + @Override public Changes evaluateScript(String script, @Nullable String scriptName) { return contextualizeChanges(delegate.evaluateScript(script, scriptName)); diff --git a/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java b/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java index ff60cf4b6e5..8b4c27884eb 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java @@ -52,7 +52,6 @@ import org.codehaus.groovy.control.Phases; import org.codehaus.groovy.control.customizers.ImportCustomizer; import org.codehaus.groovy.tools.GroovyClass; -import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import javax.tools.JavaFileObject; @@ -276,26 +275,16 @@ public void runScriptOnce(String script) throws IOException { executedScripts.add(script); } - @NotNull @Override - public Object getVariable(String name) throws QueryScope.MissingVariableException { + protected T getVariable(String name) throws QueryScope.MissingVariableException { try { - return groovyShell.getContext().getVariable(name); + // noinspection unchecked + return (T) groovyShell.getContext().getVariable(name); } catch (MissingPropertyException mpe) { throw new QueryScope.MissingVariableException("No binding for: " + name, mpe); } } - @Override - public T getVariable(String name, T defaultValue) { - try { - // noinspection unchecked - return (T) getVariable(name); - } catch (QueryScope.MissingVariableException e) { - return defaultValue; - } - } - private void evaluateCommand(String command) { groovyShell.evaluate(command); } @@ -704,12 +693,6 @@ private static boolean isAnInteger(final String s) { } } - @Override - public Map getVariables() { - // noinspection unchecked - return Collections.unmodifiableMap(groovyShell.getContext().getVariables()); - } - @Override protected GroovySnapshot emptySnapshot() { return new GroovySnapshot(Collections.emptyMap()); @@ -755,18 +738,19 @@ public void close() { } } - public Set getVariableNames() { + @Override + protected Set getVariableNames() { // noinspection unchecked - return Collections.unmodifiableSet(groovyShell.getContext().getVariables().keySet()); + return Set.copyOf(groovyShell.getContext().getVariables().keySet()); } @Override - public boolean hasVariableName(String name) { + protected boolean hasVariableName(String name) { return groovyShell.getContext().hasVariable(name); } @Override - public void setVariable(String name, @Nullable Object newValue) { + protected void setVariable(String name, @Nullable Object newValue) { groovyShell.getContext().setVariable(NameValidator.validateQueryParameterName(name), newValue); // Observe changes from this "setVariable" (potentially capturing previous or concurrent external changes from diff --git a/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java b/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java index d62454e2e43..0e9451808e5 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java @@ -37,24 +37,13 @@ public NoLanguageDeephavenSession(final UpdateGraph updateGraph, variables = new LinkedHashMap<>(); } - @NotNull @Override - public Object getVariable(String name) throws QueryScope.MissingVariableException { - final Object var = variables.get(name); - if (var != null) { - return var; - } - throw new QueryScope.MissingVariableException("No global variable for: " + name); - } - - @Override - public T getVariable(String name, T defaultValue) { - try { - // noinspection unchecked - return (T) getVariable(name); - } catch (QueryScope.MissingVariableException e) { - return defaultValue; + protected T getVariable(String name) throws QueryScope.MissingVariableException { + if (!variables.containsKey(name)) { + throw new QueryScope.MissingVariableException("No global variable for: " + name); } + // noinspection unchecked + return (T) variables.get(name); } @Override @@ -81,22 +70,17 @@ protected void evaluate(String command, @Nullable String scriptName) { } @Override - public Map getVariables() { - return Collections.unmodifiableMap(variables); - } - - @Override - public Set getVariableNames() { - return Collections.unmodifiableSet(variables.keySet()); + protected Set getVariableNames() { + return Set.copyOf(variables.keySet()); } @Override - public boolean hasVariableName(String name) { + protected boolean hasVariableName(String name) { return variables.containsKey(name); } @Override - public void setVariable(String name, @Nullable Object newValue) { + protected void setVariable(String name, @Nullable Object newValue) { variables.put(name, newValue); // changeListener is always null for NoLanguageDeephavenSession; we have no mechanism for reporting scope // changes diff --git a/engine/table/src/main/java/io/deephaven/engine/util/ScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/ScriptSession.java index 09cc8e89726..0c7a7abaa05 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/ScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/ScriptSession.java @@ -26,6 +26,12 @@ public interface ScriptSession extends LivenessNode { */ VariableProvider getVariableProvider(); + /** + * + * @return + */ + QueryScope getQueryScope(); + /** * Obtain an {@link ExecutionContext} instance for the current script session. This is the execution context that is * used when executing scripts. diff --git a/engine/table/src/test/java/io/deephaven/engine/table/impl/FuzzerTest.java b/engine/table/src/test/java/io/deephaven/engine/table/impl/FuzzerTest.java index b179795b59d..2893b8374db 100644 --- a/engine/table/src/test/java/io/deephaven/engine/table/impl/FuzzerTest.java +++ b/engine/table/src/test/java/io/deephaven/engine/table/impl/FuzzerTest.java @@ -117,7 +117,7 @@ private void testFuzzerScriptFile(final long timeSeed, String s, boolean realtim clock.now += DateTimeUtils.SECOND / 10 * timeRandom.nextInt(20); } - final TimeTable timeTable = (TimeTable) session.getVariable("tt"); + final TimeTable timeTable = session.getQueryScope().readParamValue("tt", null); final int steps = TstUtils.SHORT_TESTS ? 20 : 100; @@ -157,7 +157,7 @@ public void testInterestingFuzzerSeeds() throws IOException, InterruptedExceptio validateBindingTables(session, hardReferences); final ControlledUpdateGraph updateGraph = ExecutionContext.getContext().getUpdateGraph().cast(); - final TimeTable timeTable = (TimeTable) session.getVariable("tt"); + final TimeTable timeTable = session.getQueryScope().readParamValue("tt"); for (int step = 0; step < fuzzDescriptor.steps; ++step) { final int fstep = step; updateGraph.runWithinUnitTestCycle(() -> { @@ -255,7 +255,7 @@ private void runLargeFuzzerSetWithSeed(long mainTestSeed, int firstRun, int last final long startTime = System.currentTimeMillis(); final long loopStart = System.currentTimeMillis(); - final TimeTable timeTable = (TimeTable) session.getVariable("tt"); + final TimeTable timeTable = session.getQueryScope().readParamValue("tt"); final RuntimeMemory.Sample sample = new RuntimeMemory.Sample(); final ControlledUpdateGraph updateGraph = ExecutionContext.getContext().getUpdateGraph().cast(); for (int step = 0; step < stepsToRun; ++step) { @@ -309,7 +309,7 @@ private void annotateBinding(GroovyDeephavenSession session) { private void addPrintListener( GroovyDeephavenSession session, final String variable, Map hardReferences) { - final Table table = (Table) session.getVariable(variable); + final Table table = session.getQueryScope().readParamValue(variable); System.out.println(variable); TableTools.showWithRowSet(table); System.out.println(); diff --git a/engine/table/src/test/java/io/deephaven/engine/util/scripts/TestGroovyDeephavenSession.java b/engine/table/src/test/java/io/deephaven/engine/util/scripts/TestGroovyDeephavenSession.java index e1074176a6e..4eca265daa3 100644 --- a/engine/table/src/test/java/io/deephaven/engine/util/scripts/TestGroovyDeephavenSession.java +++ b/engine/table/src/test/java/io/deephaven/engine/util/scripts/TestGroovyDeephavenSession.java @@ -64,7 +64,7 @@ public void teardown() { public T fetch(final String name, final Class clazz) { // note var is guaranteed to be non-null - final Object var = session.getVariable(name); + final Object var = session.getQueryScope().readParamValue(name); if (clazz.isAssignableFrom(var.getClass())) { // noinspection unchecked return (T) var; @@ -148,14 +148,14 @@ public void testGroovyClassReload() throws IOException { session.evaluateScript( "public class MyClass1 { public static int getMyVar() { return 42 ; } };\n ExecutionContext.getContext().getQueryLibrary().importClass(MyClass1);\nt = emptyTable(1).update(\"Var=MyClass1.getMyVar()\");\n", "Script1").throwIfError(); - final Table t = (Table) session.getVariable("t"); + final Table t = session.getQueryScope().readParamValue("t"); final int var1 = t.getColumnSource("Var").getInt(0); assertEquals(42, var1); session.evaluateScript( "public class MyClass1 { public static int getMyVar() { return 43 ; } };\n ExecutionContext.getContext().getQueryLibrary().importClass(MyClass1);\n t2 = emptyTable(1).update(\"Var=MyClass1.getMyVar()\");\n", "Script2").throwIfError(); - final Table t2 = (Table) session.getVariable("t2"); + final Table t2 = session.getQueryScope().readParamValue("t2"); final int var2 = t2.getColumnSource("Var").getInt(0); assertEquals(43, var2); @@ -165,7 +165,7 @@ public void testGroovyClassReload() throws IOException { session.evaluateScript("ExecutionContext.getContext().getQueryLibrary().importClass(" + getClass().getCanonicalName() + ".class);\n t3 = emptyTable(1).update(\"Var=" + getClass().getCanonicalName() + ".VALUE_FOR_IMPORT\");\n", "Script3").throwIfError(); - final Table t3 = (Table) session.getVariable("t3"); + final Table t3 = session.getQueryScope().readParamValue("t3"); final int var3 = t3.getColumnSource("Var").getInt(0); assertEquals(VALUE_FOR_IMPORT, var3); } @@ -453,7 +453,7 @@ public void testPotentialAmbiguousMethodCalls() { try { c = "primMin = min(" + Arrays.toString(a).substring(1, Arrays.toString(a).length() - 1) + ");\n"; session.evaluateScript(c).throwIfError(); - Integer primMin = (Integer) session.getVariable("primMin"); + Integer primMin = session.getQueryScope().readParamValue("primMin"); assertEquals(Numeric.min(a), primMin.intValue()); } catch (Exception e) { e.printStackTrace(); @@ -465,7 +465,7 @@ public void testPotentialAmbiguousMethodCalls() { c = "z = " + z + "; \n" + "d = " + d + "; \n" + "wrapMin = min(" + Arrays.toString(a).substring(1, Arrays.toString(a).length() - 1) + ", z);\n"; session.evaluateScript(c).throwIfError(); - Integer wrapperMin = (Integer) session.getVariable("wrapMin"); + Integer wrapperMin = session.getQueryScope().readParamValue("wrapMin"); assertEquals(Math.min(Numeric.min(a), z), wrapperMin.intValue()); } catch (Exception e) { e.printStackTrace(); @@ -476,7 +476,7 @@ public void testPotentialAmbiguousMethodCalls() { c = "z = " + z + "; \n" + "d = " + d + "; \n" + "m2 = max(" + Arrays.toString(a).substring(1, Arrays.toString(a).length() - 1) + ", z, d);\n"; session.evaluateScript(c).throwIfError(); - Double wrapperMax = (Double) session.getVariable("m2"); + Double wrapperMax = session.getQueryScope().readParamValue("m2"); assertEquals(5.0d, wrapperMax, 0.0d); } catch (Exception e) { e.printStackTrace(); @@ -487,7 +487,7 @@ public void testPotentialAmbiguousMethodCalls() { try { QueryScope.addParam("z", z); session.evaluateScript(c).throwIfError(); - final Table t = (Table) session.getVariable("t"); + final Table t = session.getQueryScope().readParamValue("t"); final int var2 = t.getColumnSource("Z").getInt(0); assertEquals(Numeric.min(Y, z), var2); } catch (Exception e) { @@ -499,7 +499,7 @@ public void testPotentialAmbiguousMethodCalls() { try { QueryScope.addParam("z", z); session.evaluateScript(c).throwIfError(); - final Table t = (Table) session.getVariable("t"); + final Table t = session.getQueryScope().readParamValue("t"); final double var2 = t.getColumnSource("Z").getDouble(0); assertEquals(Numeric.min(Y, 5d), var2, 1e-10); } catch (Exception e) { @@ -512,7 +512,7 @@ public void testPotentialAmbiguousMethodCalls() { QueryScope.addParam("z", z); QueryScope.addParam("d", d); session.evaluateScript(c).throwIfError(); - final Table t = (Table) session.getVariable("t"); + final Table t = session.getQueryScope().readParamValue("t"); final double var2 = t.getColumnSource("Z").getDouble(0); assertEquals(Numeric.min(Y, d), var2, 1e-10); } catch (Exception e) { @@ -524,7 +524,7 @@ public void testPotentialAmbiguousMethodCalls() { try { QueryScope.addParam("z", z); session.evaluateScript(c).throwIfError(); - final Table t = (Table) session.getVariable("t"); + final Table t = session.getQueryScope().readParamValue("t"); final int var2 = t.getColumnSource("Z").getInt(0); assertEquals(Numeric.max(Y, z), var2); } catch (Exception e) { @@ -537,7 +537,7 @@ public void testPotentialAmbiguousMethodCalls() { try { QueryScope.addParam("z", z); session.evaluateScript(c).throwIfError(); - final Table t = (Table) session.getVariable("t"); + final Table t = session.getQueryScope().readParamValue("t"); final double var2 = t.getColumnSource("Z").getDouble(0); assertEquals(Numeric.max(Y, 5d), var2, 1e-10); } catch (Exception e) { @@ -550,7 +550,7 @@ public void testPotentialAmbiguousMethodCalls() { QueryScope.addParam("z", z); QueryScope.addParam("d", d); session.evaluateScript(c).throwIfError(); - final Table t = (Table) session.getVariable("t"); + final Table t = session.getQueryScope().readParamValue("t"); final double var2 = t.getColumnSource("Z").getDouble(0); assertEquals(Numeric.max(Y, d), var2, 1e-10); } catch (Exception e) { @@ -562,7 +562,7 @@ public void testPotentialAmbiguousMethodCalls() { try { QueryScope.addParam("z", z); session.evaluateScript(c).throwIfError(); - final Table t = (Table) session.getVariable("t"); + final Table t = session.getQueryScope().readParamValue("t"); final int[] var2 = t.getColumnSource("Z", int[].class).get(0); assertArrayEquals(Sort.sort(Y, z), var2); } catch (Exception e) { @@ -577,7 +577,7 @@ public void testPotentialAmbiguousMethodCalls() { // QueryScope.addParam("z", z); // QueryScope.addParam("d", d); // session.evaluateScript(c).throwIfError(); - // final Table t = (Table) session.getVariable("t"); + // final Table t = session.getQueryScope().readParamValue("t"); // final Comparable[] var2 = t.getColumnSource("Z", Comparable[].class).get(0); // // noinspection unchecked // assertArrayEquals(Sort.sortObj(Y, d), var2); @@ -590,7 +590,7 @@ public void testPotentialAmbiguousMethodCalls() { try { QueryScope.addParam("z", z); session.evaluateScript(c).throwIfError(); - final Table t = (Table) session.getVariable("t"); + final Table t = session.getQueryScope().readParamValue("t"); final int[] var2 = t.getColumnSource("Z", int[].class).get(0); assertArrayEquals(Sort.sortDescending(Y, z), var2); } catch (Exception e) { @@ -627,7 +627,7 @@ public void testPotentialAmbiguousMethodCalls() { public void testMinInFormula() { QueryScope.addParam("d", 5d); session.evaluateScript("t = emptyTable(1).updateView(\"Y=1\", \"Z=min(Y,d)\")\n").throwIfError(); - final Table t = (Table) session.getVariable("t"); + final Table t = session.getQueryScope().readParamValue("t"); } } diff --git a/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java b/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java index df5cb3a84c3..966c52ae524 100644 --- a/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java +++ b/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java @@ -8,6 +8,7 @@ import io.deephaven.base.LockFreeArrayQueue; import io.deephaven.configuration.Configuration; import io.deephaven.engine.context.ExecutionContext; +import io.deephaven.engine.context.QueryScope; import io.deephaven.engine.table.Table; import io.deephaven.engine.table.impl.perf.QueryPerformanceNugget; import io.deephaven.engine.table.impl.perf.QueryPerformanceRecorder; @@ -280,13 +281,11 @@ public void bindTableToVariable( } exportBuilder.submit(() -> { - ScriptSession scriptSession = - exportedConsole != null ? exportedConsole.get() : scriptSessionProvider.get(); + QueryScope queryScope = exportedConsole != null ? exportedConsole.get().getQueryScope() + : ExecutionContext.getContext().getQueryScope(); + Table table = exportedTable.get(); - ExecutionContext.getContext().getQueryScope().putParam(request.getVariableName(), table); - if (DynamicNode.notDynamicOrIsRefreshing(table)) { - scriptSession.manage(table); - } + queryScope.putParam(request.getVariableName(), table); responseObserver.onNext(BindTableToVariableResponse.getDefaultInstance()); responseObserver.onCompleted(); }); diff --git a/server/src/main/java/io/deephaven/server/console/completer/PythonAutoCompleteObserver.java b/server/src/main/java/io/deephaven/server/console/completer/PythonAutoCompleteObserver.java index 93b55ffa477..4eea6ee85a8 100644 --- a/server/src/main/java/io/deephaven/server/console/completer/PythonAutoCompleteObserver.java +++ b/server/src/main/java/io/deephaven/server/console/completer/PythonAutoCompleteObserver.java @@ -11,14 +11,12 @@ import io.deephaven.server.console.ConsoleServiceGrpcImpl; import io.deephaven.server.session.SessionCloseableObserver; import io.deephaven.server.session.SessionState; -import io.deephaven.util.SafeCloseable; import io.grpc.stub.StreamObserver; import org.jpy.PyObject; import javax.inject.Provider; import java.util.ArrayList; import java.util.List; -import java.util.stream.Collectors; import static io.deephaven.extensions.barrage.util.GrpcUtil.safelyComplete; import static io.deephaven.extensions.barrage.util.GrpcUtil.safelyOnNext; @@ -113,7 +111,7 @@ private void handleAutocompleteRequest(AutoCompleteRequest request, request.getRequestId() > 0 ? request.getRequestId() : request.getGetCompletionItems().getRequestId(); try { final ScriptSession scriptSession = exportedConsole.get(); - PyObject completer = (PyObject) scriptSession.getVariableProvider().getVariable("jedi_settings", null); + PyObject completer = scriptSession.getVariableProvider().getVariable("jedi_settings", null); boolean canJedi = completer.callMethod("is_enabled").getBooleanValue(); if (!canJedi) { log.trace().append("Ignoring completion request because jedi is disabled").endl(); diff --git a/server/src/test/java/io/deephaven/server/appmode/ApplicationTest.java b/server/src/test/java/io/deephaven/server/appmode/ApplicationTest.java index 73e40a82107..01e668e1bbf 100644 --- a/server/src/test/java/io/deephaven/server/appmode/ApplicationTest.java +++ b/server/src/test/java/io/deephaven/server/appmode/ApplicationTest.java @@ -31,6 +31,14 @@ public class ApplicationTest { private AbstractScriptSession session = null; + @After + public void tearDown() { + if (session != null) { + session.forceReferenceCountToZero(); + session = null; + } + } + @Test public void app00() { ApplicationState app = ApplicationFactory.create(ApplicationConfigs.testAppDir(), ApplicationConfigs.app00(), diff --git a/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java b/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java index f9716a31d93..7483d09a724 100644 --- a/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java +++ b/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java @@ -680,30 +680,30 @@ public void testGetSchema() { final Table tickingTable = ExecutionContext.getContext().getUpdateGraph().sharedLock().computeLocked( () -> TableTools.timeTable(1_000_000).update("I = i")); - try (final SafeCloseable ignored = LivenessScopeStack.open(scriptSession.getQueryScope(), false)) { - // stuff table into the scope - ExecutionContext.getContext().getQueryScope().putParam(staticTableName, table); - ExecutionContext.getContext().getQueryScope().putParam(tickingTableName, tickingTable); - - // test fetch info from scoped ticket - assertSchemaMatchesTable(flightClient.getSchema(arrowFlightDescriptorForName(staticTableName)).getSchema(), - table); - assertSchemaMatchesTable(flightClient.getSchema(arrowFlightDescriptorForName(tickingTableName)).getSchema(), - tickingTable); - - // test list flights which runs through scoped tickets - final MutableInt seenTables = new MutableInt(); - flightClient.listFlights(Criteria.ALL).forEach(fi -> { - seenTables.increment(); - if (fi.getDescriptor().equals(arrowFlightDescriptorForName(staticTableName))) { - assertInfoMatchesTable(fi, table); - } else { - assertInfoMatchesTable(fi, tickingTable); - } - }); + // try (final SafeCloseable ignored = LivenessScopeStack.open(scriptSession.getQueryScope(), false)) { + // stuff table into the scope + ExecutionContext.getContext().getQueryScope().putParam(staticTableName, table); + ExecutionContext.getContext().getQueryScope().putParam(tickingTableName, tickingTable); - Assert.eq(seenTables.intValue(), "seenTables.intValue()", 2); - } + // test fetch info from scoped ticket + assertSchemaMatchesTable(flightClient.getSchema(arrowFlightDescriptorForName(staticTableName)).getSchema(), + table); + assertSchemaMatchesTable(flightClient.getSchema(arrowFlightDescriptorForName(tickingTableName)).getSchema(), + tickingTable); + + // test list flights which runs through scoped tickets + final MutableInt seenTables = new MutableInt(); + flightClient.listFlights(Criteria.ALL).forEach(fi -> { + seenTables.increment(); + if (fi.getDescriptor().equals(arrowFlightDescriptorForName(staticTableName))) { + assertInfoMatchesTable(fi, table); + } else { + assertInfoMatchesTable(fi, tickingTable); + } + }); + + Assert.eq(seenTables.intValue(), "seenTables.intValue()", 2); + // } } @Test From 5a1a7cf21f2f9a6e31b2dadf961148956e351f9a Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Thu, 21 Dec 2023 12:22:17 -0600 Subject: [PATCH 06/23] Remove VariableProvider, use QueryScope instead --- .../engine/util/AbstractScriptSession.java | 118 ++++++------------ .../engine/util/DelegatingScriptSession.java | 5 - .../deephaven/engine/util/ScriptSession.java | 11 +- .../engine/util/VariableProvider.java | 25 ---- .../lang/completion/ChunkerCompleter.java | 25 ++-- .../lang/completion/CompletionRequest.java | 12 +- .../completion/ChunkerCompleterMixin.groovy | 4 +- .../ChunkerCompletionHandlerTest.groovy | 52 ++------ .../lang/completion/ChunkerGroovyTest.groovy | 7 -- .../completion/ChunkerParseTestMixin.groovy | 6 +- .../lang/completion/ChunkerParserTest.groovy | 8 -- .../lang/completion/ChunkerPythonTest.groovy | 8 -- ...lumnExpressionCompletionHandlerTest.groovy | 53 ++++---- .../console/ConsoleServiceGrpcImpl.java | 2 +- .../completer/JavaAutoCompleteObserver.java | 4 +- .../completer/PythonAutoCompleteObserver.java | 8 +- .../ApplicationServiceGrpcImplTest.java | 2 +- .../test/FlightMessageRoundTripTest.java | 2 +- 18 files changed, 111 insertions(+), 241 deletions(-) delete mode 100644 engine/table/src/main/java/io/deephaven/engine/util/VariableProvider.java diff --git a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java index f6268f6af0b..c73224f7e14 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java @@ -12,7 +12,6 @@ import io.deephaven.engine.context.ExecutionContext; import io.deephaven.engine.liveness.LivenessArtifact; import io.deephaven.engine.liveness.LivenessReferent; -import io.deephaven.engine.liveness.LivenessScope; import io.deephaven.engine.liveness.LivenessScopeStack; import io.deephaven.engine.table.PartitionedTable; import io.deephaven.engine.table.Table; @@ -35,12 +34,13 @@ import java.nio.file.Path; import java.util.Collections; import java.util.LinkedHashSet; -import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.UUID; +import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.function.Supplier; import static io.deephaven.engine.table.Table.NON_DISPLAY_TABLE; @@ -150,9 +150,12 @@ public synchronized final Changes evaluateScript(final String script, @Nullable RuntimeException evaluateErr = null; final Changes diff; + + // Take the write lock while running script code, so that readers can't look at variables again until + // the script has finished. + variableAccessLock.writeLock().lock(); // retain any objects which are created in the executed code, we'll release them when the script session // closes - variableAccessLock.writeLock().lock(); try (final S initialSnapshot = takeSnapshot(); final SafeCloseable ignored = LivenessScopeStack.open(queryScope, false)) { @@ -258,9 +261,6 @@ protected void destroy() { */ protected abstract void evaluate(String command, @Nullable String scriptName); - /** - * @return a query scope that wraps - */ public QueryScope getQueryScope() { return queryScope; } @@ -321,72 +321,22 @@ public TableDefinition getTableDefinition(final String var) { */ protected abstract void setVariable(String name, @Nullable Object value); - @Override - public VariableProvider getVariableProvider() { - return new VariableProvider() { - @Override - public Set getVariableNames() { - variableAccessLock.readLock().lock(); - try { - return AbstractScriptSession.this.getVariableNames(); - } finally { - variableAccessLock.readLock().unlock(); - } - } - - @Override - public Class getVariableType(String var) { - variableAccessLock.readLock().lock(); - try { - return AbstractScriptSession.this.getVariableType(var); - } finally { - variableAccessLock.readLock().unlock(); - } - } - - @Override - public T getVariable(String var, T defaultValue) { - variableAccessLock.readLock().lock(); - try { - if (!hasVariableName(var)) { - return defaultValue; - } - return AbstractScriptSession.this.getVariable(var); - } finally { - variableAccessLock.readLock().unlock(); - } - } - - @Override - public TableDefinition getTableDefinition(String var) { - variableAccessLock.readLock().lock(); - try { - return AbstractScriptSession.this.getTableDefinition(var); - } finally { - variableAccessLock.readLock().unlock(); - } - } - - @Override - public boolean hasVariableName(String name) { - variableAccessLock.readLock().lock(); - try { - return AbstractScriptSession.this.hasVariableName(name); - } finally { - variableAccessLock.readLock().unlock(); - } - } + private static void doLocked(@NotNull final Lock lock, @NotNull final Runnable runnable) { + lock.lock(); + try { + runnable.run(); + } finally { + lock.unlock(); + } + } - @Override - public void setVariable(String name, T value) { - variableAccessLock.writeLock().lock(); - try { - AbstractScriptSession.this.setVariable(name, value); - } finally { - variableAccessLock.writeLock().unlock(); - } - } - }; + private static T doLocked(@NotNull final Lock lock, @NotNull final Supplier supplier) { + lock.lock(); + try { + return supplier.get(); + } finally { + lock.unlock(); + } } // ----------------------------------------------------------------------------------------------------------------- @@ -394,6 +344,9 @@ public void setVariable(String name, T value) { // ----------------------------------------------------------------------------------------------------------------- public class ScriptSessionQueryScope extends QueryScope { + /** + * Internal workaround to support python calling pushScope. + */ public ScriptSession scriptSession() { return AbstractScriptSession.this; } @@ -401,7 +354,9 @@ public ScriptSession scriptSession() { @Override public Set getParamNames() { final Set result = new LinkedHashSet<>(); - for (final String name : getVariableProvider().getVariableNames()) { + Set variables = + doLocked(variableAccessLock.readLock(), AbstractScriptSession.this::getVariableNames); + for (final String name : variables) { if (NameValidator.isValidQueryParameterName(name)) { result.add(name); } @@ -411,7 +366,8 @@ public Set getParamNames() { @Override public boolean hasParamName(String name) { - return NameValidator.isValidQueryParameterName(name) && getVariableProvider().hasVariableName(name); + return NameValidator.isValidQueryParameterName(name) + && doLocked(variableAccessLock.readLock(), () -> AbstractScriptSession.this.hasVariableName(name)); } @Override @@ -428,11 +384,15 @@ public T readParamValue(final String name) throws QueryScope.MissingVariable if (!NameValidator.isValidQueryParameterName(name)) { throw new QueryScope.MissingVariableException("Name " + name + " is invalid"); } - if (!getVariableProvider().hasVariableName(name)) { - throw new MissingVariableException("Missing variable " + name); + variableAccessLock.readLock().lock(); + try { + if (!hasVariableName(name)) { + throw new MissingVariableException("Missing variable " + name); + } + return AbstractScriptSession.this.getVariable(name); + } finally { + variableAccessLock.readLock().unlock(); } - // noinspection unchecked - return (T) getVariableProvider().getVariable(name, null); } @Override @@ -440,7 +400,7 @@ public T readParamValue(final String name, final T defaultValue) { if (!NameValidator.isValidQueryParameterName(name)) { return defaultValue; } - return getVariableProvider().getVariable(name, defaultValue); + return doLocked(variableAccessLock.readLock(), () -> AbstractScriptSession.this.getVariable(name)); } @Override @@ -449,7 +409,7 @@ public void putParam(final String name, final T value) { if (value instanceof LivenessReferent && DynamicNode.notDynamicOrIsRefreshing(value)) { manage((LivenessReferent) value); } - getVariableProvider().setVariable(name, value); + doLocked(variableAccessLock.writeLock(), () -> AbstractScriptSession.this.setVariable(name, value)); } @Override diff --git a/engine/table/src/main/java/io/deephaven/engine/util/DelegatingScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/DelegatingScriptSession.java index dfbe94b98f1..b9f3814fd2d 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/DelegatingScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/DelegatingScriptSession.java @@ -62,11 +62,6 @@ public void observeScopeChanges() { delegate.observeScopeChanges(); } - @Override - public VariableProvider getVariableProvider() { - return delegate.getVariableProvider(); - } - @Override public QueryScope getQueryScope() { return delegate.getQueryScope(); diff --git a/engine/table/src/main/java/io/deephaven/engine/util/ScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/ScriptSession.java index 0c7a7abaa05..44280df2aa0 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/ScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/ScriptSession.java @@ -19,16 +19,9 @@ public interface ScriptSession extends LivenessNode { /** - * A {@link VariableProvider} instance, for services like autocomplete which may want a limited "just the variables" - * view of our session state. + * Provides access to the query scope defined by the state in this script session. * - * @return a VariableProvider instance backed by the global/binding context of this script session. - */ - VariableProvider getVariableProvider(); - - /** - * - * @return + * @return an implementation defined QueryScope, allowing access to state in the script session */ QueryScope getQueryScope(); diff --git a/engine/table/src/main/java/io/deephaven/engine/util/VariableProvider.java b/engine/table/src/main/java/io/deephaven/engine/util/VariableProvider.java deleted file mode 100644 index adf3771e07c..00000000000 --- a/engine/table/src/main/java/io/deephaven/engine/util/VariableProvider.java +++ /dev/null @@ -1,25 +0,0 @@ -/** - * Copyright (c) 2016-2022 Deephaven Data Labs and Patent Pending - */ -package io.deephaven.engine.util; - -import io.deephaven.engine.table.TableDefinition; - -import java.util.Set; - -/** - * Provides access to the variables in a script session. - */ -public interface VariableProvider { - Set getVariableNames(); - - Class getVariableType(String var); - - T getVariable(String var, T defaultValue); - - TableDefinition getTableDefinition(String var); - - boolean hasVariableName(String name); - - void setVariable(String name, T value); -} diff --git a/open-api/lang-tools/src/main/java/io/deephaven/lang/completion/ChunkerCompleter.java b/open-api/lang-tools/src/main/java/io/deephaven/lang/completion/ChunkerCompleter.java index ca59bf2453e..258c4861c35 100644 --- a/open-api/lang-tools/src/main/java/io/deephaven/lang/completion/ChunkerCompleter.java +++ b/open-api/lang-tools/src/main/java/io/deephaven/lang/completion/ChunkerCompleter.java @@ -3,11 +3,11 @@ */ package io.deephaven.lang.completion; +import io.deephaven.engine.context.QueryScope; import io.deephaven.engine.table.ColumnDefinition; import io.deephaven.engine.table.Table; import io.deephaven.engine.table.TableDefinition; import io.deephaven.engine.context.QueryScope.MissingVariableException; -import io.deephaven.engine.util.VariableProvider; import io.deephaven.io.logger.Logger; import io.deephaven.lang.api.HasScope; import io.deephaven.lang.api.IsScope; @@ -44,16 +44,16 @@ public enum SearchDirection { public static final String PROP_SUGGEST_STATIC_METHODS = "suggest.all.static.methods"; private final Logger log; - private final VariableProvider variables; + private final QueryScope variables; private final CompletionLookups lookups; private String defaultQuoteType; private ParsedDocument doc; - public ChunkerCompleter(final Logger log, VariableProvider variables) { + public ChunkerCompleter(final Logger log, QueryScope variables) { this(log, variables, new CompletionLookups(Collections.emptySet())); } - public ChunkerCompleter(final Logger log, VariableProvider variables, CompletionLookups lookups) { + public ChunkerCompleter(final Logger log, QueryScope variables, CompletionLookups lookups) { this.log = log; this.variables = variables; this.lookups = lookups; @@ -574,7 +574,7 @@ private void assignCompletion(Collection results, Chunke // no value for this assignment... offer up anything from scope. FuzzyList sorted = new FuzzyList<>(""); - for (String varName : variables.getVariableNames()) { + for (String varName : variables.getParamNames()) { sorted.add(varName, varName); } for (String varName : sorted) { @@ -593,7 +593,7 @@ private void assignCompletion(Collection results, Chunke // Really, we should be adding all variable names like we do, then visiting all source, // removing anything which occurs later than here in source, and adding any assignments which // occur earlier in source-but-not-in-runtime-variable-pool. IDS-1517-23 - for (String varName : variables.getVariableNames()) { + for (String varName : variables.getParamNames()) { if (camelMatch(varName, startWith)) { sorted.add(varName, varName); } @@ -883,7 +883,7 @@ private void doMethodCompletion( private void doVariableCompletion(Collection results, String variablePrefix, Token replacing, CompletionRequest request) { FuzzyList sorter = new FuzzyList<>(variablePrefix); - for (String name : variables.getVariableNames()) { + for (String name : variables.getParamNames()) { if (!name.equals(variablePrefix) && camelMatch(name, variablePrefix)) { // only suggest names which are camel-case-matches (ignoring same-as-existing-variable names) sorter.add(name, name); @@ -1018,9 +1018,12 @@ private Optional> resolveScopeType( IsScope o = scope.get(0); - final Class type = variables.getVariableType(o.getName()); - if (type != null) { - return Optional.of(type); + final Object instance = variables.readParamValue(o.getName(), null); + if (instance != null) { + if (instance instanceof Table) { + return Optional.of(Table.class); + } + return Optional.of(instance.getClass()); } Optional> result = resolveScope(scope) .map(Object::getClass); @@ -1099,7 +1102,7 @@ private Optional resolveScope(List scope) { // TODO: also handle static classes / variables only present in source (code not run yet) IDS-1517-23 try { - final Object var = variables.getVariable(o.getName(), null); + final Object var = variables.readParamValue(o.getName(), null); if (var == null) { return Optional.empty(); } diff --git a/open-api/lang-tools/src/main/java/io/deephaven/lang/completion/CompletionRequest.java b/open-api/lang-tools/src/main/java/io/deephaven/lang/completion/CompletionRequest.java index 3d0ce0543e7..7c30c33f89f 100644 --- a/open-api/lang-tools/src/main/java/io/deephaven/lang/completion/CompletionRequest.java +++ b/open-api/lang-tools/src/main/java/io/deephaven/lang/completion/CompletionRequest.java @@ -4,9 +4,10 @@ package io.deephaven.lang.completion; import io.deephaven.base.verify.Require; +import io.deephaven.engine.context.QueryScope; import io.deephaven.engine.table.ColumnDefinition; +import io.deephaven.engine.table.Table; import io.deephaven.engine.table.TableDefinition; -import io.deephaven.engine.util.VariableProvider; import io.deephaven.internal.log.LoggerFactory; import io.deephaven.io.logger.Logger; import io.deephaven.lang.generated.ChunkerAssign; @@ -83,7 +84,7 @@ public CompletionRequest candidate(int index) { } public TableDefinition getTableDefinition(final ChunkerCompleter completer, final ParsedDocument doc, - VariableProvider variables, String name) { + QueryScope variables, String name) { // Each request maintains a local cache of looked-up table definitions, to avoid going to the VariableHandler // unless needed // Note that we do NOT go to the completer.getReferencedTables map at all; @@ -95,8 +96,11 @@ public TableDefinition getTableDefinition(final ChunkerCompleter completer, fina // variable exists. return localDefs.get(name); } - TableDefinition result = variables.getTableDefinition(name); - if (result == null) { + Object value = variables.readParamValue(name, null); + TableDefinition result = null; + if (value instanceof Table) { + result = ((Table) value).getDefinition(); + } else { // If the result was null, we can try to search for an assign statement that is initialized w/ something we // _can_ grok. final List assignment = completer.findAssignment(doc, this, name); diff --git a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerCompleterMixin.groovy b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerCompleterMixin.groovy index dee84d5d6ca..0193a6d3ffc 100644 --- a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerCompleterMixin.groovy +++ b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerCompleterMixin.groovy @@ -1,6 +1,6 @@ package io.deephaven.lang.completion -import io.deephaven.engine.util.VariableProvider +import io.deephaven.engine.context.QueryScope import io.deephaven.internal.log.LoggerFactory import io.deephaven.io.logger.Logger import io.deephaven.proto.backplane.script.grpc.CompletionItem @@ -10,7 +10,7 @@ import io.deephaven.lang.parse.ParsedDocument */ trait ChunkerCompleterMixin extends ChunkerParseTestMixin { - Set performSearch(ParsedDocument doc, int from, VariableProvider vars) { + Set performSearch(ParsedDocument doc, int from, QueryScope vars) { Logger log = LoggerFactory.getLogger(CompletionHandler) diff --git a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerCompletionHandlerTest.groovy b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerCompletionHandlerTest.groovy index 27a00a3dc04..32f3667fff7 100644 --- a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerCompletionHandlerTest.groovy +++ b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerCompletionHandlerTest.groovy @@ -1,9 +1,9 @@ package io.deephaven.lang.completion +import io.deephaven.engine.context.QueryScope import io.deephaven.engine.context.TestExecutionContext import io.deephaven.engine.table.Table -import io.deephaven.engine.table.TableDefinition -import io.deephaven.engine.util.VariableProvider +import io.deephaven.engine.table.TableFactory import io.deephaven.internal.log.LoggerFactory import io.deephaven.io.logger.Logger import io.deephaven.lang.parse.CompletionParser @@ -49,24 +49,14 @@ class ChunkerCompletionHandlerTest extends Specification implements ChunkerCompl return this } - VariableProvider mockVars(Closure configure) { - return Mock(VariableProvider, configure) - } - @Unroll def "Complex chains of methods on tables can parse #src sanely at any index"(String src) { CompletionParser p = new CompletionParser() when: doc = p.parse(src) - VariableProvider vars = Mock(VariableProvider){ - _ * getVariableType('t') >> Table - _ * getVariableType(_) >> null - _ * getVariable(_, _) >> null - _ * getVariableNames() >> [] - _ * getTableDefinition('emptyTable') >> TableDefinition.of() - 0 * _ - } + QueryScope vars = new QueryScope.StandaloneImpl(); + vars.putParam('t', TableFactory.emptyTable(0)) then: assertAllValid(doc, src) @@ -96,13 +86,8 @@ t = emptyTable(10).update( t = emptyTable(10) u = t.''' CompletionParser p = new CompletionParser() - VariableProvider vars = Mock(VariableProvider){ - _ * getVariableType('t') >> Table - _ * getVariableType(_) >> null - _ * getVariable(_,_) >> null - _ * getVariableNames() >> [] - 0 * _ - } + QueryScope vars = new QueryScope.StandaloneImpl(); + vars.putParam('t', TableFactory.emptyTable(0)) when: doc = p.parse(src) @@ -137,10 +122,9 @@ u = t.''' doc = p.parse(src) LoggerFactory.getLogger(CompletionHandler) - VariableProvider variables = Mock(VariableProvider) { - _ * getVariableNames() >> ['emptyTable'] - 0 * _ - } + QueryScope variables = new QueryScope.StandaloneImpl(); + variables.putParam('emptyTable', TableFactory.emptyTable(0)) + when: "Cursor is at EOF, table name completion from t is returned" Set result = performSearch(doc, src.length(), variables) @@ -164,10 +148,8 @@ c = 3 p.update(uri, 1, [ makeChange(3, 0, src2) ]) doc = p.finish(uri) - VariableProvider variables = Mock(VariableProvider) { - _ * getVariableNames() >> ['emptyTable'] - 0 * _ - } + QueryScope variables = new QueryScope.StandaloneImpl(); + variables.putParam('emptyTable', TableFactory.emptyTable(0)) when: "Cursor is at EOF, table name completion from t is returned" Set result = performSearch(doc, (src1 + src2).length(), variables) @@ -194,20 +176,12 @@ b = 2 doc = p.parse(src) LoggerFactory.getLogger(CompletionHandler) - VariableProvider variables = Mock(VariableProvider) { - _ * getVariableNames() >> ['emptyTable'] - 0 * _ - } + QueryScope variables = new QueryScope.StandaloneImpl(); + variables.putParam('emptyTable', TableFactory.emptyTable(0)) when: "Cursor is in the comment after the variablename+dot and completion is requested" Set result = performSearch(doc, beforeCursor.length(), variables) then: "Expect the completion result to suggest nothing" result.size() == 0 } - @Override - VariableProvider getVariables() { - return Mock(VariableProvider) { - _ * getVariableNames() >> [] - } - } } diff --git a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerGroovyTest.groovy b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerGroovyTest.groovy index cd337ffbb5a..a2ed96dfb71 100644 --- a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerGroovyTest.groovy +++ b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerGroovyTest.groovy @@ -1,6 +1,5 @@ package io.deephaven.lang.completion -import io.deephaven.engine.util.VariableProvider import io.deephaven.lang.parse.CompletionParser import spock.lang.Specification @@ -112,10 +111,4 @@ l.add(new SomeClass().method(new Thing1.Thing2> [] - } - } } diff --git a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerParseTestMixin.groovy b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerParseTestMixin.groovy index c77bf009ebe..a03fce0f9e2 100644 --- a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerParseTestMixin.groovy +++ b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerParseTestMixin.groovy @@ -2,7 +2,7 @@ package io.deephaven.lang.completion import groovy.transform.CompileDynamic import groovy.transform.CompileStatic -import io.deephaven.engine.util.VariableProvider +import io.deephaven.engine.context.EmptyQueryScope import io.deephaven.internal.log.LoggerFactory import io.deephaven.io.logger.Logger import io.deephaven.lang.generated.Node @@ -38,8 +38,6 @@ trait ChunkerParseTestMixin { } } - abstract VariableProvider getVariables() - ParsedDocument parse(String command) { CompletionParser p = new CompletionParser(); doc = p.parse(command) @@ -57,7 +55,7 @@ trait ChunkerParseTestMixin { String doCompletion(String command, int completionPos, int resultIndex) { Logger log = LoggerFactory.getLogger(CompletionHandler) - ChunkerCompleter completer = new ChunkerCompleter(log, variables) + ChunkerCompleter completer = new ChunkerCompleter(log, EmptyQueryScope.INSTANCE) parse(command) Set results = completer.runCompletion(doc, completionPos) List result = results.toList() diff --git a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerParserTest.groovy b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerParserTest.groovy index 542ab36e105..e84c8e12922 100644 --- a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerParserTest.groovy +++ b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerParserTest.groovy @@ -1,7 +1,6 @@ package io.deephaven.lang.completion import groovy.text.SimpleTemplateEngine -import io.deephaven.engine.util.VariableProvider import io.deephaven.lang.generated.* import io.deephaven.lang.parse.CompletionParser import io.deephaven.lang.parse.LspTools @@ -222,13 +221,6 @@ t = db. q << [ '"' , "'" ] } - @Override - VariableProvider getVariables() { - return Mock(VariableProvider) { - _ * getVariableNames() >> [] - } - } - static String quotify(String str, String q) { new SimpleTemplateEngine().createTemplate(str) .make([ q: q]).toString() diff --git a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerPythonTest.groovy b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerPythonTest.groovy index 8ab97c7b01a..acd9781d6e0 100644 --- a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerPythonTest.groovy +++ b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerPythonTest.groovy @@ -1,6 +1,5 @@ package io.deephaven.lang.completion -import io.deephaven.engine.util.VariableProvider import io.deephaven.lang.parse.CompletionParser import spock.lang.Specification @@ -32,11 +31,4 @@ def typesafe(param: Type = None) -> int: assertAllValid(doc, src) } - - @Override - VariableProvider getVariables() { - return Mock(VariableProvider) { - _ * getVariableNames() >> [] - } - } } diff --git a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ColumnExpressionCompletionHandlerTest.groovy b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ColumnExpressionCompletionHandlerTest.groovy index 89cf1c7a665..50e2fdd39f2 100644 --- a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ColumnExpressionCompletionHandlerTest.groovy +++ b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ColumnExpressionCompletionHandlerTest.groovy @@ -1,14 +1,13 @@ package io.deephaven.lang.completion -import io.deephaven.base.clock.Clock +import io.deephaven.engine.context.QueryScope import io.deephaven.engine.context.TestExecutionContext; -import io.deephaven.engine.table.Table -import io.deephaven.engine.table.TableDefinition -import io.deephaven.engine.util.VariableProvider +import io.deephaven.engine.table.TableFactory import io.deephaven.internal.log.LoggerFactory import io.deephaven.io.logger.Logger import io.deephaven.lang.parse.CompletionParser import io.deephaven.proto.backplane.script.grpc.CompletionItem +import io.deephaven.qst.column.Column import io.deephaven.util.SafeCloseable import spock.lang.Specification import spock.lang.Unroll @@ -41,12 +40,11 @@ class ColumnExpressionCompletionHandlerTest extends Specification implements Chu doc = p.parse(src) Logger log = LoggerFactory.getLogger(CompletionHandler) - VariableProvider variables = Mock(VariableProvider) { - (0..1) * getVariableNames() >> ['t'] - (0..1) * getVariableType('t') >> Table - (0..1) * getTableDefinition('t') >> TableDefinition.from(['Date', 'DateTime'], [String, Instant] - ) - } + QueryScope variables = new QueryScope.StandaloneImpl() + variables.putParam("t", TableFactory.newTable( + Column.of('Date', String.class, new String[0]), + Column.of('DateTime', Instant.class, new Instant[0])) + ) ChunkerCompleter completer = new ChunkerCompleter(log, variables) @@ -103,17 +101,17 @@ t = t.updateView ( 'D doc = p.parse(src) Logger log = LoggerFactory.getLogger(CompletionHandler) - VariableProvider variables = Mock() { - (0..1) * getTableDefinition('t') >> TableDefinition.from( - ['Date', 'Delta', 'NotMeThough'], [String, Long, Integer]) - (0..1) * getVariableType('t') >> Table - (0..1) * getVariableNames() >> [] - } + QueryScope variables = new QueryScope.StandaloneImpl(); + variables.putParam('t', TableFactory.newTable( + Column.of('Date', String.class, new String[0]), + Column.of('Delta', Long.class, new Long[0]), + Column.of('NotMeThough', Integer.class, new Integer[0]), + )); ChunkerCompleter completer = new ChunkerCompleter(log, variables) when: "Cursor is at EOF, table name completion from t is returned" - Set result = completer.runCompletion(doc, pos) + Set result = completer.runCompletion(doc, pos) result.removeIf({it.textEdit.text == 'updateView('}) // t = t.where ( 'D @@ -143,10 +141,11 @@ t = t.update('A=') .update( 'B=') doc = p.parse(src) Logger log = LoggerFactory.getLogger(CompletionHandler) - VariableProvider variables = Mock(VariableProvider) { - _ * getTableDefinition('t') >> TableDefinition.from(['A1', 'A2'], [Long, Integer]) - 0 * _ - } + QueryScope variables = new QueryScope.StandaloneImpl() + variables.putParam('t', TableFactory.newTable( + Column.of('A1', Long.class, new Long[0]), + Column.of('A2', Integer.class, new Integer[0]), + )); ChunkerCompleter completer = new ChunkerCompleter(log, variables) @@ -200,10 +199,8 @@ t.where('""" doc = p.parse(src) Logger log = LoggerFactory.getLogger(CompletionHandler) - VariableProvider variables = Mock(VariableProvider) { - _ * getTableDefinition('t') >> null - 0 * _ - } + QueryScope variables = new QueryScope.StandaloneImpl() + variables.putParam('t', null); ChunkerCompleter completer = new ChunkerCompleter(log, variables) @@ -219,10 +216,4 @@ t.where('""" cleanup: System.clearProperty(ChunkerCompleter.PROP_SUGGEST_STATIC_METHODS) } - @Override - VariableProvider getVariables() { - return Mock(VariableProvider) { - _ * getVariableNames() >> [] - } - } } diff --git a/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java b/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java index 966c52ae524..e73643fa2e2 100644 --- a/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java +++ b/server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java @@ -305,7 +305,7 @@ public StreamObserver autoCompleteStream( final ScriptSession scriptSession = scriptSessionProvider.get(); scriptSession.evaluateScript( "from deephaven_internal.auto_completer import jedi_settings ; jedi_settings.set_scope(globals())"); - settings[0] = scriptSession.getVariableProvider().getVariable("jedi_settings", null); + settings[0] = scriptSession.getQueryScope().readParamValue("jedi_settings"); } catch (Exception err) { log.error().append("Error trying to enable jedi autocomplete").append(err).endl(); } diff --git a/server/src/main/java/io/deephaven/server/console/completer/JavaAutoCompleteObserver.java b/server/src/main/java/io/deephaven/server/console/completer/JavaAutoCompleteObserver.java index dfe76a3f095..338957936be 100644 --- a/server/src/main/java/io/deephaven/server/console/completer/JavaAutoCompleteObserver.java +++ b/server/src/main/java/io/deephaven/server/console/completer/JavaAutoCompleteObserver.java @@ -1,8 +1,8 @@ package io.deephaven.server.console.completer; import com.google.rpc.Code; +import io.deephaven.engine.context.QueryScope; import io.deephaven.engine.util.ScriptSession; -import io.deephaven.engine.util.VariableProvider; import io.deephaven.internal.log.LoggerFactory; import io.deephaven.io.logger.Logger; import io.deephaven.lang.completion.ChunkerCompleter; @@ -172,7 +172,7 @@ private GetCompletionItemsResponse getCompletionItems(GetCompletionItemsRequest SessionState.ExportObject exportedConsole, CompletionParser parser, StreamObserver responseObserver) { final ScriptSession scriptSession = exportedConsole.get(); - final VariableProvider vars = scriptSession.getVariableProvider(); + final QueryScope vars = scriptSession.getQueryScope(); final VersionedTextDocumentIdentifier doc = request.getTextDocument(); final CompletionLookups h = CompletionLookups.preload(scriptSession, customCompletionFactory); // The only stateful part of a completer is the CompletionLookups, which are already diff --git a/server/src/main/java/io/deephaven/server/console/completer/PythonAutoCompleteObserver.java b/server/src/main/java/io/deephaven/server/console/completer/PythonAutoCompleteObserver.java index 4eea6ee85a8..4f44caa6ff3 100644 --- a/server/src/main/java/io/deephaven/server/console/completer/PythonAutoCompleteObserver.java +++ b/server/src/main/java/io/deephaven/server/console/completer/PythonAutoCompleteObserver.java @@ -48,7 +48,7 @@ public void onNext(AutoCompleteRequest value) { case OPEN_DOCUMENT: { final TextDocumentItem doc = value.getOpenDocument().getTextDocument(); PyObject completer = - (PyObject) scriptSession.get().getVariableProvider().getVariable("jedi_settings", null); + (PyObject) scriptSession.get().getQueryScope().readParamValue("jedi_settings"); completer.callMethod("open_doc", doc.getText(), doc.getUri(), doc.getVersion()); break; } @@ -57,7 +57,7 @@ public void onNext(AutoCompleteRequest value) { final VersionedTextDocumentIdentifier text = request.getTextDocument(); PyObject completer = - (PyObject) scriptSession.get().getVariableProvider().getVariable("jedi_settings", null); + (PyObject) scriptSession.get().getQueryScope().readParamValue("jedi_settings"); String uri = text.getUri(); int version = text.getVersion(); String document = completer.callMethod("get_doc", text.getUri()).getStringValue(); @@ -76,7 +76,7 @@ public void onNext(AutoCompleteRequest value) { } case CLOSE_DOCUMENT: { PyObject completer = - (PyObject) scriptSession.get().getVariableProvider().getVariable("jedi_settings", null); + (PyObject) scriptSession.get().getQueryScope().readParamValue("jedi_settings"); CloseDocumentRequest request = value.getCloseDocument(); completer.callMethod("close_doc", request.getTextDocument().getUri()); break; @@ -111,7 +111,7 @@ private void handleAutocompleteRequest(AutoCompleteRequest request, request.getRequestId() > 0 ? request.getRequestId() : request.getGetCompletionItems().getRequestId(); try { final ScriptSession scriptSession = exportedConsole.get(); - PyObject completer = scriptSession.getVariableProvider().getVariable("jedi_settings", null); + PyObject completer = scriptSession.getQueryScope().readParamValue("jedi_settings"); boolean canJedi = completer.callMethod("is_enabled").getBooleanValue(); if (!canJedi) { log.trace().append("Ignoring completion request because jedi is disabled").endl(); diff --git a/server/src/test/java/io/deephaven/server/appmode/ApplicationServiceGrpcImplTest.java b/server/src/test/java/io/deephaven/server/appmode/ApplicationServiceGrpcImplTest.java index 35ba754d59f..b3cd17e6c54 100644 --- a/server/src/test/java/io/deephaven/server/appmode/ApplicationServiceGrpcImplTest.java +++ b/server/src/test/java/io/deephaven/server/appmode/ApplicationServiceGrpcImplTest.java @@ -89,7 +89,7 @@ public void onListFieldsSubscribeFailedObserver() { // trigger a change ScriptSession scriptSession = new NoLanguageDeephavenSession( ExecutionContext.getDefaultContext().getUpdateGraph(), ThreadInitializationFactory.NO_OP); - scriptSession.getVariableProvider().setVariable("key", "hello world"); + scriptSession.getQueryScope().putParam("key", "hello world"); ScriptSession.Changes changes = new ScriptSession.Changes(); changes.created.put("key", "Object"); applicationServiceGrpcImpl.onScopeChanges(scriptSession, changes); diff --git a/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java b/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java index 7483d09a724..adc9ba341be 100644 --- a/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java +++ b/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java @@ -481,7 +481,7 @@ public void testLoginHeaderAnonymous() { final String ANONYMOUS = "Anonymous"; closeClient(); - scriptSession.getVariableProvider().setVariable("test", TableTools.emptyTable(10).update("I=i")); + ExecutionContext.getContext().getQueryScope().putParam("test", TableTools.emptyTable(10).update("I=i")); final MutableBoolean tokenChanged = new MutableBoolean(); flightClient = FlightClient.builder().location(serverLocation) From d309e3532334cd3c59d6e634cb4914cc208adf1c Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Thu, 21 Dec 2023 12:30:36 -0600 Subject: [PATCH 07/23] Remove dead method, dead cleanup code in flight round trip test --- .../engine/util/AbstractScriptSession.java | 10 - .../test/FlightMessageRoundTripTest.java | 178 ++++++++---------- 2 files changed, 83 insertions(+), 105 deletions(-) diff --git a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java index c73224f7e14..35d7ef5ef2c 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java @@ -279,16 +279,6 @@ private Class getVariableType(final String var) { } } - - public TableDefinition getTableDefinition(final String var) { - if (!hasVariableName(var)) { - return null; - } - Object o = getVariable(var); - return o instanceof Table ? ((Table) o).getDefinition() : null; - } - - /** * Retrieve a variable from the script session's bindings. * diff --git a/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java b/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java index adc9ba341be..52703279043 100644 --- a/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java +++ b/server/test/src/main/java/io/deephaven/server/test/FlightMessageRoundTripTest.java @@ -193,14 +193,10 @@ public interface TestComponent { SessionService sessionService(); - AbstractScriptSession scriptSession(); - GrpcServer server(); TestAuthModule.BasicAuthTestImpl basicAuthHandler(); - Map authRequestHandlers(); - ExecutionContext executionContext(); TestAuthorizationProvider authorizationProvider(); @@ -216,7 +212,6 @@ public interface TestComponent { protected SessionService sessionService; private SessionState currentSession; - private AbstractScriptSession scriptSession; private SafeCloseable executionContext; private Location serverLocation; protected TestComponent component; @@ -243,7 +238,6 @@ public void setup() throws IOException { server.start(); localPort = server.getPort(); - scriptSession = component.scriptSession(); sessionService = component.sessionService(); serverLocation = Location.forGrpcInsecure("localhost", localPort); @@ -680,7 +674,6 @@ public void testGetSchema() { final Table tickingTable = ExecutionContext.getContext().getUpdateGraph().sharedLock().computeLocked( () -> TableTools.timeTable(1_000_000).update("I = i")); - // try (final SafeCloseable ignored = LivenessScopeStack.open(scriptSession.getQueryScope(), false)) { // stuff table into the scope ExecutionContext.getContext().getQueryScope().putParam(staticTableName, table); ExecutionContext.getContext().getQueryScope().putParam(tickingTableName, tickingTable); @@ -703,7 +696,6 @@ public void testGetSchema() { }); Assert.eq(seenTables.intValue(), "seenTables.intValue()", 2); - // } } @Test @@ -711,74 +703,72 @@ public void testDoExchangeSnapshot() throws Exception { final String staticTableName = "flightInfoTest"; final Table table = TableTools.emptyTable(10).update("I = i", "J = i + 0.01"); - try (final SafeCloseable ignored = LivenessScopeStack.open(scriptSession.getQueryScope(), false)) { - // stuff table into the scope - ExecutionContext.getContext().getQueryScope().putParam(staticTableName, table); - - // build up a snapshot request - byte[] magic = new byte[] {100, 112, 104, 110}; // equivalent to '0x6E687064' (ASCII "dphn") - - FlightDescriptor fd = FlightDescriptor.command(magic); - - try (FlightClient.ExchangeReaderWriter erw = flightClient.doExchange(fd); - final RootAllocator allocator = new RootAllocator(Integer.MAX_VALUE)) { - - final FlatBufferBuilder metadata = new FlatBufferBuilder(); - - // use 0 for batch size and max message size to use server-side defaults - int optOffset = - BarrageSnapshotOptions.createBarrageSnapshotOptions(metadata, ColumnConversionMode.Stringify, - false, 0, 0); - - final int ticOffset = - BarrageSnapshotRequest.createTicketVector(metadata, - ScopeTicketHelper.nameToBytes(staticTableName)); - BarrageSnapshotRequest.startBarrageSnapshotRequest(metadata); - BarrageSnapshotRequest.addColumns(metadata, 0); - BarrageSnapshotRequest.addViewport(metadata, 0); - BarrageSnapshotRequest.addSnapshotOptions(metadata, optOffset); - BarrageSnapshotRequest.addTicket(metadata, ticOffset); - metadata.finish(BarrageSnapshotRequest.endBarrageSnapshotRequest(metadata)); - - final FlatBufferBuilder wrapper = new FlatBufferBuilder(); - final int innerOffset = wrapper.createByteVector(metadata.dataBuffer()); - wrapper.finish(BarrageMessageWrapper.createBarrageMessageWrapper( - wrapper, - 0x6E687064, // the numerical representation of the ASCII "dphn". - BarrageMessageType.BarrageSnapshotRequest, - innerOffset)); - - // extract the bytes and package them in an ArrowBuf for transmission - byte[] msg = wrapper.sizedByteArray(); - ArrowBuf data = allocator.buffer(msg.length); - data.writeBytes(msg); - - erw.getWriter().putMetadata(data); - erw.getWriter().completed(); - - // read everything from the server (expecting schema message and one data message) - int totalRowCount = 0; - while (erw.getReader().next()) { - final int offset = totalRowCount; - final VectorSchemaRoot root = erw.getReader().getRoot(); - final int rowCount = root.getRowCount(); - totalRowCount += rowCount; - - // check the values against the source table - org.apache.arrow.vector.IntVector iv = - (org.apache.arrow.vector.IntVector) root.getVector(0); - for (int i = 0; i < rowCount; ++i) { - assertEquals("int match:", DataAccessHelpers.getColumn(table, 0).get(offset + i), iv.get(i)); - } - org.apache.arrow.vector.Float8Vector dv = - (org.apache.arrow.vector.Float8Vector) root.getVector(1); - for (int i = 0; i < rowCount; ++i) { - assertEquals("double match: ", DataAccessHelpers.getColumn(table, 1).get(offset + i), - dv.get(i)); - } + // stuff table into the scope + ExecutionContext.getContext().getQueryScope().putParam(staticTableName, table); + + // build up a snapshot request + byte[] magic = new byte[] {100, 112, 104, 110}; // equivalent to '0x6E687064' (ASCII "dphn") + + FlightDescriptor fd = FlightDescriptor.command(magic); + + try (FlightClient.ExchangeReaderWriter erw = flightClient.doExchange(fd); + final RootAllocator allocator = new RootAllocator(Integer.MAX_VALUE)) { + + final FlatBufferBuilder metadata = new FlatBufferBuilder(); + + // use 0 for batch size and max message size to use server-side defaults + int optOffset = + BarrageSnapshotOptions.createBarrageSnapshotOptions(metadata, ColumnConversionMode.Stringify, + false, 0, 0); + + final int ticOffset = + BarrageSnapshotRequest.createTicketVector(metadata, + ScopeTicketHelper.nameToBytes(staticTableName)); + BarrageSnapshotRequest.startBarrageSnapshotRequest(metadata); + BarrageSnapshotRequest.addColumns(metadata, 0); + BarrageSnapshotRequest.addViewport(metadata, 0); + BarrageSnapshotRequest.addSnapshotOptions(metadata, optOffset); + BarrageSnapshotRequest.addTicket(metadata, ticOffset); + metadata.finish(BarrageSnapshotRequest.endBarrageSnapshotRequest(metadata)); + + final FlatBufferBuilder wrapper = new FlatBufferBuilder(); + final int innerOffset = wrapper.createByteVector(metadata.dataBuffer()); + wrapper.finish(BarrageMessageWrapper.createBarrageMessageWrapper( + wrapper, + 0x6E687064, // the numerical representation of the ASCII "dphn". + BarrageMessageType.BarrageSnapshotRequest, + innerOffset)); + + // extract the bytes and package them in an ArrowBuf for transmission + byte[] msg = wrapper.sizedByteArray(); + ArrowBuf data = allocator.buffer(msg.length); + data.writeBytes(msg); + + erw.getWriter().putMetadata(data); + erw.getWriter().completed(); + + // read everything from the server (expecting schema message and one data message) + int totalRowCount = 0; + while (erw.getReader().next()) { + final int offset = totalRowCount; + final VectorSchemaRoot root = erw.getReader().getRoot(); + final int rowCount = root.getRowCount(); + totalRowCount += rowCount; + + // check the values against the source table + org.apache.arrow.vector.IntVector iv = + (org.apache.arrow.vector.IntVector) root.getVector(0); + for (int i = 0; i < rowCount; ++i) { + assertEquals("int match:", DataAccessHelpers.getColumn(table, 0).get(offset + i), iv.get(i)); + } + org.apache.arrow.vector.Float8Vector dv = + (org.apache.arrow.vector.Float8Vector) root.getVector(1); + for (int i = 0; i < rowCount; ++i) { + assertEquals("double match: ", DataAccessHelpers.getColumn(table, 1).get(offset + i), + dv.get(i)); } - assertEquals(table.size(), totalRowCount); } + assertEquals(table.size(), totalRowCount); } } @@ -787,34 +777,32 @@ public void testDoExchangeProtocol() throws Exception { final String staticTableName = "flightInfoTest"; final Table table = TableTools.emptyTable(10).update("I = i", "J = i + 0.01"); - try (final SafeCloseable ignored = LivenessScopeStack.open(scriptSession.getQueryScope(), false)) { - // stuff table into the scope - ExecutionContext.getContext().getQueryScope().putParam(staticTableName, table); - - // java-flight requires us to send a message, but cannot add app metadata, send a dummy message - byte[] empty = new byte[] {}; - final FlightDescriptor fd = FlightDescriptor.command(empty); - try (FlightClient.ExchangeReaderWriter erw = flightClient.doExchange(fd); - final RootAllocator allocator = new RootAllocator(Integer.MAX_VALUE)) { + // stuff table into the scope + ExecutionContext.getContext().getQueryScope().putParam(staticTableName, table); - byte[] msg = new byte[0]; - ArrowBuf data = allocator.buffer(msg.length); - data.writeBytes(msg); + // java-flight requires us to send a message, but cannot add app metadata, send a dummy message + byte[] empty = new byte[] {}; + final FlightDescriptor fd = FlightDescriptor.command(empty); + try (FlightClient.ExchangeReaderWriter erw = flightClient.doExchange(fd); + final RootAllocator allocator = new RootAllocator(Integer.MAX_VALUE)) { - erw.getWriter().putMetadata(data); - erw.getWriter().completed(); + byte[] msg = new byte[0]; + ArrowBuf data = allocator.buffer(msg.length); + data.writeBytes(msg); - Exception exception = assertThrows(FlightRuntimeException.class, () -> { - erw.getReader().next(); - }); + erw.getWriter().putMetadata(data); + erw.getWriter().completed(); - String expectedMessage = "failed to receive Barrage request metadata"; - String actualMessage = exception.getMessage(); + Exception exception = assertThrows(FlightRuntimeException.class, () -> { + erw.getReader().next(); + }); - assertTrue(actualMessage.contains(expectedMessage)); - } + String expectedMessage = "failed to receive Barrage request metadata"; + String actualMessage = exception.getMessage(); + assertTrue(actualMessage.contains(expectedMessage)); } + } @Test From 8b7c82073b6fe2592d0660677b51e2ca3cf8c43a Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Thu, 21 Dec 2023 12:35:22 -0600 Subject: [PATCH 08/23] Remove more dead code, fix potential lock order issue --- .../engine/util/AbstractScriptSession.java | 25 ++++--------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java index 35d7ef5ef2c..48231592582 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java @@ -151,9 +151,6 @@ public synchronized final Changes evaluateScript(final String script, @Nullable RuntimeException evaluateErr = null; final Changes diff; - // Take the write lock while running script code, so that readers can't look at variables again until - // the script has finished. - variableAccessLock.writeLock().lock(); // retain any objects which are created in the executed code, we'll release them when the script session // closes try (final S initialSnapshot = takeSnapshot(); @@ -163,7 +160,11 @@ public synchronized final Changes evaluateScript(final String script, @Nullable // Actually evaluate the script; use the enclosing auth context, since AbstractScriptSession's // ExecutionContext never has a non-null AuthContext executionContext.withAuthContext(ExecutionContext.getContext().getAuthContext()) - .apply(() -> evaluate(script, scriptName)); + .apply(() -> { + // Take the write lock while running script code, so that readers can't look at variables again until + // the script has finished. + doLocked(variableAccessLock.writeLock(), () -> evaluate(script, scriptName)); + }); } catch (final RuntimeException err) { evaluateErr = err; } @@ -172,8 +173,6 @@ public synchronized final Changes evaluateScript(final String script, @Nullable observeScopeChanges(); // Use the "last" snapshot created as a side effect of observeScopeChanges() as our "to" diff = createDiff(initialSnapshot, lastSnapshot, evaluateErr); - } finally { - variableAccessLock.writeLock().unlock(); } return diff; @@ -265,20 +264,6 @@ public QueryScope getQueryScope() { return queryScope; } - private Class getVariableType(final String var) { - if (hasVariableName(var)) { - return null; - } - final Object result = getVariable(var); - if (result == null) { - return null; - } else if (result instanceof Table) { - return Table.class; - } else { - return result.getClass(); - } - } - /** * Retrieve a variable from the script session's bindings. * From bd2abccc4ec0e66c83fae123050064753c751d58 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Thu, 21 Dec 2023 12:37:31 -0600 Subject: [PATCH 09/23] Field ordering --- .../io/deephaven/engine/util/AbstractScriptSession.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java index 48231592582..0a3e6e428f7 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java @@ -68,17 +68,16 @@ private static void createOrClearDirectory(final File directory) { } } - private final File classCacheDirectory; - - protected final ExecutionContext executionContext; + private final ReadWriteLock variableAccessLock = new ReentrantReadWriteLock(); private final ObjectTypeLookup objectTypeLookup; private final Listener changeListener; + private final File classCacheDirectory; + private final QueryScope queryScope; - private final ReadWriteLock variableAccessLock = new ReentrantReadWriteLock(); + protected final ExecutionContext executionContext; private S lastSnapshot; - private final QueryScope queryScope; protected AbstractScriptSession( UpdateGraph updateGraph, From 8f8102e20d722ea2de948ee838492f5549cb2590 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Thu, 21 Dec 2023 12:52:08 -0600 Subject: [PATCH 10/23] Correct comment layout --- .../java/io/deephaven/engine/util/AbstractScriptSession.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java index 0a3e6e428f7..fa18c2023b1 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java @@ -160,8 +160,8 @@ public synchronized final Changes evaluateScript(final String script, @Nullable // ExecutionContext never has a non-null AuthContext executionContext.withAuthContext(ExecutionContext.getContext().getAuthContext()) .apply(() -> { - // Take the write lock while running script code, so that readers can't look at variables again until - // the script has finished. + // Take the write lock while running script code, so that readers can't look at variables + // again until the script has finished. doLocked(variableAccessLock.writeLock(), () -> evaluate(script, scriptName)); }); } catch (final RuntimeException err) { From 4efb27e6d564301fdc6c7c2bb07c5e4779315374 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Fri, 22 Dec 2023 09:18:57 -0600 Subject: [PATCH 11/23] Review feedback: * Synchronize access to groovy variables * unmanage items removed from query scope * Ensure the script session's query scope is active when executing code --- .../python/PythonDeephavenSession.java | 26 ++++--- .../engine/context/ExecutionContext.java | 16 ++++ .../deephaven/engine/context/QueryScope.java | 10 ++- .../engine/util/AbstractScriptSession.java | 78 ++++++------------- .../engine/util/GroovyDeephavenSession.java | 33 ++++---- .../util/NoLanguageDeephavenSession.java | 15 ++-- 6 files changed, 92 insertions(+), 86 deletions(-) diff --git a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java index f3a97999a17..992d1bf6790 100644 --- a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java +++ b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java @@ -39,10 +39,8 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; -import java.util.Collections; -import java.util.LinkedHashMap; -import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -171,10 +169,9 @@ private void runScript(String script) throws IOException { @SuppressWarnings("unchecked") @Override - protected T getVariable(String name) throws QueryScope.MissingVariableException { - return (T) scope - .getValue(name) - .orElseThrow(() -> new QueryScope.MissingVariableException("No variable for: " + name)); + protected Optional getVariable(String name) throws QueryScope.MissingVariableException { + return (Optional) scope + .getValue(name); } @@ -278,24 +275,33 @@ protected boolean hasVariableName(String name) { } @Override - protected synchronized void setVariable(String name, @Nullable Object newValue) { + protected synchronized Object setVariable(String name, @Nullable Object newValue) { final PyDictWrapper globals = scope.mainGlobals(); + + Object old; if (newValue == null) { try { - globals.delItem(name); + old = globals.unwrap().callMethod("pop", name); } catch (KeyError key) { - // ignore + old = null; } } else { if (!(newValue instanceof PyObject)) { newValue = PythonObjectWrapper.wrap(newValue); } + // This isn't thread safe, we're relying on the GIL being kind to us (as we have historically done). There + // is no built-in for "replace a variable and return the old one". + old = globals.get(name); globals.setItem(name, newValue); } // Observe changes from this "setVariable" (potentially capturing previous or concurrent external changes from // other threads) observeScopeChanges(); + + // This doesn't return the same Java instance of PyObject, so we won't decref it properly, but + // again, that is consistent with how we've historically treated these references. + return old; } @Override diff --git a/engine/context/src/main/java/io/deephaven/engine/context/ExecutionContext.java b/engine/context/src/main/java/io/deephaven/engine/context/ExecutionContext.java index a698a85b841..52b750ec143 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/ExecutionContext.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/ExecutionContext.java @@ -172,6 +172,22 @@ public ExecutionContext withOperationInitializer(final OperationInitializer oper operationInitializer); } + /** + * Returns, or creates, an execution context with the given value for {@code queryScope} and existing values for the + * other members. + * + * @param queryScope the query scope to use instead + * @return the execution context + */ + public ExecutionContext withQueryScope(QueryScope queryScope) { + if (queryScope == this.queryScope) { + return this; + } + return new ExecutionContext(isSystemic, authContext, queryLibrary, queryScope, queryCompiler, updateGraph, + operationInitializer); + } + + /** * Execute runnable within this execution context. */ diff --git a/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java index 21370076c8c..0abc1f966b6 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java @@ -278,7 +278,15 @@ public void putParam(final String name, final T value) { } NameValidator.validateQueryParameterName(name); // TODO: Can I get rid of this applyValueConversions? It's too inconsistent to feel safe. - valueRetrievers.put(name, new SimpleValueRetriever<>(name, applyValueConversions(value))); + ValueRetriever oldValueRetriever = + valueRetrievers.put(name, new SimpleValueRetriever<>(name, applyValueConversions(value))); + + if (oldValueRetriever != null) { + Object oldValue = oldValueRetriever.getValue(); + if (oldValue instanceof LivenessReferent && DynamicNode.notDynamicOrIsRefreshing(oldValue)) { + unmanage((LivenessReferent) oldValue); + } + } } private static abstract class ValueRetriever { diff --git a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java index fa18c2023b1..5fedea3c744 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java @@ -15,7 +15,6 @@ import io.deephaven.engine.liveness.LivenessScopeStack; import io.deephaven.engine.table.PartitionedTable; import io.deephaven.engine.table.Table; -import io.deephaven.engine.table.TableDefinition; import io.deephaven.engine.context.QueryScope; import io.deephaven.engine.context.QueryScopeParam; import io.deephaven.engine.table.hierarchical.HierarchicalTable; @@ -26,7 +25,6 @@ import io.deephaven.plugin.type.ObjectTypeLookup; import io.deephaven.util.SafeCloseable; import io.deephaven.util.thread.ThreadInitializationFactory; -import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import java.io.File; @@ -37,10 +35,6 @@ import java.util.Optional; import java.util.Set; import java.util.UUID; -import java.util.concurrent.locks.Lock; -import java.util.concurrent.locks.ReadWriteLock; -import java.util.concurrent.locks.ReentrantReadWriteLock; -import java.util.function.Supplier; import static io.deephaven.engine.table.Table.NON_DISPLAY_TABLE; @@ -68,8 +62,6 @@ private static void createOrClearDirectory(final File directory) { } } - private final ReadWriteLock variableAccessLock = new ReentrantReadWriteLock(); - private final ObjectTypeLookup objectTypeLookup; private final Listener changeListener; private final File classCacheDirectory; @@ -159,11 +151,8 @@ public synchronized final Changes evaluateScript(final String script, @Nullable // Actually evaluate the script; use the enclosing auth context, since AbstractScriptSession's // ExecutionContext never has a non-null AuthContext executionContext.withAuthContext(ExecutionContext.getContext().getAuthContext()) - .apply(() -> { - // Take the write lock while running script code, so that readers can't look at variables - // again until the script has finished. - doLocked(variableAccessLock.writeLock(), () -> evaluate(script, scriptName)); - }); + .withQueryScope(queryScope) + .apply(() -> evaluate(script, scriptName)); } catch (final RuntimeException err) { evaluateErr = err; } @@ -264,23 +253,22 @@ public QueryScope getQueryScope() { } /** - * Retrieve a variable from the script session's bindings. + * Retrieve a variable from the script session's bindings. Values may need to be unwrapped. * * @param name the variable to retrieve - * @return the variable - * @throws QueryScope.MissingVariableException if the variable does not exist + * @return the variable value, or empty if not present */ - protected abstract T getVariable(String name) throws QueryScope.MissingVariableException; + protected abstract Optional getVariable(String name); /** - * Retrieves all of the variable names present in the session's scope + * Retrieves all variable names present in the session's scope. * * @return an immutable set of variable names */ protected abstract Set getVariableNames(); /** - * Check if the scope has the given variable name + * Check if the scope has the given variable name. * * @param name the variable name * @return True iff the scope has the given variable name @@ -292,26 +280,10 @@ public QueryScope getQueryScope() { * * @param name the variable name to set * @param value the new value of the variable + * @return the old previous value for this name, if any. As with {@link #getVariable(String)}, may need to be + * unwrapped. */ - protected abstract void setVariable(String name, @Nullable Object value); - - private static void doLocked(@NotNull final Lock lock, @NotNull final Runnable runnable) { - lock.lock(); - try { - runnable.run(); - } finally { - lock.unlock(); - } - } - - private static T doLocked(@NotNull final Lock lock, @NotNull final Supplier supplier) { - lock.lock(); - try { - return supplier.get(); - } finally { - lock.unlock(); - } - } + protected abstract Object setVariable(String name, @Nullable Object value); // ----------------------------------------------------------------------------------------------------------------- // ScriptSession-based QueryScope implementation, with no remote scope or object reflection support @@ -328,8 +300,7 @@ public ScriptSession scriptSession() { @Override public Set getParamNames() { final Set result = new LinkedHashSet<>(); - Set variables = - doLocked(variableAccessLock.readLock(), AbstractScriptSession.this::getVariableNames); + Set variables = AbstractScriptSession.this.getVariableNames(); for (final String name : variables) { if (NameValidator.isValidQueryParameterName(name)) { result.add(name); @@ -340,8 +311,7 @@ public Set getParamNames() { @Override public boolean hasParamName(String name) { - return NameValidator.isValidQueryParameterName(name) - && doLocked(variableAccessLock.readLock(), () -> AbstractScriptSession.this.hasVariableName(name)); + return NameValidator.isValidQueryParameterName(name) && hasVariableName(name); } @Override @@ -358,15 +328,8 @@ public T readParamValue(final String name) throws QueryScope.MissingVariable if (!NameValidator.isValidQueryParameterName(name)) { throw new QueryScope.MissingVariableException("Name " + name + " is invalid"); } - variableAccessLock.readLock().lock(); - try { - if (!hasVariableName(name)) { - throw new MissingVariableException("Missing variable " + name); - } - return AbstractScriptSession.this.getVariable(name); - } finally { - variableAccessLock.readLock().unlock(); - } + // noinspection unchecked + return (T) getVariable(name).orElseThrow(() -> new MissingVariableException("Missing variable " + name)); } @Override @@ -374,7 +337,8 @@ public T readParamValue(final String name, final T defaultValue) { if (!NameValidator.isValidQueryParameterName(name)) { return defaultValue; } - return doLocked(variableAccessLock.readLock(), () -> AbstractScriptSession.this.getVariable(name)); + // noinspection unchecked + return (T) getVariable(name).orElse(defaultValue); } @Override @@ -383,7 +347,15 @@ public void putParam(final String name, final T value) { if (value instanceof LivenessReferent && DynamicNode.notDynamicOrIsRefreshing(value)) { manage((LivenessReferent) value); } - doLocked(variableAccessLock.writeLock(), () -> AbstractScriptSession.this.setVariable(name, value)); + + Object oldValue = AbstractScriptSession.this.setVariable(name, value); + + Object unwrappedOldValue = unwrapObject(oldValue); + + if (unwrappedOldValue instanceof LivenessReferent + && DynamicNode.notDynamicOrIsRefreshing(unwrappedOldValue)) { + unmanage((LivenessReferent) unwrappedOldValue); + } } @Override diff --git a/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java b/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java index 8b4c27884eb..8feea7a5e00 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java @@ -7,7 +7,6 @@ import groovy.lang.Binding; import groovy.lang.GroovyClassLoader; import groovy.lang.GroovyShell; -import groovy.lang.MissingPropertyException; import io.deephaven.api.agg.Aggregation; import io.deephaven.api.updateby.BadDataBehavior; import io.deephaven.api.updateby.DeltaControl; @@ -134,6 +133,7 @@ protected Class loadClass(String name, boolean resolve) throws ClassNotFoundE private final ImportCustomizer loadedGroovyScriptImports = new ImportCustomizer(); private final Set dynamicClasses = new HashSet<>(); + private final Map bindingBackingMap = Collections.synchronizedMap(new LinkedHashMap<>()); private final GroovyShell groovyShell; private int counter; @@ -176,8 +176,8 @@ public GroovyDeephavenSession( consoleConfig.getCompilationCustomizers().add(consoleImports); consoleConfig.setTargetDirectory(executionContext.getQueryCompiler().getFakeClassDestination()); - - groovyShell = new GroovyShell(scriptClassLoader, consoleConfig) { + Binding binding = new Binding(bindingBackingMap); + groovyShell = new GroovyShell(scriptClassLoader, binding, consoleConfig) { protected synchronized String generateScriptName() { return GroovyDeephavenSession.this.generateScriptName(); } @@ -276,12 +276,13 @@ public void runScriptOnce(String script) throws IOException { } @Override - protected T getVariable(String name) throws QueryScope.MissingVariableException { - try { - // noinspection unchecked - return (T) groovyShell.getContext().getVariable(name); - } catch (MissingPropertyException mpe) { - throw new QueryScope.MissingVariableException("No binding for: " + name, mpe); + protected Optional getVariable(String name) { + synchronized (bindingBackingMap) { + if (bindingBackingMap.containsKey(name)) { + // noinspection unchecked + return Optional.of((T) bindingBackingMap.get(name)); + } + return Optional.empty(); } } @@ -740,22 +741,26 @@ public void close() { @Override protected Set getVariableNames() { - // noinspection unchecked - return Set.copyOf(groovyShell.getContext().getVariables().keySet()); + synchronized (bindingBackingMap) { + return Set.copyOf(bindingBackingMap.keySet()); + } } @Override protected boolean hasVariableName(String name) { - return groovyShell.getContext().hasVariable(name); + return bindingBackingMap.containsKey(name); } @Override - protected void setVariable(String name, @Nullable Object newValue) { - groovyShell.getContext().setVariable(NameValidator.validateQueryParameterName(name), newValue); + protected Object setVariable(String name, @Nullable Object newValue) { + NameValidator.validateQueryParameterName(name); + + Object oldValue = bindingBackingMap.put(name, newValue); // Observe changes from this "setVariable" (potentially capturing previous or concurrent external changes from // other threads) observeScopeChanges(); + return oldValue; } public Binding getBinding() { diff --git a/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java b/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java index 0e9451808e5..ae498d26151 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java @@ -3,15 +3,14 @@ */ package io.deephaven.engine.util; -import io.deephaven.engine.context.QueryScope; import io.deephaven.engine.updategraph.UpdateGraph; import io.deephaven.util.thread.ThreadInitializationFactory; -import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import java.util.Collections; import java.util.LinkedHashMap; import java.util.Map; +import java.util.Optional; import java.util.Set; /** @@ -34,16 +33,16 @@ public NoLanguageDeephavenSession(final UpdateGraph updateGraph, super(updateGraph, threadInitializationFactory, null, null); this.scriptType = scriptType; - variables = new LinkedHashMap<>(); + variables = Collections.synchronizedMap(new LinkedHashMap<>()); } @Override - protected T getVariable(String name) throws QueryScope.MissingVariableException { + protected Optional getVariable(String name) { if (!variables.containsKey(name)) { - throw new QueryScope.MissingVariableException("No global variable for: " + name); + return Optional.empty(); } // noinspection unchecked - return (T) variables.get(name); + return Optional.of((T) variables.get(name)); } @Override @@ -80,8 +79,8 @@ protected boolean hasVariableName(String name) { } @Override - protected void setVariable(String name, @Nullable Object newValue) { - variables.put(name, newValue); + protected Object setVariable(String name, @Nullable Object newValue) { + return variables.put(name, newValue); // changeListener is always null for NoLanguageDeephavenSession; we have no mechanism for reporting scope // changes } From c04947685676557529d36a9444b17b52a3291677 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Fri, 22 Dec 2023 11:46:59 -0600 Subject: [PATCH 12/23] Revert attempt to use optional (incompatible with null returns) --- .../python/PythonDeephavenSession.java | 7 ++++--- .../engine/util/AbstractScriptSession.java | 18 ++++++++++++------ .../engine/util/GroovyDeephavenSession.java | 6 +++--- .../util/NoLanguageDeephavenSession.java | 8 ++++---- 4 files changed, 23 insertions(+), 16 deletions(-) diff --git a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java index 992d1bf6790..72aa6e21530 100644 --- a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java +++ b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java @@ -169,9 +169,10 @@ private void runScript(String script) throws IOException { @SuppressWarnings("unchecked") @Override - protected Optional getVariable(String name) throws QueryScope.MissingVariableException { - return (Optional) scope - .getValue(name); + protected T getVariable(String name) throws QueryScope.MissingVariableException { + return (T) scope + .getValue(name) + .orElseThrow(() -> new QueryScope.MissingVariableException("Missing variable " + name)); } diff --git a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java index 5fedea3c744..d48dbbc2b59 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java @@ -255,10 +255,11 @@ public QueryScope getQueryScope() { /** * Retrieve a variable from the script session's bindings. Values may need to be unwrapped. * - * @param name the variable to retrieve - * @return the variable value, or empty if not present + * @param name the name of the variable to retrieve + * @return the variable value + * @throws QueryScope.MissingVariableException if the variable does not exist */ - protected abstract Optional getVariable(String name); + protected abstract T getVariable(String name) throws QueryScope.MissingVariableException; /** * Retrieves all variable names present in the session's scope. @@ -329,7 +330,7 @@ public T readParamValue(final String name) throws QueryScope.MissingVariable throw new QueryScope.MissingVariableException("Name " + name + " is invalid"); } // noinspection unchecked - return (T) getVariable(name).orElseThrow(() -> new MissingVariableException("Missing variable " + name)); + return (T) getVariable(name); } @Override @@ -337,8 +338,13 @@ public T readParamValue(final String name, final T defaultValue) { if (!NameValidator.isValidQueryParameterName(name)) { return defaultValue; } - // noinspection unchecked - return (T) getVariable(name).orElse(defaultValue); + + try { + // noinspection unchecked + return (T) getVariable(name); + } catch (MissingVariableException e) { + return defaultValue; + } } @Override diff --git a/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java b/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java index 8feea7a5e00..4603c17f7ee 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java @@ -276,13 +276,13 @@ public void runScriptOnce(String script) throws IOException { } @Override - protected Optional getVariable(String name) { + protected T getVariable(String name) { synchronized (bindingBackingMap) { if (bindingBackingMap.containsKey(name)) { // noinspection unchecked - return Optional.of((T) bindingBackingMap.get(name)); + return (T) bindingBackingMap.get(name); } - return Optional.empty(); + throw new QueryScope.MissingVariableException("Missing variable " + name); } } diff --git a/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java b/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java index ae498d26151..5172c230de5 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java @@ -3,6 +3,7 @@ */ package io.deephaven.engine.util; +import io.deephaven.engine.context.QueryScope; import io.deephaven.engine.updategraph.UpdateGraph; import io.deephaven.util.thread.ThreadInitializationFactory; import org.jetbrains.annotations.Nullable; @@ -10,7 +11,6 @@ import java.util.Collections; import java.util.LinkedHashMap; import java.util.Map; -import java.util.Optional; import java.util.Set; /** @@ -37,12 +37,12 @@ public NoLanguageDeephavenSession(final UpdateGraph updateGraph, } @Override - protected Optional getVariable(String name) { + protected T getVariable(String name) { if (!variables.containsKey(name)) { - return Optional.empty(); + throw new QueryScope.MissingVariableException("Missing variable " + name); } // noinspection unchecked - return Optional.of((T) variables.get(name)); + return (T) variables.get(name); } @Override From deb0a77c473d7982b50e793df2bffa40477021c5 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Mon, 8 Jan 2024 12:18:38 -0600 Subject: [PATCH 13/23] Review feedback --- .../python/PythonDeephavenSession.java | 46 +++++++++++-------- .../deephaven/engine/context/QueryScope.java | 19 ++++++-- .../table/impl/lang/QueryLanguageParser.java | 4 +- .../engine/util/AbstractScriptSession.java | 15 ++---- .../util/NoLanguageDeephavenSession.java | 14 ++++-- .../engine/table/impl/FuzzerTest.java | 2 +- .../server/appmode/ApplicationTest.java | 1 - 7 files changed, 59 insertions(+), 42 deletions(-) diff --git a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java index 72aa6e21530..1979cbe8601 100644 --- a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java +++ b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java @@ -30,6 +30,7 @@ import org.jpy.KeyError; import org.jpy.PyDictWrapper; import org.jpy.PyInputMode; +import org.jpy.PyLib; import org.jpy.PyLib.CallableKind; import org.jpy.PyModule; import org.jpy.PyObject; @@ -45,6 +46,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.function.Consumer; +import java.util.function.Predicate; import java.util.stream.Collectors; /** @@ -266,8 +268,8 @@ private Object maybeUnwrap(PyObject o) { } @Override - protected Set getVariableNames() { - return scope.getKeys().collect(Collectors.toUnmodifiableSet()); + protected Set getVariableNames(Predicate allowName) { + return PyLib.ensureGil(() -> scope.getKeys().filter(allowName).collect(Collectors.toUnmodifiableSet())); } @Override @@ -277,24 +279,30 @@ protected boolean hasVariableName(String name) { @Override protected synchronized Object setVariable(String name, @Nullable Object newValue) { - final PyDictWrapper globals = scope.mainGlobals(); - - Object old; - if (newValue == null) { - try { - old = globals.unwrap().callMethod("pop", name); - } catch (KeyError key) { - old = null; - } - } else { - if (!(newValue instanceof PyObject)) { - newValue = PythonObjectWrapper.wrap(newValue); + Object old = PyLib.ensureGil(() -> { + final PyDictWrapper globals = scope.mainGlobals(); + + if (newValue == null) { + try { + return globals.unwrap().callMethod("pop", name); + } catch (KeyError key) { + return null; + } + } else { + Object wrapped; + if (newValue instanceof PyObject) { + wrapped = newValue; + } else { + wrapped = PythonObjectWrapper.wrap(newValue); + } + // This isn't thread safe, we're relying on the GIL being kind to us (as we have historically done). + // There + // is no built-in for "replace a variable and return the old one". + Object prev = globals.get(name); + globals.setItem(name, wrapped); + return prev; } - // This isn't thread safe, we're relying on the GIL being kind to us (as we have historically done). There - // is no built-in for "replace a variable and return the old one". - old = globals.get(name); - globals.setItem(name, newValue); - } + }); // Observe changes from this "setVariable" (potentially capturing previous or concurrent external changes from // other threads) diff --git a/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java index 0abc1f966b6..01de65c4f81 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java @@ -3,8 +3,9 @@ */ package io.deephaven.engine.context; +import io.deephaven.engine.liveness.Liveness; import io.deephaven.engine.liveness.LivenessReferent; -import io.deephaven.engine.liveness.LivenessScope; +import io.deephaven.engine.liveness.ReferenceCountedLivenessNode; import io.deephaven.engine.updategraph.DynamicNode; import io.deephaven.time.DateTimeUtils; import io.deephaven.hash.KeyedObjectHashMap; @@ -24,7 +25,7 @@ /** * Variable scope used to resolve parameter values during query execution. */ -public abstract class QueryScope extends LivenessScope implements LogOutputAppendable { +public abstract class QueryScope extends ReferenceCountedLivenessNode implements LogOutputAppendable { /** * Adds a parameter to the default instance {@link QueryScope}, or updates the value of an existing parameter. @@ -113,6 +114,14 @@ private static Object applyValueConversions(final Object value) { return value; } + protected QueryScope() { + super(false); + + // if (!Liveness.REFERENCE_TRACKING_DISABLED) { + incrementReferenceCount(); + // } + } + // ----------------------------------------------------------------------------------------------------------------- // Scope manipulation helper methods // ----------------------------------------------------------------------------------------------------------------- @@ -229,7 +238,11 @@ public static class StandaloneImpl extends QueryScope { private final KeyedObjectHashMap valueRetrievers = new KeyedObjectHashMap<>(new ValueRetrieverNameKey()); - public StandaloneImpl() {} + @Override + protected void destroy() { + super.destroy(); + valueRetrievers.clear(); + } @Override public Set getParamNames() { diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java index d10b147f18b..71addc2fdaa 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java @@ -2710,10 +2710,10 @@ private void prepareVectorizationArgs(MethodCallExpr n, QueryScope queryScope, E addLiteralArg(expressions[i], argTypes[i], pyCallableWrapper); } else if (expressions[i] instanceof NameExpr) { String name = expressions[i].asNameExpr().getNameAsString(); - if (queryScope.hasParamName(name)) { + try { Object param = queryScope.readParamValue(name); pyCallableWrapper.addChunkArgument(new ConstantChunkArgument(param, argTypes[i])); - } else { + } catch (QueryScope.MissingVariableException ex) { // A column name or one of the special variables pyCallableWrapper.addChunkArgument(new ColumnChunkArgument(name, argTypes[i])); } diff --git a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java index d48dbbc2b59..0ee85fb36dc 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java @@ -30,11 +30,10 @@ import java.io.File; import java.io.IOException; import java.nio.file.Path; -import java.util.Collections; -import java.util.LinkedHashSet; import java.util.Optional; import java.util.Set; import java.util.UUID; +import java.util.function.Predicate; import static io.deephaven.engine.table.Table.NON_DISPLAY_TABLE; @@ -264,9 +263,10 @@ public QueryScope getQueryScope() { /** * Retrieves all variable names present in the session's scope. * + * @param allowName a predicate to decide if a name should be in the returned immutable set * @return an immutable set of variable names */ - protected abstract Set getVariableNames(); + protected abstract Set getVariableNames(Predicate allowName); /** * Check if the scope has the given variable name. @@ -300,14 +300,7 @@ public ScriptSession scriptSession() { @Override public Set getParamNames() { - final Set result = new LinkedHashSet<>(); - Set variables = AbstractScriptSession.this.getVariableNames(); - for (final String name : variables) { - if (NameValidator.isValidQueryParameterName(name)) { - result.add(name); - } - } - return Collections.unmodifiableSet(result); + return AbstractScriptSession.this.getVariableNames(NameValidator::isValidQueryParameterName); } @Override diff --git a/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java b/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java index 5172c230de5..ab105c70cb3 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java @@ -38,11 +38,13 @@ public NoLanguageDeephavenSession(final UpdateGraph updateGraph, @Override protected T getVariable(String name) { - if (!variables.containsKey(name)) { - throw new QueryScope.MissingVariableException("Missing variable " + name); + synchronized (variables) { + if (!variables.containsKey(name)) { + throw new QueryScope.MissingVariableException("Missing variable " + name); + } + // noinspection unchecked + return (T) variables.get(name); } - // noinspection unchecked - return (T) variables.get(name); } @Override @@ -70,7 +72,9 @@ protected void evaluate(String command, @Nullable String scriptName) { @Override protected Set getVariableNames() { - return Set.copyOf(variables.keySet()); + synchronized (variables) { + return Set.copyOf(variables.keySet()); + } } @Override diff --git a/engine/table/src/test/java/io/deephaven/engine/table/impl/FuzzerTest.java b/engine/table/src/test/java/io/deephaven/engine/table/impl/FuzzerTest.java index 2893b8374db..a709cae9c75 100644 --- a/engine/table/src/test/java/io/deephaven/engine/table/impl/FuzzerTest.java +++ b/engine/table/src/test/java/io/deephaven/engine/table/impl/FuzzerTest.java @@ -117,7 +117,7 @@ private void testFuzzerScriptFile(final long timeSeed, String s, boolean realtim clock.now += DateTimeUtils.SECOND / 10 * timeRandom.nextInt(20); } - final TimeTable timeTable = session.getQueryScope().readParamValue("tt", null); + final TimeTable timeTable = session.getQueryScope().readParamValue("tt"); final int steps = TstUtils.SHORT_TESTS ? 20 : 100; diff --git a/server/src/test/java/io/deephaven/server/appmode/ApplicationTest.java b/server/src/test/java/io/deephaven/server/appmode/ApplicationTest.java index 01e668e1bbf..19f222fd2c2 100644 --- a/server/src/test/java/io/deephaven/server/appmode/ApplicationTest.java +++ b/server/src/test/java/io/deephaven/server/appmode/ApplicationTest.java @@ -34,7 +34,6 @@ public class ApplicationTest { @After public void tearDown() { if (session != null) { - session.forceReferenceCountToZero(); session = null; } } From c5c1ce799f394cfba099744d778f0ce932523881 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Mon, 8 Jan 2024 13:35:23 -0600 Subject: [PATCH 14/23] Old requested rename, and added missing transferTo --- .../integrations/python/PythonDeephavenSession.java | 2 +- .../deephaven/engine/util/AbstractScriptSession.java | 10 +++++++--- .../deephaven/engine/util/GroovyDeephavenSession.java | 2 +- .../engine/util/NoLanguageDeephavenSession.java | 2 +- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java index 1979cbe8601..eb48a1e6985 100644 --- a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java +++ b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java @@ -273,7 +273,7 @@ protected Set getVariableNames(Predicate allowName) { } @Override - protected boolean hasVariableName(String name) { + protected boolean hasVariable(String name) { return scope.containsKey(name); } diff --git a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java index 0ee85fb36dc..835fb437799 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java @@ -12,6 +12,7 @@ import io.deephaven.engine.context.ExecutionContext; import io.deephaven.engine.liveness.LivenessArtifact; import io.deephaven.engine.liveness.LivenessReferent; +import io.deephaven.engine.liveness.LivenessScope; import io.deephaven.engine.liveness.LivenessScopeStack; import io.deephaven.engine.table.PartitionedTable; import io.deephaven.engine.table.Table; @@ -143,8 +144,9 @@ public synchronized final Changes evaluateScript(final String script, @Nullable // retain any objects which are created in the executed code, we'll release them when the script session // closes + LivenessScope localScope = new LivenessScope(); try (final S initialSnapshot = takeSnapshot(); - final SafeCloseable ignored = LivenessScopeStack.open(queryScope, false)) { + final SafeCloseable ignored = LivenessScopeStack.open(localScope, false)) { try { // Actually evaluate the script; use the enclosing auth context, since AbstractScriptSession's @@ -154,6 +156,8 @@ public synchronized final Changes evaluateScript(final String script, @Nullable .apply(() -> evaluate(script, scriptName)); } catch (final RuntimeException err) { evaluateErr = err; + } finally { + localScope.transferTo(queryScope); } // Observe changes during this evaluation (potentially capturing external changes from other threads) @@ -274,7 +278,7 @@ public QueryScope getQueryScope() { * @param name the variable name * @return True iff the scope has the given variable name */ - protected abstract boolean hasVariableName(String name); + protected abstract boolean hasVariable(String name); /** * Inserts a value into the script's scope. @@ -305,7 +309,7 @@ public Set getParamNames() { @Override public boolean hasParamName(String name) { - return NameValidator.isValidQueryParameterName(name) && hasVariableName(name); + return NameValidator.isValidQueryParameterName(name) && hasVariable(name); } @Override diff --git a/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java b/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java index 4603c17f7ee..dac111c2cd5 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java @@ -747,7 +747,7 @@ protected Set getVariableNames() { } @Override - protected boolean hasVariableName(String name) { + protected boolean hasVariable(String name) { return bindingBackingMap.containsKey(name); } diff --git a/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java b/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java index ab105c70cb3..71e28e792f2 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java @@ -78,7 +78,7 @@ protected Set getVariableNames() { } @Override - protected boolean hasVariableName(String name) { + protected boolean hasVariable(String name) { return variables.containsKey(name); } From 42ba9becc283548fd47c1f271cc14509a2fa091f Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Mon, 8 Jan 2024 13:54:20 -0600 Subject: [PATCH 15/23] Finish implementation (don't trust IJ's refactor tools..?) --- .../io/deephaven/engine/util/GroovyDeephavenSession.java | 5 +++-- .../deephaven/engine/util/NoLanguageDeephavenSession.java | 6 ++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java b/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java index dac111c2cd5..e374c02339c 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java @@ -71,6 +71,7 @@ import java.time.ZonedDateTime; import java.util.*; import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Predicate; import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Collectors; @@ -740,9 +741,9 @@ public void close() { } @Override - protected Set getVariableNames() { + protected Set getVariableNames(Predicate allowName) { synchronized (bindingBackingMap) { - return Set.copyOf(bindingBackingMap.keySet()); + return bindingBackingMap.keySet().stream().filter(allowName).collect(Collectors.toUnmodifiableSet()); } } diff --git a/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java b/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java index 71e28e792f2..3ed47efd40a 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java @@ -12,6 +12,8 @@ import java.util.LinkedHashMap; import java.util.Map; import java.util.Set; +import java.util.function.Predicate; +import java.util.stream.Collectors; /** * ScriptSession implementation that simply allows variables to be exported. This is not intended for use in user @@ -71,9 +73,9 @@ protected void evaluate(String command, @Nullable String scriptName) { } @Override - protected Set getVariableNames() { + protected Set getVariableNames(Predicate allowName) { synchronized (variables) { - return Set.copyOf(variables.keySet()); + return variables.keySet().stream().filter(allowName).collect(Collectors.toUnmodifiableSet()); } } From 0b6caa884ead8cc099d76120ed975551e04ceee9 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Mon, 8 Jan 2024 18:55:24 -0600 Subject: [PATCH 16/23] Review feedback --- .../src/main/java/io/deephaven/engine/context/QueryScope.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java index 01de65c4f81..268aa334444 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java @@ -117,9 +117,7 @@ private static Object applyValueConversions(final Object value) { protected QueryScope() { super(false); - // if (!Liveness.REFERENCE_TRACKING_DISABLED) { - incrementReferenceCount(); - // } + retainReference(); } // ----------------------------------------------------------------------------------------------------------------- From cb8646ded2eb65b90e1379b82b50e5a45f9c264a Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Thu, 11 Jan 2024 14:18:15 -0600 Subject: [PATCH 17/23] Live code review --- .../engine/context/EmptyQueryScope.java | 4 +- .../engine/context/ExecutionContext.java | 4 +- .../engine/context/PoisonedQueryScope.java | 4 +- .../deephaven/engine/context/QueryScope.java | 94 ++++--------------- .../engine/util/AbstractScriptSession.java | 11 +-- .../ChunkerCompletionHandlerTest.groovy | 10 +- ...lumnExpressionCompletionHandlerTest.groovy | 8 +- 7 files changed, 35 insertions(+), 100 deletions(-) diff --git a/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java index 368a7d38926..bc438179e55 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java @@ -6,7 +6,7 @@ import java.util.Collections; import java.util.Set; -public class EmptyQueryScope extends QueryScope { +public class EmptyQueryScope implements QueryScope { public final static EmptyQueryScope INSTANCE = new EmptyQueryScope(); private EmptyQueryScope() {} @@ -22,7 +22,7 @@ public boolean hasParamName(String name) { } @Override - protected QueryScopeParam createParam(String name) throws MissingVariableException { + public QueryScopeParam createParam(String name) throws MissingVariableException { throw new MissingVariableException("Missing variable " + name); } diff --git a/engine/context/src/main/java/io/deephaven/engine/context/ExecutionContext.java b/engine/context/src/main/java/io/deephaven/engine/context/ExecutionContext.java index d84c5d30119..b135dd4d1df 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/ExecutionContext.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/ExecutionContext.java @@ -349,7 +349,7 @@ public Builder emptyQueryScope() { */ @ScriptApi public Builder newQueryScope() { - this.queryScope = new QueryScope.StandaloneImpl(); + this.queryScope = new QueryScope.StandaloneQueryScope(); return this; } @@ -387,7 +387,7 @@ public Builder captureQueryScopeVars(String... vars) { return newQueryScope(); } - this.queryScope = new QueryScope.StandaloneImpl(); + this.queryScope = new QueryScope.StandaloneQueryScope(); final QueryScopeParam[] params = getContext().getQueryScope().getParams(Arrays.asList(vars)); for (final QueryScopeParam param : params) { diff --git a/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java index 66cefae2452..b3d3855df39 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java @@ -7,7 +7,7 @@ import java.util.Set; -public class PoisonedQueryScope extends QueryScope { +public class PoisonedQueryScope implements QueryScope { public static final PoisonedQueryScope INSTANCE = new PoisonedQueryScope(); @@ -28,7 +28,7 @@ public boolean hasParamName(String name) { } @Override - protected QueryScopeParam createParam(String name) throws MissingVariableException { + public QueryScopeParam createParam(String name) throws MissingVariableException { return fail(); } diff --git a/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java index 268aa334444..79857c1db0b 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java @@ -3,10 +3,6 @@ */ package io.deephaven.engine.context; -import io.deephaven.engine.liveness.Liveness; -import io.deephaven.engine.liveness.LivenessReferent; -import io.deephaven.engine.liveness.ReferenceCountedLivenessNode; -import io.deephaven.engine.updategraph.DynamicNode; import io.deephaven.time.DateTimeUtils; import io.deephaven.hash.KeyedObjectHashMap; import io.deephaven.hash.KeyedObjectKey; @@ -16,7 +12,6 @@ import io.deephaven.util.QueryConstants; import org.jetbrains.annotations.NotNull; -import java.lang.reflect.Field; import java.time.Duration; import java.time.Instant; import java.time.Period; @@ -25,7 +20,7 @@ /** * Variable scope used to resolve parameter values during query execution. */ -public abstract class QueryScope extends ReferenceCountedLivenessNode implements LogOutputAppendable { +public interface QueryScope extends LogOutputAppendable { /** * Adds a parameter to the default instance {@link QueryScope}, or updates the value of an existing parameter. @@ -34,7 +29,7 @@ public abstract class QueryScope extends ReferenceCountedLivenessNode implements * @param value value to assign to the parameter. * @param type of the parameter/value. */ - public static void addParam(final String name, final T value) { + static void addParam(final String name, final T value) { ExecutionContext.getContext().getQueryScope().putParam(name, value); } @@ -46,7 +41,7 @@ public static void addParam(final String name, final T value) { * @return parameter value. * @throws MissingVariableException variable name is not defined. */ - public static T getParamValue(final String name) throws MissingVariableException { + static T getParamValue(final String name) throws MissingVariableException { return ExecutionContext.getContext().getQueryScope().readParamValue(name); } @@ -58,7 +53,7 @@ public static T getParamValue(final String name) throws MissingVariableExcep * A type of RuntimeException thrown when a variable referenced within the {@link QueryScope} is not defined or, * more likely, has not been added to the scope. */ - public static class MissingVariableException extends RuntimeException { + class MissingVariableException extends RuntimeException { public MissingVariableException(final String message, final Throwable cause) { super(message, cause); @@ -114,12 +109,6 @@ private static Object applyValueConversions(final Object value) { return value; } - protected QueryScope() { - super(false); - - retainReference(); - } - // ----------------------------------------------------------------------------------------------------------------- // Scope manipulation helper methods // ----------------------------------------------------------------------------------------------------------------- @@ -131,7 +120,7 @@ protected QueryScope() { * @return A newly-constructed array of newly-constructed Params. * @throws QueryScope.MissingVariableException If any of the named scope variables does not exist. */ - public final QueryScopeParam[] getParams(final Collection names) throws MissingVariableException { + default QueryScopeParam[] getParams(final Collection names) throws MissingVariableException { final QueryScopeParam[] result = new QueryScopeParam[names.size()]; int pi = 0; for (final String name : names) { @@ -149,7 +138,7 @@ public final QueryScopeParam[] getParams(final Collection names) throws * * @return A collection of scope variable names. */ - public abstract Set getParamNames(); + Set getParamNames(); /** * Check if the scope has the given name. @@ -157,7 +146,7 @@ public final QueryScopeParam[] getParams(final Collection names) throws * @param name param name * @return True iff the scope has the given param name */ - public abstract boolean hasParamName(String name); + boolean hasParamName(String name); /** * Get a QueryScopeParam by name. @@ -166,7 +155,7 @@ public final QueryScopeParam[] getParams(final Collection names) throws * @return newly-constructed QueryScopeParam (name + value-snapshot pair). * @throws QueryScope.MissingVariableException If any of the named scope variables does not exist. */ - protected abstract QueryScopeParam createParam(final String name) throws MissingVariableException; + QueryScopeParam createParam(final String name) throws MissingVariableException; /** * Get the value of a given scope parameter by name. @@ -175,7 +164,7 @@ public final QueryScopeParam[] getParams(final Collection names) throws * @return parameter value. * @throws QueryScope.MissingVariableException If no such scope parameter exists. */ - public abstract T readParamValue(final String name) throws MissingVariableException; + T readParamValue(final String name) throws MissingVariableException; /** * Get the value of a given scope parameter by name. @@ -184,7 +173,7 @@ public final QueryScopeParam[] getParams(final Collection names) throws * @param defaultValue default parameter value. * @return parameter value, or the default parameter value, if the value is not present. */ - public abstract T readParamValue(final String name, final T defaultValue); + T readParamValue(final String name, final T defaultValue); /** * Add a parameter to the scope. @@ -192,7 +181,7 @@ public final QueryScopeParam[] getParams(final Collection names) throws * @param name parameter name. * @param value parameter value. */ - public abstract void putParam(final String name, final T value); + void putParam(final String name, final T value); /** * Asks the session to remove any wrapping that exists on scoped objects so that clients can fetch them. Defaults to @@ -201,7 +190,7 @@ public final QueryScopeParam[] getParams(final Collection names) throws * @param object the scoped object * @return an obj which can be consumed by a client */ - public Object unwrapObject(Object object) { + default Object unwrapObject(Object object) { return object; } @@ -210,7 +199,7 @@ public Object unwrapObject(Object object) { // ----------------------------------------------------------------------------------------------------------------- @Override - public LogOutput append(@NotNull final LogOutput logOutput) { + default LogOutput append(@NotNull final LogOutput logOutput) { logOutput.append('{'); for (final String paramName : getParamNames()) { final Object paramValue = readParamValue(paramName); @@ -231,17 +220,11 @@ public LogOutput append(@NotNull final LogOutput logOutput) { // Map-based implementation, with remote scope and object reflection support // ----------------------------------------------------------------------------------------------------------------- - public static class StandaloneImpl extends QueryScope { + class StandaloneQueryScope implements QueryScope { private final KeyedObjectHashMap valueRetrievers = new KeyedObjectHashMap<>(new ValueRetrieverNameKey()); - @Override - protected void destroy() { - super.destroy(); - valueRetrievers.clear(); - } - @Override public Set getParamNames() { return valueRetrievers.keySet(); @@ -253,7 +236,7 @@ public boolean hasParamName(String name) { } @Override - protected QueryScopeParam createParam(final String name) throws MissingVariableException { + public QueryScopeParam createParam(final String name) throws MissingVariableException { // noinspection unchecked final ValueRetriever valueRetriever = valueRetrievers.get(name); if (valueRetriever == null) { @@ -284,20 +267,9 @@ public T readParamValue(final String name, final T defaultValue) { @Override public void putParam(final String name, final T value) { - if (value instanceof LivenessReferent && DynamicNode.notDynamicOrIsRefreshing(value)) { - manage((LivenessReferent) value); - } NameValidator.validateQueryParameterName(name); // TODO: Can I get rid of this applyValueConversions? It's too inconsistent to feel safe. - ValueRetriever oldValueRetriever = - valueRetrievers.put(name, new SimpleValueRetriever<>(name, applyValueConversions(value))); - - if (oldValueRetriever != null) { - Object oldValue = oldValueRetriever.getValue(); - if (oldValue instanceof LivenessReferent && DynamicNode.notDynamicOrIsRefreshing(oldValue)) { - unmanage((LivenessReferent) oldValue); - } - } + valueRetrievers.put(name, new SimpleValueRetriever<>(name, applyValueConversions(value))); } private static abstract class ValueRetriever { @@ -352,39 +324,5 @@ public QueryScopeParam createParam() { return new QueryScopeParam<>(getName(), getValue()); } } - - private static class ReflectiveValueRetriever extends ValueRetriever { - - private final Object object; - private final Field field; - - public ReflectiveValueRetriever(final Object object, final Field field) { - super(field.getName()); - this.object = object; - this.field = field; - field.setAccessible(true); - } - - @Override - public T getValue() { - try { - // noinspection unchecked - return (T) field.get(object); - } catch (IllegalAccessException e) { - throw new RuntimeException(e); - } - } - - @Override - public Class getType() { - // noinspection unchecked - return (Class) field.getType(); - } - - @Override - public QueryScopeParam createParam() { - return new QueryScopeParam<>(getName(), getValue()); - } - } } } diff --git a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java index 88946cda466..57a96af7c96 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java @@ -64,7 +64,7 @@ private static void createOrClearDirectory(final File directory) { private final ObjectTypeLookup objectTypeLookup; private final Listener changeListener; private final File classCacheDirectory; - private final QueryScope queryScope; + private final ScriptSessionQueryScope queryScope; protected final ExecutionContext executionContext; @@ -143,9 +143,8 @@ public synchronized final Changes evaluateScript(final String script, @Nullable // retain any objects which are created in the executed code, we'll release them when the script session // closes - LivenessScope localScope = new LivenessScope(); try (final S initialSnapshot = takeSnapshot(); - final SafeCloseable ignored = LivenessScopeStack.open(localScope, false)) { + final SafeCloseable ignored = LivenessScopeStack.open(queryScope, false)) { try { // Actually evaluate the script; use the enclosing auth context, since AbstractScriptSession's @@ -155,8 +154,6 @@ public synchronized final Changes evaluateScript(final String script, @Nullable .apply(() -> evaluate(script, scriptName)); } catch (final RuntimeException err) { evaluateErr = err; - } finally { - localScope.transferTo(queryScope); } // Observe changes during this evaluation (potentially capturing external changes from other threads) @@ -293,7 +290,7 @@ public QueryScope getQueryScope() { // ScriptSession-based QueryScope implementation, with no remote scope or object reflection support // ----------------------------------------------------------------------------------------------------------------- - public class ScriptSessionQueryScope extends QueryScope { + public class ScriptSessionQueryScope extends LivenessScope implements QueryScope { /** * Internal workaround to support python calling pushScope. */ @@ -312,7 +309,7 @@ public boolean hasParamName(String name) { } @Override - protected QueryScopeParam createParam(final String name) + public QueryScopeParam createParam(final String name) throws QueryScope.MissingVariableException { if (!NameValidator.isValidQueryParameterName(name)) { throw new QueryScope.MissingVariableException("Name " + name + " is invalid"); diff --git a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerCompletionHandlerTest.groovy b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerCompletionHandlerTest.groovy index 32f3667fff7..32aeb9fbb9f 100644 --- a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerCompletionHandlerTest.groovy +++ b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerCompletionHandlerTest.groovy @@ -55,7 +55,7 @@ class ChunkerCompletionHandlerTest extends Specification implements ChunkerCompl when: doc = p.parse(src) - QueryScope vars = new QueryScope.StandaloneImpl(); + QueryScope vars = new QueryScope.StandaloneQueryScope(); vars.putParam('t', TableFactory.emptyTable(0)) then: @@ -86,7 +86,7 @@ t = emptyTable(10).update( t = emptyTable(10) u = t.''' CompletionParser p = new CompletionParser() - QueryScope vars = new QueryScope.StandaloneImpl(); + QueryScope vars = new QueryScope.StandaloneQueryScope(); vars.putParam('t', TableFactory.emptyTable(0)) when: @@ -122,7 +122,7 @@ u = t.''' doc = p.parse(src) LoggerFactory.getLogger(CompletionHandler) - QueryScope variables = new QueryScope.StandaloneImpl(); + QueryScope variables = new QueryScope.StandaloneQueryScope(); variables.putParam('emptyTable', TableFactory.emptyTable(0)) @@ -148,7 +148,7 @@ c = 3 p.update(uri, 1, [ makeChange(3, 0, src2) ]) doc = p.finish(uri) - QueryScope variables = new QueryScope.StandaloneImpl(); + QueryScope variables = new QueryScope.StandaloneQueryScope(); variables.putParam('emptyTable', TableFactory.emptyTable(0)) when: "Cursor is at EOF, table name completion from t is returned" @@ -176,7 +176,7 @@ b = 2 doc = p.parse(src) LoggerFactory.getLogger(CompletionHandler) - QueryScope variables = new QueryScope.StandaloneImpl(); + QueryScope variables = new QueryScope.StandaloneQueryScope(); variables.putParam('emptyTable', TableFactory.emptyTable(0)) when: "Cursor is in the comment after the variablename+dot and completion is requested" diff --git a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ColumnExpressionCompletionHandlerTest.groovy b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ColumnExpressionCompletionHandlerTest.groovy index 461cb5735c1..a28228274ea 100644 --- a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ColumnExpressionCompletionHandlerTest.groovy +++ b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ColumnExpressionCompletionHandlerTest.groovy @@ -41,7 +41,7 @@ class ColumnExpressionCompletionHandlerTest extends Specification implements Chu doc = p.parse(src) Logger log = LoggerFactory.getLogger(CompletionHandler) - QueryScope variables = new QueryScope.StandaloneImpl() + QueryScope variables = new QueryScope.StandaloneQueryScope() variables.putParam("t", TableFactory.newTable( Column.of('Date', LocalDate.class, new LocalDate[0]), Column.of('DateTime', Instant.class, new Instant[0])) @@ -90,7 +90,7 @@ t = t.updateView ( 'D doc = p.parse(src) Logger log = LoggerFactory.getLogger(CompletionHandler) - QueryScope variables = new QueryScope.StandaloneImpl(); + QueryScope variables = new QueryScope.StandaloneQueryScope(); variables.putParam('t', TableFactory.newTable( Column.of('Date', String.class, new String[0]), Column.of('Delta', Long.class, new Long[0]), @@ -130,7 +130,7 @@ t = t.update('A=') .update( 'B=') doc = p.parse(src) Logger log = LoggerFactory.getLogger(CompletionHandler) - QueryScope variables = new QueryScope.StandaloneImpl() + QueryScope variables = new QueryScope.StandaloneQueryScope() variables.putParam('t', TableFactory.newTable( Column.of('A1', Long.class, new Long[0]), Column.of('A2', Integer.class, new Integer[0]), @@ -188,7 +188,7 @@ t.where('""" doc = p.parse(src) Logger log = LoggerFactory.getLogger(CompletionHandler) - QueryScope variables = new QueryScope.StandaloneImpl() + QueryScope variables = new QueryScope.StandaloneQueryScope() variables.putParam('t', null); ChunkerCompleter completer = new ChunkerCompleter(log, variables) From abb9dcb8d3dc622042621045af02ed06b5674581 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Fri, 12 Jan 2024 11:29:16 -0600 Subject: [PATCH 18/23] Live review feedback --- .../python/PythonDeephavenSession.java | 4 +- .../engine/context/EmptyQueryScope.java | 35 ++++ .../engine/context/ExecutionContext.java | 4 +- .../engine/context/PoisonedQueryScope.java | 34 ++++ .../deephaven/engine/context/QueryScope.java | 168 +---------------- .../engine/context/StandaloneQueryScope.java | 176 ++++++++++++++++++ .../engine/util/AbstractScriptSession.java | 6 +- .../ChunkerCompletionHandlerTest.groovy | 11 +- ...lumnExpressionCompletionHandlerTest.groovy | 9 +- 9 files changed, 270 insertions(+), 177 deletions(-) create mode 100644 engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java diff --git a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java index 007ed4cff8f..4de234de16a 100644 --- a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java +++ b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java @@ -42,7 +42,6 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.Objects; -import java.util.Optional; import java.util.Set; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -302,8 +301,7 @@ protected synchronized Object setVariable(String name, @Nullable Object newValue wrapped = PythonObjectWrapper.wrap(newValue); } // This isn't thread safe, we're relying on the GIL being kind to us (as we have historically done). - // There - // is no built-in for "replace a variable and return the old one". + // There is no built-in for "replace a variable and return the old one". Object prev = globals.get(name); globals.setItem(name, wrapped); return prev; diff --git a/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java index bc438179e55..76e3adfb092 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java @@ -3,8 +3,13 @@ */ package io.deephaven.engine.context; +import io.deephaven.engine.liveness.LivenessReferent; +import org.jetbrains.annotations.NotNull; + +import java.lang.ref.WeakReference; import java.util.Collections; import java.util.Set; +import java.util.stream.Stream; public class EmptyQueryScope implements QueryScope { public final static EmptyQueryScope INSTANCE = new EmptyQueryScope(); @@ -40,4 +45,34 @@ public T readParamValue(String name, T defaultValue) { public void putParam(String name, T value) { throw new IllegalStateException("EmptyQueryScope cannot create parameters"); } + + @Override + public boolean tryManage(@NotNull LivenessReferent referent) { + throw new UnsupportedOperationException("tryManage"); + } + + @Override + public boolean tryUnmanage(@NotNull LivenessReferent referent) { + throw new UnsupportedOperationException("tryUnmanage"); + } + + @Override + public boolean tryUnmanage(@NotNull Stream referents) { + throw new UnsupportedOperationException("tryUnmanage"); + } + + @Override + public boolean tryRetainReference() { + throw new UnsupportedOperationException("tryRetainReference"); + } + + @Override + public void dropReference() { + throw new UnsupportedOperationException("dropReference"); + } + + @Override + public WeakReference getWeakReference() { + throw new UnsupportedOperationException("getWeakReference"); + } } diff --git a/engine/context/src/main/java/io/deephaven/engine/context/ExecutionContext.java b/engine/context/src/main/java/io/deephaven/engine/context/ExecutionContext.java index b135dd4d1df..673ae1ff745 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/ExecutionContext.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/ExecutionContext.java @@ -349,7 +349,7 @@ public Builder emptyQueryScope() { */ @ScriptApi public Builder newQueryScope() { - this.queryScope = new QueryScope.StandaloneQueryScope(); + this.queryScope = new StandaloneQueryScope(); return this; } @@ -387,7 +387,7 @@ public Builder captureQueryScopeVars(String... vars) { return newQueryScope(); } - this.queryScope = new QueryScope.StandaloneQueryScope(); + this.queryScope = new StandaloneQueryScope(); final QueryScopeParam[] params = getContext().getQueryScope().getParams(Arrays.asList(vars)); for (final QueryScopeParam param : params) { diff --git a/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java index b3d3855df39..02b17020ef5 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java @@ -3,9 +3,13 @@ */ package io.deephaven.engine.context; +import io.deephaven.engine.liveness.LivenessReferent; import io.deephaven.util.ExecutionContextRegistrationException; +import org.jetbrains.annotations.NotNull; +import java.lang.ref.WeakReference; import java.util.Set; +import java.util.stream.Stream; public class PoisonedQueryScope implements QueryScope { @@ -46,4 +50,34 @@ public T readParamValue(String name, T defaultValue) { public void putParam(String name, T value) { fail(); } + + @Override + public boolean tryManage(@NotNull LivenessReferent referent) { + return fail(); + } + + @Override + public boolean tryUnmanage(@NotNull LivenessReferent referent) { + return fail(); + } + + @Override + public boolean tryUnmanage(@NotNull Stream referents) { + return fail(); + } + + @Override + public boolean tryRetainReference() { + return fail(); + } + + @Override + public void dropReference() { + fail(); + } + + @Override + public WeakReference getWeakReference() { + return fail(); + } } diff --git a/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java index 79857c1db0b..59d98ce63cb 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java @@ -3,24 +3,19 @@ */ package io.deephaven.engine.context; -import io.deephaven.time.DateTimeUtils; -import io.deephaven.hash.KeyedObjectHashMap; -import io.deephaven.hash.KeyedObjectKey; +import io.deephaven.engine.liveness.LivenessNode; import io.deephaven.base.log.LogOutput; import io.deephaven.base.log.LogOutputAppendable; -import io.deephaven.api.util.NameValidator; -import io.deephaven.util.QueryConstants; import org.jetbrains.annotations.NotNull; -import java.time.Duration; -import java.time.Instant; -import java.time.Period; -import java.util.*; +import java.util.Collection; +import java.util.Objects; +import java.util.Set; /** * Variable scope used to resolve parameter values during query execution. */ -public interface QueryScope extends LogOutputAppendable { +public interface QueryScope extends LivenessNode, LogOutputAppendable { /** * Adds a parameter to the default instance {@link QueryScope}, or updates the value of an existing parameter. @@ -68,47 +63,6 @@ private MissingVariableException(final Throwable cause) { } } - /** - * Apply conversions to certain scope variable values. - * - * @param value value - * @return value, or an appropriately converted substitute. - */ - private static Object applyValueConversions(final Object value) { - if (value instanceof String) { - final String stringValue = (String) value; - - if (stringValue.length() > 0 && stringValue.charAt(0) == '\'' - && stringValue.charAt(stringValue.length() - 1) == '\'') { - final String datetimeString = stringValue.substring(1, stringValue.length() - 1); - - final Instant instant = DateTimeUtils.parseInstantQuiet(datetimeString); - if (instant != null) { - return instant; - } - - final long localTime = DateTimeUtils.parseDurationNanosQuiet(datetimeString); - if (localTime != QueryConstants.NULL_LONG) { - return localTime; - } - - final Period period = DateTimeUtils.parsePeriodQuiet(datetimeString); - if (period != null) { - return period; - } - - final Duration duration = DateTimeUtils.parseDurationQuiet(datetimeString); - if (duration != null) { - return duration; - } - - throw new RuntimeException("Cannot parse datetime/time/period : " + stringValue); - } - } - - return value; - } - // ----------------------------------------------------------------------------------------------------------------- // Scope manipulation helper methods // ----------------------------------------------------------------------------------------------------------------- @@ -176,7 +130,7 @@ default QueryScopeParam[] getParams(final Collection names) throws Missi T readParamValue(final String name, final T defaultValue); /** - * Add a parameter to the scope. + * Add a parameter to the scope. Objects passed in will have their liveness managed by the scope. * * @param name parameter name. * @param value parameter value. @@ -215,114 +169,4 @@ default LogOutput append(@NotNull final LogOutput logOutput) { } return logOutput.nl().append('}'); } - - // ----------------------------------------------------------------------------------------------------------------- - // Map-based implementation, with remote scope and object reflection support - // ----------------------------------------------------------------------------------------------------------------- - - class StandaloneQueryScope implements QueryScope { - - private final KeyedObjectHashMap valueRetrievers = - new KeyedObjectHashMap<>(new ValueRetrieverNameKey()); - - @Override - public Set getParamNames() { - return valueRetrievers.keySet(); - } - - @Override - public boolean hasParamName(String name) { - return valueRetrievers.containsKey(name); - } - - @Override - public QueryScopeParam createParam(final String name) throws MissingVariableException { - // noinspection unchecked - final ValueRetriever valueRetriever = valueRetrievers.get(name); - if (valueRetriever == null) { - throw new MissingVariableException("Missing variable " + name); - } - return valueRetriever.createParam(); - } - - @Override - public T readParamValue(final String name) throws MissingVariableException { - // noinspection unchecked - final ValueRetriever valueRetriever = valueRetrievers.get(name); - if (valueRetriever == null) { - throw new MissingVariableException("Missing variable " + name); - } - return valueRetriever.getValue(); - } - - @Override - public T readParamValue(final String name, final T defaultValue) { - // noinspection unchecked - final ValueRetriever valueRetriever = valueRetrievers.get(name); - if (valueRetriever == null) { - return defaultValue; - } - return valueRetriever.getValue(); - } - - @Override - public void putParam(final String name, final T value) { - NameValidator.validateQueryParameterName(name); - // TODO: Can I get rid of this applyValueConversions? It's too inconsistent to feel safe. - valueRetrievers.put(name, new SimpleValueRetriever<>(name, applyValueConversions(value))); - } - - private static abstract class ValueRetriever { - - private final String name; - - protected ValueRetriever(String name) { - this.name = name; - } - - public String getName() { - return name; - } - - public abstract T getValue(); - - public abstract Class getType(); - - public abstract QueryScopeParam createParam(); - } - - private static class ValueRetrieverNameKey extends KeyedObjectKey.Basic { - - @Override - public String getKey(ValueRetriever valueRetriever) { - return valueRetriever.getName(); - } - } - - private static class SimpleValueRetriever extends ValueRetriever { - - private final T value; - - public SimpleValueRetriever(final String name, final T value) { - super(name); - this.value = value; - } - - @Override - public T getValue() { - return value; - } - - @Override - public Class getType() { - // noinspection unchecked - return (Class) (value != null ? value.getClass() : Object.class); - } - - @Override - public QueryScopeParam createParam() { - return new QueryScopeParam<>(getName(), getValue()); - } - } - } } diff --git a/engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java new file mode 100644 index 00000000000..65fb10c8a57 --- /dev/null +++ b/engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java @@ -0,0 +1,176 @@ +package io.deephaven.engine.context; + +import io.deephaven.api.util.NameValidator; +import io.deephaven.engine.liveness.LivenessArtifact; +import io.deephaven.engine.liveness.LivenessReferent; +import io.deephaven.engine.updategraph.DynamicNode; +import io.deephaven.hash.KeyedObjectHashMap; +import io.deephaven.hash.KeyedObjectKey; +import io.deephaven.time.DateTimeUtils; +import io.deephaven.util.QueryConstants; + +import java.time.Duration; +import java.time.Instant; +import java.time.Period; +import java.util.Set; + +/** + * Map-based implementation, with remote scope and object reflection support. + */ +public class StandaloneQueryScope extends LivenessArtifact implements QueryScope { + + private final KeyedObjectHashMap valueRetrievers = + new KeyedObjectHashMap<>(new ValueRetrieverNameKey()); + + /** + * Apply conversions to certain scope variable values. + * + * @param value value + * @return value, or an appropriately converted substitute. + */ + private static Object applyValueConversions(final Object value) { + if (value instanceof String) { + final String stringValue = (String) value; + + if (stringValue.length() > 0 && stringValue.charAt(0) == '\'' + && stringValue.charAt(stringValue.length() - 1) == '\'') { + final String datetimeString = stringValue.substring(1, stringValue.length() - 1); + + final Instant instant = DateTimeUtils.parseInstantQuiet(datetimeString); + if (instant != null) { + return instant; + } + + final long localTime = DateTimeUtils.parseDurationNanosQuiet(datetimeString); + if (localTime != QueryConstants.NULL_LONG) { + return localTime; + } + + final Period period = DateTimeUtils.parsePeriodQuiet(datetimeString); + if (period != null) { + return period; + } + + final Duration duration = DateTimeUtils.parseDurationQuiet(datetimeString); + if (duration != null) { + return duration; + } + + throw new RuntimeException("Cannot parse datetime/time/period : " + stringValue); + } + } + + return value; + } + + @Override + public Set getParamNames() { + return valueRetrievers.keySet(); + } + + @Override + public boolean hasParamName(String name) { + return valueRetrievers.containsKey(name); + } + + @Override + public QueryScopeParam createParam(final String name) throws MissingVariableException { + // noinspection unchecked + final ValueRetriever valueRetriever = valueRetrievers.get(name); + if (valueRetriever == null) { + throw new MissingVariableException("Missing variable " + name); + } + return valueRetriever.createParam(); + } + + @Override + public T readParamValue(final String name) throws MissingVariableException { + // noinspection unchecked + final ValueRetriever valueRetriever = valueRetrievers.get(name); + if (valueRetriever == null) { + throw new MissingVariableException("Missing variable " + name); + } + return valueRetriever.getValue(); + } + + @Override + public T readParamValue(final String name, final T defaultValue) { + // noinspection unchecked + final ValueRetriever valueRetriever = valueRetrievers.get(name); + if (valueRetriever == null) { + return defaultValue; + } + return valueRetriever.getValue(); + } + + @Override + public void putParam(final String name, final T value) { + if (value instanceof LivenessReferent && DynamicNode.notDynamicOrIsRefreshing(value)) { + manage((LivenessReferent) value); + } + NameValidator.validateQueryParameterName(name); + // TODO: Can I get rid of this applyValueConversions? It's too inconsistent to feel safe. + ValueRetriever oldValueRetriever = + valueRetrievers.put(name, new SimpleValueRetriever<>(name, applyValueConversions(value))); + + if (oldValueRetriever != null) { + Object oldValue = oldValueRetriever.getValue(); + if (oldValue instanceof LivenessReferent && DynamicNode.notDynamicOrIsRefreshing(oldValue)) { + unmanage((LivenessReferent) oldValue); + } + } + } + + private static abstract class ValueRetriever { + + private final String name; + + protected ValueRetriever(String name) { + this.name = name; + } + + public String getName() { + return name; + } + + public abstract T getValue(); + + public abstract Class getType(); + + public abstract QueryScopeParam createParam(); + } + + private static class ValueRetrieverNameKey extends KeyedObjectKey.Basic { + + @Override + public String getKey(ValueRetriever valueRetriever) { + return valueRetriever.getName(); + } + } + + private static class SimpleValueRetriever extends ValueRetriever { + + private final T value; + + public SimpleValueRetriever(final String name, final T value) { + super(name); + this.value = value; + } + + @Override + public T getValue() { + return value; + } + + @Override + public Class getType() { + // noinspection unchecked + return (Class) (value != null ? value.getClass() : Object.class); + } + + @Override + public QueryScopeParam createParam() { + return new QueryScopeParam<>(getName(), getValue()); + } + } +} diff --git a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java index 57a96af7c96..de0899800ca 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java @@ -84,7 +84,6 @@ protected AbstractScriptSession( createOrClearDirectory(classCacheDirectory); queryScope = new ScriptSessionQueryScope(); - manage(queryScope); final QueryCompiler compilerContext = QueryCompiler.create(classCacheDirectory, Thread.currentThread().getContextClassLoader()); @@ -232,6 +231,11 @@ public Changes evaluateScript(Path scriptPath) { @Override protected void destroy() { super.destroy(); + + // Release a reference to the query scope, so if we're the last ones holding the bag, the scope and its contents + // can go away + queryScope.release(); + // Clear our session's script directory: if (classCacheDirectory.exists()) { FileUtils.deleteRecursively(classCacheDirectory); diff --git a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerCompletionHandlerTest.groovy b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerCompletionHandlerTest.groovy index 32aeb9fbb9f..fa07581e499 100644 --- a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerCompletionHandlerTest.groovy +++ b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ChunkerCompletionHandlerTest.groovy @@ -1,6 +1,7 @@ package io.deephaven.lang.completion import io.deephaven.engine.context.QueryScope +import io.deephaven.engine.context.StandaloneQueryScope import io.deephaven.engine.context.TestExecutionContext import io.deephaven.engine.table.Table import io.deephaven.engine.table.TableFactory @@ -55,7 +56,7 @@ class ChunkerCompletionHandlerTest extends Specification implements ChunkerCompl when: doc = p.parse(src) - QueryScope vars = new QueryScope.StandaloneQueryScope(); + QueryScope vars = new StandaloneQueryScope(); vars.putParam('t', TableFactory.emptyTable(0)) then: @@ -86,7 +87,7 @@ t = emptyTable(10).update( t = emptyTable(10) u = t.''' CompletionParser p = new CompletionParser() - QueryScope vars = new QueryScope.StandaloneQueryScope(); + QueryScope vars = new StandaloneQueryScope(); vars.putParam('t', TableFactory.emptyTable(0)) when: @@ -122,7 +123,7 @@ u = t.''' doc = p.parse(src) LoggerFactory.getLogger(CompletionHandler) - QueryScope variables = new QueryScope.StandaloneQueryScope(); + QueryScope variables = new StandaloneQueryScope(); variables.putParam('emptyTable', TableFactory.emptyTable(0)) @@ -148,7 +149,7 @@ c = 3 p.update(uri, 1, [ makeChange(3, 0, src2) ]) doc = p.finish(uri) - QueryScope variables = new QueryScope.StandaloneQueryScope(); + QueryScope variables = new StandaloneQueryScope(); variables.putParam('emptyTable', TableFactory.emptyTable(0)) when: "Cursor is at EOF, table name completion from t is returned" @@ -176,7 +177,7 @@ b = 2 doc = p.parse(src) LoggerFactory.getLogger(CompletionHandler) - QueryScope variables = new QueryScope.StandaloneQueryScope(); + QueryScope variables = new StandaloneQueryScope(); variables.putParam('emptyTable', TableFactory.emptyTable(0)) when: "Cursor is in the comment after the variablename+dot and completion is requested" diff --git a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ColumnExpressionCompletionHandlerTest.groovy b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ColumnExpressionCompletionHandlerTest.groovy index a28228274ea..1d7f95e55ca 100644 --- a/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ColumnExpressionCompletionHandlerTest.groovy +++ b/open-api/lang-tools/src/test/groovy/io/deephaven/lang/completion/ColumnExpressionCompletionHandlerTest.groovy @@ -1,6 +1,7 @@ package io.deephaven.lang.completion import io.deephaven.engine.context.QueryScope +import io.deephaven.engine.context.StandaloneQueryScope import io.deephaven.engine.context.TestExecutionContext; import io.deephaven.engine.table.TableFactory import io.deephaven.internal.log.LoggerFactory @@ -41,7 +42,7 @@ class ColumnExpressionCompletionHandlerTest extends Specification implements Chu doc = p.parse(src) Logger log = LoggerFactory.getLogger(CompletionHandler) - QueryScope variables = new QueryScope.StandaloneQueryScope() + QueryScope variables = new StandaloneQueryScope() variables.putParam("t", TableFactory.newTable( Column.of('Date', LocalDate.class, new LocalDate[0]), Column.of('DateTime', Instant.class, new Instant[0])) @@ -90,7 +91,7 @@ t = t.updateView ( 'D doc = p.parse(src) Logger log = LoggerFactory.getLogger(CompletionHandler) - QueryScope variables = new QueryScope.StandaloneQueryScope(); + QueryScope variables = new StandaloneQueryScope(); variables.putParam('t', TableFactory.newTable( Column.of('Date', String.class, new String[0]), Column.of('Delta', Long.class, new Long[0]), @@ -130,7 +131,7 @@ t = t.update('A=') .update( 'B=') doc = p.parse(src) Logger log = LoggerFactory.getLogger(CompletionHandler) - QueryScope variables = new QueryScope.StandaloneQueryScope() + QueryScope variables = new StandaloneQueryScope() variables.putParam('t', TableFactory.newTable( Column.of('A1', Long.class, new Long[0]), Column.of('A2', Integer.class, new Integer[0]), @@ -188,7 +189,7 @@ t.where('""" doc = p.parse(src) Logger log = LoggerFactory.getLogger(CompletionHandler) - QueryScope variables = new QueryScope.StandaloneQueryScope() + QueryScope variables = new StandaloneQueryScope() variables.putParam('t', null); ChunkerCompleter completer = new ChunkerCompleter(log, variables) From 25e4da762b16f396cb13184660c96f1a1e3c2d54 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Fri, 12 Jan 2024 11:34:26 -0600 Subject: [PATCH 19/23] Clean up standalone impl --- .../engine/context/StandaloneQueryScope.java | 69 +++++++------------ 1 file changed, 23 insertions(+), 46 deletions(-) diff --git a/engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java index 65fb10c8a57..263b8927621 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java @@ -15,11 +15,11 @@ import java.util.Set; /** - * Map-based implementation, with remote scope and object reflection support. + * Map-based implementation, extending LivenessArtifact to manage the objects passed into it. */ public class StandaloneQueryScope extends LivenessArtifact implements QueryScope { - private final KeyedObjectHashMap valueRetrievers = + private final KeyedObjectHashMap> valueRetrievers = new KeyedObjectHashMap<>(new ValueRetrieverNameKey()); /** @@ -32,7 +32,7 @@ private static Object applyValueConversions(final Object value) { if (value instanceof String) { final String stringValue = (String) value; - if (stringValue.length() > 0 && stringValue.charAt(0) == '\'' + if (!stringValue.isEmpty() && stringValue.charAt(0) == '\'' && stringValue.charAt(stringValue.length() - 1) == '\'') { final String datetimeString = stringValue.substring(1, stringValue.length() - 1); @@ -75,32 +75,32 @@ public boolean hasParamName(String name) { @Override public QueryScopeParam createParam(final String name) throws MissingVariableException { - // noinspection unchecked - final ValueRetriever valueRetriever = valueRetrievers.get(name); + final ValueRetriever valueRetriever = valueRetrievers.get(name); if (valueRetriever == null) { throw new MissingVariableException("Missing variable " + name); } - return valueRetriever.createParam(); + //noinspection unchecked + return (QueryScopeParam) valueRetriever.createParam(); } @Override public T readParamValue(final String name) throws MissingVariableException { - // noinspection unchecked - final ValueRetriever valueRetriever = valueRetrievers.get(name); + final ValueRetriever valueRetriever = valueRetrievers.get(name); if (valueRetriever == null) { throw new MissingVariableException("Missing variable " + name); } - return valueRetriever.getValue(); + // noinspection unchecked + return (T) valueRetriever.getValue(); } @Override public T readParamValue(final String name, final T defaultValue) { - // noinspection unchecked - final ValueRetriever valueRetriever = valueRetrievers.get(name); + final ValueRetriever valueRetriever = valueRetrievers.get(name); if (valueRetriever == null) { return defaultValue; } - return valueRetriever.getValue(); + // noinspection unchecked + return (T) valueRetriever.getValue(); } @Override @@ -111,7 +111,7 @@ public void putParam(final String name, final T value) { NameValidator.validateQueryParameterName(name); // TODO: Can I get rid of this applyValueConversions? It's too inconsistent to feel safe. ValueRetriever oldValueRetriever = - valueRetrievers.put(name, new SimpleValueRetriever<>(name, applyValueConversions(value))); + valueRetrievers.put(name, new ValueRetriever<>(name, applyValueConversions(value))); if (oldValueRetriever != null) { Object oldValue = oldValueRetriever.getValue(); @@ -121,56 +121,33 @@ public void putParam(final String name, final T value) { } } - private static abstract class ValueRetriever { + private static class ValueRetriever { + protected final T value; private final String name; - protected ValueRetriever(String name) { + protected ValueRetriever(String name, final T value) { this.name = name; + this.value = value; } public String getName() { return name; } - public abstract T getValue(); - - public abstract Class getType(); - - public abstract QueryScopeParam createParam(); - } - - private static class ValueRetrieverNameKey extends KeyedObjectKey.Basic { - - @Override - public String getKey(ValueRetriever valueRetriever) { - return valueRetriever.getName(); - } - } - - private static class SimpleValueRetriever extends ValueRetriever { - - private final T value; - - public SimpleValueRetriever(final String name, final T value) { - super(name); - this.value = value; - } - - @Override public T getValue() { return value; } - @Override - public Class getType() { - // noinspection unchecked - return (Class) (value != null ? value.getClass() : Object.class); + public QueryScopeParam createParam() { + return new QueryScopeParam<>(getName(), getValue()); } + } + private static class ValueRetrieverNameKey extends KeyedObjectKey.Basic> { @Override - public QueryScopeParam createParam() { - return new QueryScopeParam<>(getName(), getValue()); + public String getKey(ValueRetriever valueRetriever) { + return valueRetriever.getName(); } } } From d2208c3aa651c7d6678c84a574c400d8aa12b7a0 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Fri, 12 Jan 2024 11:41:54 -0600 Subject: [PATCH 20/23] spotless --- .../java/io/deephaven/engine/context/StandaloneQueryScope.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java index 263b8927621..04ace24e87c 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java @@ -79,7 +79,7 @@ public QueryScopeParam createParam(final String name) throws MissingVaria if (valueRetriever == null) { throw new MissingVariableException("Missing variable " + name); } - //noinspection unchecked + // noinspection unchecked return (QueryScopeParam) valueRetriever.createParam(); } From 7598e14c38d23b54a5db78d71274ad31eaa92305 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Thu, 18 Jan 2024 15:11:17 -0600 Subject: [PATCH 21/23] Review feedback --- .../python/PythonDeephavenSession.java | 27 +++------ .../engine/context/EmptyQueryScope.java | 6 ++ .../engine/context/PoisonedQueryScope.java | 6 ++ .../deephaven/engine/context/QueryScope.java | 39 ++++++------ .../engine/context/StandaloneQueryScope.java | 59 ++++--------------- .../engine/util/AbstractScriptSession.java | 21 +++++-- .../engine/util/GroovyDeephavenSession.java | 7 +++ .../util/NoLanguageDeephavenSession.java | 7 +++ 8 files changed, 79 insertions(+), 93 deletions(-) diff --git a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java index 4de234de16a..8ab8eed850e 100644 --- a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java +++ b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java @@ -41,6 +41,7 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; +import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.concurrent.ExecutorService; @@ -248,30 +249,12 @@ protected Changes createDiff(PythonSnapshot from, PythonSnapshot to, RuntimeExce final String name = change.call(String.class, "__getitem__", int.class, 0); final PyObject fromValue = change.call(PyObject.class, "__getitem__", int.class, 1); final PyObject toValue = change.call(PyObject.class, "__getitem__", int.class, 2); - applyVariableChangeToDiff(diff, name, maybeUnwrap(fromValue), maybeUnwrap(toValue)); + applyVariableChangeToDiff(diff, name, unwrapObject(fromValue), unwrapObject(toValue)); } return diff; } } - private Object maybeUnwrap(Object o) { - if (o instanceof PyObject) { - return maybeUnwrap((PyObject) o); - } - return o; - } - - private Object maybeUnwrap(PyObject o) { - if (o == null) { - return null; - } - final Object javaObject = module.javaify(o); - if (javaObject != null) { - return javaObject; - } - return o; - } - @Override protected Set getVariableNames(Predicate allowName) { return PyLib.ensureGil(() -> scope.getKeys().filter(allowName).collect(Collectors.toUnmodifiableSet())); @@ -317,6 +300,12 @@ protected synchronized Object setVariable(String name, @Nullable Object newValue return old; } + @Override + protected Map getAllValues() { + return PyLib + .ensureGil(() -> scope.getEntries().collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue))); + } + @Override public String scriptType() { return SCRIPT_TYPE; diff --git a/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java index 76e3adfb092..c3eecb174ab 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java @@ -8,6 +8,7 @@ import java.lang.ref.WeakReference; import java.util.Collections; +import java.util.Map; import java.util.Set; import java.util.stream.Stream; @@ -46,6 +47,11 @@ public void putParam(String name, T value) { throw new IllegalStateException("EmptyQueryScope cannot create parameters"); } + @Override + public Map readAllValues() { + return Collections.emptyMap(); + } + @Override public boolean tryManage(@NotNull LivenessReferent referent) { throw new UnsupportedOperationException("tryManage"); diff --git a/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java index 02b17020ef5..0691291cca2 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java @@ -8,6 +8,7 @@ import org.jetbrains.annotations.NotNull; import java.lang.ref.WeakReference; +import java.util.Map; import java.util.Set; import java.util.stream.Stream; @@ -51,6 +52,11 @@ public void putParam(String name, T value) { fail(); } + @Override + public Map readAllValues() { + return fail(); + } + @Override public boolean tryManage(@NotNull LivenessReferent referent) { return fail(); diff --git a/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java index 59d98ce63cb..88139a8d73e 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java @@ -9,16 +9,19 @@ import org.jetbrains.annotations.NotNull; import java.util.Collection; +import java.util.Map; import java.util.Objects; import java.util.Set; /** - * Variable scope used to resolve parameter values during query execution. + * Variable scope used to resolve parameter values during query execution and to expose named objects to users. Objects + * passed in will have their liveness managed by the scope. */ public interface QueryScope extends LivenessNode, LogOutputAppendable { /** * Adds a parameter to the default instance {@link QueryScope}, or updates the value of an existing parameter. + * Objects passed in will have their liveness managed by the scope. * * @param name String name of the parameter to add. * @param value value to assign to the parameter. @@ -40,10 +43,6 @@ static T getParamValue(final String name) throws MissingVariableException { return ExecutionContext.getContext().getQueryScope().readParamValue(name); } - // ----------------------------------------------------------------------------------------------------------------- - // Implementation - // ----------------------------------------------------------------------------------------------------------------- - /** * A type of RuntimeException thrown when a variable referenced within the {@link QueryScope} is not defined or, * more likely, has not been added to the scope. @@ -63,10 +62,6 @@ private MissingVariableException(final Throwable cause) { } } - // ----------------------------------------------------------------------------------------------------------------- - // Scope manipulation helper methods - // ----------------------------------------------------------------------------------------------------------------- - /** * Get an array of Params by name. See createParam(name) implementations for details. * @@ -83,10 +78,6 @@ default QueryScopeParam[] getParams(final Collection names) throws Missi return result; } - // ----------------------------------------------------------------------------------------------------------------- - // General scope manipulation methods - // ----------------------------------------------------------------------------------------------------------------- - /** * Get all known scope variable names. * @@ -112,7 +103,8 @@ default QueryScopeParam[] getParams(final Collection names) throws Missi QueryScopeParam createParam(final String name) throws MissingVariableException; /** - * Get the value of a given scope parameter by name. + * Get the value of a given scope parameter by name. Callers may want to unwrap language-specific values using + * {@link #unwrapObject(Object)} before using them. * * @param name parameter name. * @return parameter value. @@ -121,7 +113,8 @@ default QueryScopeParam[] getParams(final Collection names) throws Missi T readParamValue(final String name) throws MissingVariableException; /** - * Get the value of a given scope parameter by name. + * Get the value of a given scope parameter by name. Callers may want to unwrap language-specific values using + * {@link #unwrapObject(Object)} before using them. * * @param name parameter name. * @param defaultValue default parameter value. @@ -138,8 +131,16 @@ default QueryScopeParam[] getParams(final Collection names) throws Missi void putParam(final String name, final T value); /** - * Asks the session to remove any wrapping that exists on scoped objects so that clients can fetch them. Defaults to - * returning the object itself. + * Returns an immutable map with all objects in the scope. Callers may want to unwrap language-specific values using + * {@link #unwrapObject(Object)} before using them. + * + * @return an immutable map with all known variables and their values. + */ + Map readAllValues(); + + /** + * Removes any wrapping that exists on a scope param object so that clients can fetch them. Defaults to returning + * the object itself. * * @param object the scoped object * @return an obj which can be consumed by a client @@ -148,10 +149,6 @@ default Object unwrapObject(Object object) { return object; } - // ----------------------------------------------------------------------------------------------------------------- - // LogOutputAppendable implementation - // ----------------------------------------------------------------------------------------------------------------- - @Override default LogOutput append(@NotNull final LogOutput logOutput) { logOutput.append('{'); diff --git a/engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java index 04ace24e87c..f8d833e717c 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java @@ -6,13 +6,10 @@ import io.deephaven.engine.updategraph.DynamicNode; import io.deephaven.hash.KeyedObjectHashMap; import io.deephaven.hash.KeyedObjectKey; -import io.deephaven.time.DateTimeUtils; -import io.deephaven.util.QueryConstants; -import java.time.Duration; -import java.time.Instant; -import java.time.Period; +import java.util.Map; import java.util.Set; +import java.util.stream.Collectors; /** * Map-based implementation, extending LivenessArtifact to manage the objects passed into it. @@ -22,47 +19,6 @@ public class StandaloneQueryScope extends LivenessArtifact implements QueryScope private final KeyedObjectHashMap> valueRetrievers = new KeyedObjectHashMap<>(new ValueRetrieverNameKey()); - /** - * Apply conversions to certain scope variable values. - * - * @param value value - * @return value, or an appropriately converted substitute. - */ - private static Object applyValueConversions(final Object value) { - if (value instanceof String) { - final String stringValue = (String) value; - - if (!stringValue.isEmpty() && stringValue.charAt(0) == '\'' - && stringValue.charAt(stringValue.length() - 1) == '\'') { - final String datetimeString = stringValue.substring(1, stringValue.length() - 1); - - final Instant instant = DateTimeUtils.parseInstantQuiet(datetimeString); - if (instant != null) { - return instant; - } - - final long localTime = DateTimeUtils.parseDurationNanosQuiet(datetimeString); - if (localTime != QueryConstants.NULL_LONG) { - return localTime; - } - - final Period period = DateTimeUtils.parsePeriodQuiet(datetimeString); - if (period != null) { - return period; - } - - final Duration duration = DateTimeUtils.parseDurationQuiet(datetimeString); - if (duration != null) { - return duration; - } - - throw new RuntimeException("Cannot parse datetime/time/period : " + stringValue); - } - } - - return value; - } - @Override public Set getParamNames() { return valueRetrievers.keySet(); @@ -105,13 +61,12 @@ public T readParamValue(final String name, final T defaultValue) { @Override public void putParam(final String name, final T value) { + NameValidator.validateQueryParameterName(name); if (value instanceof LivenessReferent && DynamicNode.notDynamicOrIsRefreshing(value)) { manage((LivenessReferent) value); } - NameValidator.validateQueryParameterName(name); - // TODO: Can I get rid of this applyValueConversions? It's too inconsistent to feel safe. ValueRetriever oldValueRetriever = - valueRetrievers.put(name, new ValueRetriever<>(name, applyValueConversions(value))); + valueRetrievers.put(name, new ValueRetriever<>(name, (Object) value)); if (oldValueRetriever != null) { Object oldValue = oldValueRetriever.getValue(); @@ -121,6 +76,12 @@ public void putParam(final String name, final T value) { } } + @Override + public Map readAllValues() { + return valueRetrievers.entrySet().stream() + .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().value)); + } + private static class ValueRetriever { protected final T value; diff --git a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java index de0899800ca..ecfbe6e648b 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java @@ -30,6 +30,7 @@ import java.io.File; import java.io.IOException; import java.nio.file.Path; +import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.UUID; @@ -285,11 +286,18 @@ public QueryScope getQueryScope() { * * @param name the variable name to set * @param value the new value of the variable - * @return the old previous value for this name, if any. As with {@link #getVariable(String)}, may need to be - * unwrapped. + * @return the old value for this name, if any. As with {@link #getVariable(String)}, may need to be unwrapped. */ protected abstract Object setVariable(String name, @Nullable Object value); + /** + * Returns an immutable map with all known variables and their values. + * + * @return an immutable map with all known variables and their values. As with {@link #getVariable(String)}, values + * may need to be unwrapped. + */ + protected abstract Map getAllValues(); + // ----------------------------------------------------------------------------------------------------------------- // ScriptSession-based QueryScope implementation, with no remote scope or object reflection support // ----------------------------------------------------------------------------------------------------------------- @@ -304,7 +312,7 @@ public ScriptSession scriptSession() { @Override public Set getParamNames() { - return AbstractScriptSession.this.getVariableNames(NameValidator::isValidQueryParameterName); + return getVariableNames(NameValidator::isValidQueryParameterName); } @Override @@ -351,7 +359,7 @@ public void putParam(final String name, final T value) { manage((LivenessReferent) value); } - Object oldValue = AbstractScriptSession.this.setVariable(name, value); + Object oldValue = setVariable(name, value); Object unwrappedOldValue = unwrapObject(oldValue); @@ -361,6 +369,11 @@ public void putParam(final String name, final T value) { } } + @Override + public Map readAllValues() { + return AbstractScriptSession.this.getAllValues(); + } + @Override public Object unwrapObject(Object object) { return AbstractScriptSession.this.unwrapObject(object); diff --git a/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java b/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java index 97546d1239b..9cb12275e73 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/GroovyDeephavenSession.java @@ -767,6 +767,13 @@ protected Object setVariable(String name, @Nullable Object newValue) { return oldValue; } + @Override + protected Map getAllValues() { + synchronized (bindingBackingMap) { + return Map.copyOf(bindingBackingMap); + } + } + public Binding getBinding() { return groovyShell.getContext(); } diff --git a/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java b/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java index 1aeffad70c9..56abfb0756e 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/NoLanguageDeephavenSession.java @@ -94,6 +94,13 @@ protected Object setVariable(String name, @Nullable Object newValue) { // changes } + @Override + protected Map getAllValues() { + synchronized (variables) { + return Map.copyOf(variables); + } + } + @Override public String scriptType() { return SCRIPT_TYPE; From 668596d74324410a70640fb78ff74f26b2cac1e7 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Fri, 19 Jan 2024 15:36:42 -0600 Subject: [PATCH 22/23] Review feedback --- .../python/PythonDeephavenSession.java | 3 ++- .../engine/context/EmptyQueryScope.java | 2 +- .../engine/context/PoisonedQueryScope.java | 2 +- .../io/deephaven/engine/context/QueryScope.java | 2 +- .../engine/context/StandaloneQueryScope.java | 4 ++-- .../main/java/io/deephaven/engine/sql/Sql.java | 16 ++++++++-------- .../engine/util/AbstractScriptSession.java | 2 +- .../server/console/ScopeTicketResolver.java | 7 +++---- 8 files changed, 19 insertions(+), 19 deletions(-) diff --git a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java index 8ab8eed850e..9ca382093ad 100644 --- a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java +++ b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java @@ -303,7 +303,8 @@ protected synchronized Object setVariable(String name, @Nullable Object newValue @Override protected Map getAllValues() { return PyLib - .ensureGil(() -> scope.getEntries().collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue))); + .ensureGil(() -> scope.getEntries() + .collect(Collectors.toUnmodifiableMap(Map.Entry::getKey, Map.Entry::getValue))); } @Override diff --git a/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java index c3eecb174ab..d2b3eb7cca2 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/EmptyQueryScope.java @@ -48,7 +48,7 @@ public void putParam(String name, T value) { } @Override - public Map readAllValues() { + public Map toMap() { return Collections.emptyMap(); } diff --git a/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java index 0691291cca2..4da6964287c 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/PoisonedQueryScope.java @@ -53,7 +53,7 @@ public void putParam(String name, T value) { } @Override - public Map readAllValues() { + public Map toMap() { return fail(); } diff --git a/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java index 88139a8d73e..2ea696b3189 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java @@ -136,7 +136,7 @@ default QueryScopeParam[] getParams(final Collection names) throws Missi * * @return an immutable map with all known variables and their values. */ - Map readAllValues(); + Map toMap(); /** * Removes any wrapping that exists on a scope param object so that clients can fetch them. Defaults to returning diff --git a/engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java b/engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java index f8d833e717c..3461992aaa0 100644 --- a/engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java +++ b/engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java @@ -77,9 +77,9 @@ public void putParam(final String name, final T value) { } @Override - public Map readAllValues() { + public Map toMap() { return valueRetrievers.entrySet().stream() - .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().value)); + .collect(Collectors.toUnmodifiableMap(Map.Entry::getKey, e -> e.getValue().value)); } private static class ValueRetriever { diff --git a/engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java b/engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java index e59add6bc08..1a602b9cf4a 100644 --- a/engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java +++ b/engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java @@ -26,6 +26,7 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.stream.Collectors; /** * Experimental SQL execution. Subject to change. @@ -79,16 +80,15 @@ private static Scope scope(Map scope, Map out } private static Map currentScriptSessionNamedTables() { - final Map scope = new HashMap<>(); // getVariables() is inefficient // See SQLTODO(catalog-reader-implementation) - QueryScope queryScope = ExecutionContext.getContext().getQueryScope(); - for (String name : queryScope.getParamNames()) { - Object paramValue = queryScope.unwrapObject(queryScope.readParamValue(name)); - if (paramValue instanceof Table) { - scope.put(name, (Table) paramValue); - } - } + final Map scope = ExecutionContext.getContext().getQueryScope() + .toMap() + .entrySet() + .stream() + .filter(e -> e.getValue() instanceof Table) + .collect(Collectors.toMap(Entry::getKey, e -> (Table) e.getValue())); + return scope; } diff --git a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java index ecfbe6e648b..bce3c2f8236 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/AbstractScriptSession.java @@ -370,7 +370,7 @@ public void putParam(final String name, final T value) { } @Override - public Map readAllValues() { + public Map toMap() { return AbstractScriptSession.this.getAllValues(); } diff --git a/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java b/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java index 2c61b2cb4cf..359b1a93a6f 100644 --- a/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java +++ b/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java @@ -55,11 +55,11 @@ public SessionState.ExportObject flightInfoFor( Object scopeVar = queryScope.readParamValue(scopeName, null); if (scopeVar == null) { throw Exceptions.statusRuntimeException(Code.NOT_FOUND, - "Could not resolve '" + logId + ": no variable exists with name '" + scopeName + "'"); + "Could not resolve '" + logId + ": no table exists with name '" + scopeName + "'"); } if (!(scopeVar instanceof Table)) { throw Exceptions.statusRuntimeException(Code.NOT_FOUND, - "Could not resolve '" + logId + "': no variable exists with name '" + scopeName + "'"); + "Could not resolve '" + logId + "': no table exists with name '" + scopeName + "'"); } Table transformed = authorization.transform((Table) scopeVar); @@ -72,8 +72,7 @@ public SessionState.ExportObject flightInfoFor( @Override public void forAllFlightInfo(@Nullable final SessionState session, final Consumer visitor) { QueryScope queryScope = ExecutionContext.getContext().getQueryScope(); - queryScope.getParamNames().forEach((varName) -> { - Object varObj = queryScope.readParamValue(varName); + queryScope.toMap().forEach((varName, varObj) -> { if (varObj instanceof Table) { visitor.accept(TicketRouter.getFlightInfo((Table) varObj, descriptorForName(varName), flightTicketForName(varName))); From 1e32a0ee408052bd93ffc84f681114cb579889f4 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Fri, 19 Jan 2024 16:23:02 -0600 Subject: [PATCH 23/23] Add missing queryScope.unwrapObject() calls --- engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java | 4 +++- .../java/io/deephaven/server/console/ScopeTicketResolver.java | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java b/engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java index 1a602b9cf4a..be9bae117f0 100644 --- a/engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java +++ b/engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java @@ -82,10 +82,12 @@ private static Scope scope(Map scope, Map out private static Map currentScriptSessionNamedTables() { // getVariables() is inefficient // See SQLTODO(catalog-reader-implementation) - final Map scope = ExecutionContext.getContext().getQueryScope() + QueryScope queryScope = ExecutionContext.getContext().getQueryScope(); + final Map scope = queryScope .toMap() .entrySet() .stream() + .map(e -> Map.entry(e.getKey(), queryScope.unwrapObject(e.getValue()))) .filter(e -> e.getValue() instanceof Table) .collect(Collectors.toMap(Entry::getKey, e -> (Table) e.getValue())); diff --git a/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java b/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java index 359b1a93a6f..ae9c58ed0ee 100644 --- a/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java +++ b/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java @@ -52,7 +52,7 @@ public SessionState.ExportObject flightInfoFor( final String scopeName = nameForDescriptor(descriptor, logId); QueryScope queryScope = ExecutionContext.getContext().getQueryScope(); - Object scopeVar = queryScope.readParamValue(scopeName, null); + Object scopeVar = queryScope.unwrapObject(queryScope.readParamValue(scopeName, null)); if (scopeVar == null) { throw Exceptions.statusRuntimeException(Code.NOT_FOUND, "Could not resolve '" + logId + ": no table exists with name '" + scopeName + "'"); @@ -73,6 +73,7 @@ public SessionState.ExportObject flightInfoFor( public void forAllFlightInfo(@Nullable final SessionState session, final Consumer visitor) { QueryScope queryScope = ExecutionContext.getContext().getQueryScope(); queryScope.toMap().forEach((varName, varObj) -> { + varObj = queryScope.unwrapObject(varObj); if (varObj instanceof Table) { visitor.accept(TicketRouter.getFlightInfo((Table) varObj, descriptorForName(varName), flightTicketForName(varName)));