Extract variable api from ScriptSession, let ScriptSession guard reads

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<String> getVariableNames(Predicate<String> 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<String, Object> getAllValues() {
+        return PyLib
+                .ensureGil(() -> scope.getEntries().collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)));
+    }
+
     @Override
     public String scriptType() {
         return SCRIPT_TYPE;

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 <T> void putParam(String name, T value) {
         throw new IllegalStateException("EmptyQueryScope cannot create parameters");
     }
 
+    @Override
+    public Map<String, Object> readAllValues() {
+        return Collections.emptyMap();
+    }
+
     @Override
     public boolean tryManage(@NotNull LivenessReferent referent) {
         throw new UnsupportedOperationException("tryManage");

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 <T> void putParam(String name, T value) {
         fail();
     }
 
+    @Override
+    public Map<String, Object> readAllValues() {
+        return fail();
+    }
+
     @Override
     public boolean tryManage(@NotNull LivenessReferent referent) {
         return fail();

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> 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<String> names) throws Missi
         return result;
     }
 
-    // -----------------------------------------------------------------------------------------------------------------
-    // General scope manipulation methods
-    // -----------------------------------------------------------------------------------------------------------------
-
     /**
      * Get all known scope variable names.
      *
@@ -112,7 +103,8 @@ default QueryScopeParam[] getParams(final Collection<String> names) throws Missi
     <T> QueryScopeParam<T> 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<String> names) throws Missi
     <T> 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<String> names) throws Missi
     <T> 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<String, Object> 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('{');

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<String, ValueRetriever<?>> 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<String> getParamNames() {
         return valueRetrievers.keySet();
@@ -105,13 +61,12 @@ public <T> T readParamValue(final String name, final T defaultValue) {
 
     @Override
     public <T> 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 <T> void putParam(final String name, final T value) {
         }
     }
 
+    @Override
+    public Map<String, Object> readAllValues() {
+        return valueRetrievers.entrySet().stream()
+                .collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().value));
+    }
+
     private static class ValueRetriever<T> {
 
         protected final T value;

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<String, Object> getAllValues();
+
     // -----------------------------------------------------------------------------------------------------------------
     // ScriptSession-based QueryScope implementation, with no remote scope or object reflection support
     // -----------------------------------------------------------------------------------------------------------------
@@ -304,7 +312,7 @@ public ScriptSession scriptSession() {
 
         @Override
         public Set<String> getParamNames() {
-            return AbstractScriptSession.this.getVariableNames(NameValidator::isValidQueryParameterName);
+            return getVariableNames(NameValidator::isValidQueryParameterName);
         }
 
         @Override
@@ -351,7 +359,7 @@ public <T> 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 <T> void putParam(final String name, final T value) {
             }
         }
 
+        @Override
+        public Map<String, Object> readAllValues() {
+            return AbstractScriptSession.this.getAllValues();
+        }
+
         @Override
         public Object unwrapObject(Object object) {
             return AbstractScriptSession.this.unwrapObject(object);

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<String, Object> getAllValues() {
+        synchronized (bindingBackingMap) {
+            return Map.copyOf(bindingBackingMap);
+        }
+    }
+
     public Binding getBinding() {
         return groovyShell.getContext();
     }

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<String, Object> getAllValues() {
+        synchronized (variables) {
+            return Map.copyOf(variables);
+        }
+    }
+
     @Override
     public String scriptType() {
         return SCRIPT_TYPE;