From 73eb5c6e4137c21c436b74338bc2ecb920cbc1c2 Mon Sep 17 00:00:00 2001 From: Nathaniel Bauernfeind Date: Mon, 12 Feb 2024 15:59:28 -0700 Subject: [PATCH 1/2] Drop Usage of `ensureGil` from PythonDeephavenSession and Copy Globals Instead --- .../python/PythonDeephavenSession.java | 36 +++++++------ .../engine/context/EmptyQueryScope.java | 4 +- .../engine/context/PoisonedQueryScope.java | 5 +- .../deephaven/engine/context/QueryScope.java | 32 ++++++++---- .../engine/context/StandaloneQueryScope.java | 37 ++++++++++---- .../java/io/deephaven/engine/sql/Sql.java | 15 ++---- .../impl/select/AbstractConditionFilter.java | 2 +- .../impl/select/codegen/FormulaAnalyzer.java | 2 +- .../engine/util/AbstractScriptSession.java | 50 ++++++++----------- .../engine/util/GroovyDeephavenSession.java | 30 ++++++++--- .../util/NoLanguageDeephavenSession.java | 25 ++++++++-- .../io/deephaven/engine/util/PythonScope.java | 7 ++- .../engine/util/PythonScopeJpyImpl.java | 3 +- .../impl/lang/TestQueryLanguageParser.java | 2 +- .../server/console/ScopeTicketResolver.java | 14 +++--- .../io/deephaven/api/util/NameValidator.java | 4 -- 16 files changed, 160 insertions(+), 108 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 9e607ba4729..1eff41c72eb 100644 --- a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java +++ b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java @@ -7,6 +7,7 @@ import io.deephaven.base.verify.Assert; import io.deephaven.configuration.Configuration; import io.deephaven.engine.context.ExecutionContext; +import io.deephaven.engine.context.StandaloneQueryScope; import io.deephaven.engine.exceptions.CancellationException; import io.deephaven.engine.context.QueryScope; import io.deephaven.engine.updategraph.OperationInitializer; @@ -45,6 +46,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.function.Consumer; +import java.util.function.Function; import java.util.function.Predicate; import java.util.stream.Collectors; @@ -255,7 +257,10 @@ protected Changes createDiff(PythonSnapshot from, PythonSnapshot to, RuntimeExce @Override protected Set getVariableNames(Predicate allowName) { - return PyLib.ensureGil(() -> scope.getKeys().filter(allowName).collect(Collectors.toUnmodifiableSet())); + return scope.currentScope().copy().keySet().stream() + .map(scope::convertStringKey) + .filter(allowName) + .collect(Collectors.toUnmodifiableSet()); } @Override @@ -299,19 +304,22 @@ protected synchronized Object setVariable(String name, @Nullable Object newValue } @Override - protected Map getAllValues(@NotNull final Predicate> predicate) { - final HashMap result = PyLib.ensureGil( - () -> scope.getEntriesRaw().>map( - e -> new AbstractMap.SimpleImmutableEntry<>(scope.convertStringKey(e.getKey()), e.getValue())) - .collect(HashMap::new, (map, entry) -> map.put(entry.getKey(), entry.getValue()), - HashMap::putAll)); - - final Iterator> iter = result.entrySet().iterator(); - while (iter.hasNext()) { - final Map.Entry entry = iter.next(); - entry.setValue(scope.convertValue((PyObject) entry.getValue())); - if (!predicate.test(entry)) { - iter.remove(); + protected Map getAllValues( + @Nullable final Function valueMapper, + @NotNull final QueryScope.ParamFilter filter) { + final Map result = new HashMap<>(); + + for (final Map.Entry entry : scope.currentScope().copy().entrySet()) { + final String name = scope.convertStringKey(entry.getKey()); + Object value = scope.convertValue(entry.getValue()); + if (valueMapper != null) { + value = valueMapper.apply(value); + } + + // noinspection unchecked + if (filter.accept(name, (T) value)) { + // noinspection unchecked + result.put(name, (T) value); } } 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 e465d487578..860c7400a7f 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 @@ -5,11 +5,13 @@ import io.deephaven.engine.liveness.LivenessReferent; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import java.lang.ref.WeakReference; import java.util.Collections; import java.util.Map; import java.util.Set; +import java.util.function.Function; import java.util.function.Predicate; import java.util.stream.Stream; @@ -49,7 +51,7 @@ public void putParam(String name, T value) { } @Override - public Map toMap(@NotNull Predicate> predicate) { + public Map toMap(@Nullable Function valueMapper, @NotNull ParamFilter filter) { 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 7acda65ad9e..18f2bd1ca32 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 @@ -6,11 +6,12 @@ import io.deephaven.engine.liveness.LivenessReferent; import io.deephaven.util.ExecutionContextRegistrationException; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import java.lang.ref.WeakReference; import java.util.Map; import java.util.Set; -import java.util.function.Predicate; +import java.util.function.Function; import java.util.stream.Stream; public class PoisonedQueryScope implements QueryScope { @@ -54,7 +55,7 @@ public void putParam(String name, T value) { } @Override - public Map toMap(@NotNull Predicate> predicate) { + public Map toMap(@Nullable Function valueMapper, @NotNull ParamFilter filter) { 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 b9df1da15f7..76d2221c5d0 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 @@ -14,7 +14,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.function.Predicate; +import java.util.function.Function; /** * Variable scope used to resolve parameter values during query execution and to expose named objects to users. Objects @@ -133,25 +133,35 @@ default QueryScopeParam[] getParams(final Collection names) throws Mi */ void putParam(final String name, final T value); + interface ParamFilter { + boolean accept(String name, T value); + } + /** * Returns a mutable map with all objects in the scope. Callers may want to unwrap language-specific values using - * {@link #unwrapObject(Object)} before using them. This map is owned by the caller and may be mutated. + * {@link #unwrapObject(Object)} before using them. This returned map is owned by the caller. * - * @param predicate a predicate to filter the map entries + * @param filter a predicate to filter the map entries * @return a caller-owned mutable map with all known variables and their values. */ - Map toMap(@NotNull Predicate> predicate); + @FinalDefault + default Map toMap(@NotNull ParamFilter filter) { + return toMap(null, filter); + } /** - * Returns a mutable map with all objects in the scope. Callers may want to unwrap language-specific values using - * {@link #unwrapObject(Object)} before using them. This map is owned by the caller and may be mutated. + * Returns a mutable map with all objects in the scope. + *

+ * Callers may want to pass in a valueMapper of {@link #unwrapObject(Object)} which would unwrap values before + * filtering. The returned map is owned by the caller. * + * @param valueMapper a function to map the values + * @param filter a predicate to filter the map entries * @return a caller-owned mutable map with all known variables and their values. + * @param the type of the mapped values */ - @FinalDefault - default Map toMap() { - return toMap(entry -> true); - } + Map toMap( + @Nullable Function valueMapper, @NotNull ParamFilter filter); /** * Removes any wrapping that exists on a scope param object so that clients can fetch them. Defaults to returning @@ -167,7 +177,7 @@ default Object unwrapObject(@Nullable Object object) { @Override default LogOutput append(@NotNull final LogOutput logOutput) { logOutput.append('{'); - for (final Map.Entry param : toMap().entrySet()) { + for (final Map.Entry param : toMap((name, value) -> true).entrySet()) { logOutput.nl().append(param.getKey()).append("="); Object paramValue = param.getValue(); if (paramValue == this) { 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 359b4c4bf33..f660fa9fa74 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,21 +6,22 @@ import io.deephaven.engine.updategraph.DynamicNode; import io.deephaven.hash.KeyedObjectHashMap; import io.deephaven.hash.KeyedObjectKey; -import org.apache.commons.lang3.tuple.ImmutablePair; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; +import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.Set; -import java.util.function.Predicate; +import java.util.function.Function; /** * Map-based implementation, extending LivenessArtifact to manage the objects passed into it. */ public class StandaloneQueryScope extends LivenessArtifact implements QueryScope { - private final KeyedObjectHashMap> valueRetrievers = - new KeyedObjectHashMap<>(new ValueRetrieverNameKey()); + private final Map> valueRetrievers = + Collections.synchronizedMap(new KeyedObjectHashMap<>(new ValueRetrieverNameKey())); @Override public Set getParamNames() { @@ -80,12 +81,28 @@ public void putParam(final String name, final T value) { } @Override - public Map toMap(@NotNull final Predicate> predicate) { - final HashMap result = new HashMap<>(); - valueRetrievers.entrySet().stream() - .map(e -> ImmutablePair.of(e.getKey(), (Object) e.getValue().value)) - .filter(predicate) - .forEach(e -> result.put(e.getKey(), e.getValue())); + public Map toMap( + @Nullable final Function valueMapper, + @NotNull final ParamFilter filter) { + final Map result = new HashMap<>(); + + synchronized (valueRetrievers) { + for (final Map.Entry> entry : valueRetrievers.entrySet()) { + final String name = entry.getKey(); + final ValueRetriever valueRetriever = entry.getValue(); + Object value = valueRetriever.getValue(); + if (valueMapper != null) { + value = valueMapper.apply(value); + } + + // noinspection unchecked + if (filter.accept(name, (T) value)) { + // noinspection unchecked + result.put(name, (T) value); + } + } + } + return result; } 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 be9bae117f0..1ba2fe0aecc 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,16 +82,11 @@ private static Scope scope(Map scope, Map out private static Map currentScriptSessionNamedTables() { // getVariables() is inefficient // See SQLTODO(catalog-reader-implementation) - 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())); - - return scope; + final QueryScope queryScope = ExecutionContext.getContext().getQueryScope(); + return queryScope.toMap(wrapped -> { + final Object value = queryScope.unwrapObject(wrapped); + return value instanceof Table ? (Table) value : null; + }, (n, t) -> t != null); } private static TableHeader adapt(TableDefinition tableDef) { diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/AbstractConditionFilter.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/AbstractConditionFilter.java index 1c7ede9c690..66a8513fa4c 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/AbstractConditionFilter.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/AbstractConditionFilter.java @@ -95,7 +95,7 @@ public synchronized void init(TableDefinition tableDefinition) { try { final QueryScope queryScope = ExecutionContext.getContext().getQueryScope(); final Map queryScopeVariables = queryScope.toMap( - NameValidator.VALID_QUERY_PARAMETER_MAP_ENTRY_PREDICATE); + (name, value) -> NameValidator.isValidQueryParameterName(name)); for (Map.Entry param : queryScopeVariables.entrySet()) { possibleVariables.put(param.getKey(), QueryScopeParamTypeUtil.getDeclaredClass(param.getValue())); Type declaredType = QueryScopeParamTypeUtil.getDeclaredType(param.getValue()); diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/codegen/FormulaAnalyzer.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/codegen/FormulaAnalyzer.java index cc9dceedf58..acc064075d3 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/codegen/FormulaAnalyzer.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/codegen/FormulaAnalyzer.java @@ -105,7 +105,7 @@ public static QueryLanguageParser.Result getCompiledFormula(Map queryScopeVariables = context.getQueryScope().toMap( - NameValidator.VALID_QUERY_PARAMETER_MAP_ENTRY_PREDICATE); + (name, value) -> NameValidator.isValidQueryParameterName(name)); for (Map.Entry param : queryScopeVariables.entrySet()) { if (possibleVariables.containsKey(param.getKey())) { // skip any existing matches 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 5cf6677e3b2..08f4c88bb52 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 @@ -8,16 +8,13 @@ import io.deephaven.api.util.NameValidator; import io.deephaven.base.FileUtils; import io.deephaven.configuration.CacheDir; -import io.deephaven.engine.context.QueryCompiler; -import io.deephaven.engine.context.ExecutionContext; +import io.deephaven.engine.context.*; 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; -import io.deephaven.engine.context.QueryScope; -import io.deephaven.engine.context.QueryScopeParam; import io.deephaven.engine.table.hierarchical.HierarchicalTable; import io.deephaven.engine.updategraph.DynamicNode; import io.deephaven.engine.updategraph.OperationInitializer; @@ -31,10 +28,8 @@ 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; +import java.util.*; +import java.util.function.Function; import java.util.function.Predicate; import static io.deephaven.engine.table.Table.NON_DISPLAY_TABLE; @@ -292,13 +287,19 @@ public QueryScope getQueryScope() { protected abstract Object setVariable(String name, @Nullable Object value); /** - * Returns a mutable map with all known variables and their values. It is owned by the caller. + * Returns a mutable map with all known variables and their values. + *

+ * Callers may want to pass in a valueMapper of {@link #unwrapObject(Object)} which would unwrap values before + * filtering. The returned map is owned by the caller. * - * @param predicate a predicate to decide if an entry should be included in the returned map - * @return a mutable map with all known variables and their values. As with {@link #getVariable(String)}, values may - * need to be unwrapped. + * @param valueMapper a function to map the values + * @param filter a predicate to filter the map entries + * @return a caller-owned mutable map with all known variables and their mapped values. As with + * {@link #getVariable(String)}, values may need to be unwrapped. + * @param the type of the mapped values */ - protected abstract Map getAllValues(@NotNull Predicate> predicate); + protected abstract Map getAllValues( + @Nullable Function valueMapper, @NotNull QueryScope.ParamFilter filter); // ----------------------------------------------------------------------------------------------------------------- // ScriptSession-based QueryScope implementation, with no remote scope or object reflection support @@ -325,30 +326,18 @@ public boolean hasParamName(String name) { @Override public QueryScopeParam createParam(final String name) throws QueryScope.MissingVariableException { - if (!NameValidator.isValidQueryParameterName(name)) { - throw new QueryScope.MissingVariableException("Name " + name + " is invalid"); - } return new QueryScopeParam<>(name, readParamValue(name)); } @Override public T readParamValue(final String name) throws QueryScope.MissingVariableException { - if (!NameValidator.isValidQueryParameterName(name)) { - throw new QueryScope.MissingVariableException("Name " + name + " is invalid"); - } - // noinspection unchecked - return (T) getVariable(name); + return getVariable(name); } @Override public T readParamValue(final String name, final T defaultValue) { - if (!NameValidator.isValidQueryParameterName(name)) { - return defaultValue; - } - try { - // noinspection unchecked - return (T) getVariable(name); + return getVariable(name); } catch (MissingVariableException e) { return defaultValue; } @@ -356,7 +345,6 @@ 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); } @@ -372,8 +360,10 @@ public void putParam(final String name, final T value) { } @Override - public Map toMap(@NotNull final Predicate> predicate) { - return AbstractScriptSession.this.getAllValues(predicate); + public Map toMap( + @Nullable final Function valueMapper, + @NotNull final ParamFilter filter) { + return AbstractScriptSession.this.getAllValues(valueMapper, filter); } @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 41af97b5a1c..1c5bc4907da 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 @@ -15,12 +15,9 @@ import io.deephaven.api.updateby.UpdateByOperation; import io.deephaven.base.FileUtils; import io.deephaven.base.Pair; -import io.deephaven.engine.context.ExecutionContext; -import io.deephaven.engine.context.QueryCompiler; +import io.deephaven.engine.context.*; import io.deephaven.configuration.Configuration; -import io.deephaven.engine.context.QueryScopeParam; import io.deephaven.engine.exceptions.CancellationException; -import io.deephaven.engine.context.QueryScope; import io.deephaven.api.util.NameValidator; import io.deephaven.engine.rowset.RowSet; import io.deephaven.engine.rowset.TrackingRowSet; @@ -73,6 +70,7 @@ import java.time.ZonedDateTime; import java.util.*; import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Function; import java.util.function.Predicate; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -770,12 +768,28 @@ protected Object setVariable(String name, @Nullable Object newValue) { } @Override - protected Map getAllValues(@NotNull final Predicate> predicate) { + protected Map getAllValues( + @Nullable final Function valueMapper, + @NotNull final QueryScope.ParamFilter filter) { + final Map result = new HashMap<>(); + synchronized (bindingBackingMap) { - return bindingBackingMap.entrySet().stream() - .filter(predicate) - .collect(HashMap::new, (map, entry) -> map.put(entry.getKey(), entry.getValue()), HashMap::putAll); + for (final Map.Entry entry : bindingBackingMap.entrySet()) { + final String name = entry.getKey(); + Object value = entry.getValue(); + if (valueMapper != null) { + value = valueMapper.apply(value); + } + + // noinspection unchecked + if (filter.accept(name, (T) value)) { + // noinspection unchecked + result.put(name, (T) value); + } + } } + + return result; } 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 9ffac0e5c59..278ca5badd0 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,7 +3,6 @@ */ package io.deephaven.engine.util; -import io.deephaven.api.util.NameValidator; import io.deephaven.engine.context.QueryScope; import io.deephaven.engine.updategraph.OperationInitializer; import io.deephaven.engine.updategraph.UpdateGraph; @@ -11,6 +10,7 @@ import org.jetbrains.annotations.Nullable; import java.util.*; +import java.util.function.Function; import java.util.function.Predicate; import java.util.stream.Collectors; @@ -94,12 +94,27 @@ protected Object setVariable(String name, @Nullable Object newValue) { } @Override - protected Map getAllValues(@NotNull final Predicate> predicate) { + protected Map getAllValues(@Nullable Function valueMapper, + QueryScope.@NotNull ParamFilter filter) { + final Map result = new HashMap<>(); + synchronized (variables) { - return variables.entrySet().stream() - .filter(predicate) - .collect(HashMap::new, (map, entry) -> map.put(entry.getKey(), entry.getValue()), HashMap::putAll); + for (final Map.Entry entry : variables.entrySet()) { + final String name = entry.getKey(); + Object value = entry.getValue(); + if (valueMapper != null) { + value = valueMapper.apply(value); + } + + // noinspection unchecked + if (filter.accept(name, (T) value)) { + // noinspection unchecked + result.put(name, (T) value); + } + } } + + return result; } @Override diff --git a/engine/table/src/main/java/io/deephaven/engine/util/PythonScope.java b/engine/table/src/main/java/io/deephaven/engine/util/PythonScope.java index ba12ecb22aa..51e5f8b9fdc 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/PythonScope.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/PythonScope.java @@ -143,7 +143,12 @@ default Collection getKeysCollection() { /** * @return the Python's __main__ module namespace */ - public PyDictWrapper mainGlobals(); + PyDictWrapper mainGlobals(); + + /** + * @return the current scope or the main globals if no scope is set + */ + PyDictWrapper currentScope(); /** * Push the provided Python scope into the thread scope stack for subsequent operations on Tables diff --git a/engine/table/src/main/java/io/deephaven/engine/util/PythonScopeJpyImpl.java b/engine/table/src/main/java/io/deephaven/engine/util/PythonScopeJpyImpl.java index c2258bc5d99..117eff8b145 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/PythonScopeJpyImpl.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/PythonScopeJpyImpl.java @@ -39,7 +39,8 @@ public PythonScopeJpyImpl(PyDictWrapper dict) { this.dict = dict; } - private PyDictWrapper currentScope() { + @Override + public PyDictWrapper currentScope() { Deque scopeStack = threadScopeStack.get(); if (scopeStack == null || scopeStack.isEmpty()) { return this.dict; diff --git a/engine/table/src/test/java/io/deephaven/engine/table/impl/lang/TestQueryLanguageParser.java b/engine/table/src/test/java/io/deephaven/engine/table/impl/lang/TestQueryLanguageParser.java index 3eba42253f5..13f2e3c5aa1 100644 --- a/engine/table/src/test/java/io/deephaven/engine/table/impl/lang/TestQueryLanguageParser.java +++ b/engine/table/src/test/java/io/deephaven/engine/table/impl/lang/TestQueryLanguageParser.java @@ -3178,7 +3178,7 @@ private void check(String expression, String resultExpression, Class resultTy final Map possibleParams; final QueryScope queryScope = ExecutionContext.getContext().getQueryScope(); if (!(queryScope instanceof PoisonedQueryScope)) { - possibleParams = queryScope.toMap(NameValidator.VALID_QUERY_PARAMETER_MAP_ENTRY_PREDICATE); + possibleParams = queryScope.toMap((name, value) -> NameValidator.isValidQueryParameterName(name)); } else { possibleParams = null; } 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 ae9c58ed0ee..3458b5ed7b0 100644 --- a/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java +++ b/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java @@ -71,14 +71,12 @@ public SessionState.ExportObject flightInfoFor( @Override 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))); - } - }); + final QueryScope queryScope = ExecutionContext.getContext().getQueryScope(); + queryScope.toMap(wrapped -> { + final Object value = queryScope.unwrapObject(wrapped); + return value instanceof Table ? (Table) value : null; + }, (n, t) -> t != null).forEach((name, table) -> visitor + .accept(TicketRouter.getFlightInfo(table, descriptorForName(name), flightTicketForName(name)))); } @Override diff --git a/table-api/src/main/java/io/deephaven/api/util/NameValidator.java b/table-api/src/main/java/io/deephaven/api/util/NameValidator.java index a37e6a8ee17..4727765fb60 100644 --- a/table-api/src/main/java/io/deephaven/api/util/NameValidator.java +++ b/table-api/src/main/java/io/deephaven/api/util/NameValidator.java @@ -6,7 +6,6 @@ import javax.lang.model.SourceVersion; import java.util.*; import java.util.function.Function; -import java.util.function.Predicate; import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -97,9 +96,6 @@ private boolean isValid(String name) { Stream.of("in", "not", "i", "ii", "k").collect( Collectors.collectingAndThen(Collectors.toSet(), Collections::unmodifiableSet)); - public static final Predicate> VALID_QUERY_PARAMETER_MAP_ENTRY_PREDICATE = - e -> isValidQueryParameterName(e.getKey()); - public static String validateTableName(String name) { return Type.TABLE.validate(name); } From e825451c2ddd763fda483189c5328b78d332b009 Mon Sep 17 00:00:00 2001 From: Nathaniel Bauernfeind Date: Fri, 16 Feb 2024 11:49:52 -0700 Subject: [PATCH 2/2] Ryan's feedback --- .../python/PythonDeephavenSession.java | 33 +++++++------- .../engine/context/EmptyQueryScope.java | 17 +++---- .../engine/context/PoisonedQueryScope.java | 8 +++- .../deephaven/engine/context/QueryScope.java | 11 ++--- .../engine/context/StandaloneQueryScope.java | 41 ++++++++++------- .../java/io/deephaven/engine/sql/Sql.java | 6 +-- .../engine/util/AbstractScriptSession.java | 18 ++++---- .../engine/util/GroovyDeephavenSession.java | 8 +--- .../util/NoLanguageDeephavenSession.java | 6 +-- .../io/deephaven/engine/util/PythonScope.java | 44 ------------------- .../engine/util/PythonScopeJpyImpl.java | 12 ----- .../server/console/ScopeTicketResolver.java | 7 +-- 12 files changed, 78 insertions(+), 133 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 1eff41c72eb..d362acecc4f 100644 --- a/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java +++ b/Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java @@ -7,7 +7,6 @@ import io.deephaven.base.verify.Assert; import io.deephaven.configuration.Configuration; import io.deephaven.engine.context.ExecutionContext; -import io.deephaven.engine.context.StandaloneQueryScope; import io.deephaven.engine.exceptions.CancellationException; import io.deephaven.engine.context.QueryScope; import io.deephaven.engine.updategraph.OperationInitializer; @@ -47,7 +46,6 @@ import java.util.concurrent.Executors; import java.util.function.Consumer; import java.util.function.Function; -import java.util.function.Predicate; import java.util.stream.Collectors; /** @@ -256,11 +254,12 @@ protected Changes createDiff(PythonSnapshot from, PythonSnapshot to, RuntimeExce } @Override - protected Set getVariableNames(Predicate allowName) { - return scope.currentScope().copy().keySet().stream() - .map(scope::convertStringKey) - .filter(allowName) - .collect(Collectors.toUnmodifiableSet()); + protected Set getVariableNames() { + try (final PyDictWrapper currScope = scope.currentScope().copy()) { + return currScope.keySet().stream() + .map(scope::convertStringKey) + .collect(Collectors.toSet()); + } } @Override @@ -309,17 +308,19 @@ protected Map getAllValues( @NotNull final QueryScope.ParamFilter filter) { final Map result = new HashMap<>(); - for (final Map.Entry entry : scope.currentScope().copy().entrySet()) { - final String name = scope.convertStringKey(entry.getKey()); - Object value = scope.convertValue(entry.getValue()); - if (valueMapper != null) { - value = valueMapper.apply(value); - } + try (final PyDictWrapper currScope = scope.currentScope().copy()) { + for (final Map.Entry entry : currScope.entrySet()) { + final String name = scope.convertStringKey(entry.getKey()); + Object value = scope.convertValue(entry.getValue()); + if (valueMapper != null) { + value = valueMapper.apply(value); + } - // noinspection unchecked - if (filter.accept(name, (T) value)) { // noinspection unchecked - result.put(name, (T) value); + if (filter.accept(name, (T) value)) { + // noinspection unchecked + result.put(name, (T) value); + } } } 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 860c7400a7f..9b0a060f736 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 @@ -5,14 +5,10 @@ import io.deephaven.engine.liveness.LivenessReferent; import org.jetbrains.annotations.NotNull; -import org.jetbrains.annotations.Nullable; import java.lang.ref.WeakReference; -import java.util.Collections; -import java.util.Map; -import java.util.Set; +import java.util.*; import java.util.function.Function; -import java.util.function.Predicate; import java.util.stream.Stream; public class EmptyQueryScope implements QueryScope { @@ -22,7 +18,7 @@ private EmptyQueryScope() {} @Override public Set getParamNames() { - return Collections.emptySet(); + return new HashSet<>(); } @Override @@ -51,8 +47,13 @@ public void putParam(String name, T value) { } @Override - public Map toMap(@Nullable Function valueMapper, @NotNull ParamFilter filter) { - return Collections.emptyMap(); + public Map toMap(@NotNull ParamFilter filter) { + return new HashMap<>(); + } + + @Override + public Map toMap(@NotNull Function valueMapper, @NotNull ParamFilter filter) { + return new HashMap<>(); } @Override 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 18f2bd1ca32..f4a2cec1804 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 @@ -6,7 +6,6 @@ import io.deephaven.engine.liveness.LivenessReferent; import io.deephaven.util.ExecutionContextRegistrationException; import org.jetbrains.annotations.NotNull; -import org.jetbrains.annotations.Nullable; import java.lang.ref.WeakReference; import java.util.Map; @@ -55,7 +54,12 @@ public void putParam(String name, T value) { } @Override - public Map toMap(@Nullable Function valueMapper, @NotNull ParamFilter filter) { + public Map toMap(@NotNull ParamFilter filter) { + return fail(); + } + + @Override + public Map toMap(@NotNull Function valueMapper, @NotNull ParamFilter filter) { 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 76d2221c5d0..36e68d66b20 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 @@ -6,7 +6,6 @@ import io.deephaven.engine.liveness.LivenessNode; import io.deephaven.base.log.LogOutput; import io.deephaven.base.log.LogOutputAppendable; -import io.deephaven.util.annotations.FinalDefault; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -84,7 +83,7 @@ default QueryScopeParam[] getParams(final Collection names) throws Mi /** * Get all known scope variable names. * - * @return A collection of scope variable names. + * @return A caller-owned mutable collection of scope variable names. */ Set getParamNames(); @@ -133,6 +132,7 @@ default QueryScopeParam[] getParams(final Collection names) throws Mi */ void putParam(final String name, final T value); + @FunctionalInterface interface ParamFilter { boolean accept(String name, T value); } @@ -144,10 +144,7 @@ interface ParamFilter { * @param filter a predicate to filter the map entries * @return a caller-owned mutable map with all known variables and their values. */ - @FinalDefault - default Map toMap(@NotNull ParamFilter filter) { - return toMap(null, filter); - } + Map toMap(@NotNull ParamFilter filter); /** * Returns a mutable map with all objects in the scope. @@ -161,7 +158,7 @@ default Map toMap(@NotNull ParamFilter filter) { * @param the type of the mapped values */ Map toMap( - @Nullable Function valueMapper, @NotNull ParamFilter filter); + @NotNull Function valueMapper, @NotNull ParamFilter filter); /** * 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 f660fa9fa74..f000f598cf7 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 @@ -1,6 +1,5 @@ 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; @@ -9,8 +8,8 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.Map; import java.util.Set; import java.util.function.Function; @@ -21,11 +20,11 @@ public class StandaloneQueryScope extends LivenessArtifact implements QueryScope { private final Map> valueRetrievers = - Collections.synchronizedMap(new KeyedObjectHashMap<>(new ValueRetrieverNameKey())); + new KeyedObjectHashMap<>(new ValueRetrieverNameKey()); @Override public Set getParamNames() { - return valueRetrievers.keySet(); + return new HashSet<>(valueRetrievers.keySet()); } @Override @@ -65,7 +64,6 @@ 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); } @@ -80,26 +78,35 @@ public void putParam(final String name, final T value) { } } + @Override + public Map toMap(@NotNull final ParamFilter filter) { + return toMapInternal(null, filter); + } + @Override public Map toMap( + @NotNull final Function valueMapper, + @NotNull final ParamFilter filter) { + return toMapInternal(valueMapper, filter); + } + + private Map toMapInternal( @Nullable final Function valueMapper, @NotNull final ParamFilter filter) { final Map result = new HashMap<>(); - synchronized (valueRetrievers) { - for (final Map.Entry> entry : valueRetrievers.entrySet()) { - final String name = entry.getKey(); - final ValueRetriever valueRetriever = entry.getValue(); - Object value = valueRetriever.getValue(); - if (valueMapper != null) { - value = valueMapper.apply(value); - } + for (final Map.Entry> entry : valueRetrievers.entrySet()) { + final String name = entry.getKey(); + final ValueRetriever valueRetriever = entry.getValue(); + Object value = valueRetriever.getValue(); + if (valueMapper != null) { + value = valueMapper.apply(value); + } + // noinspection unchecked + if (filter.accept(name, (T) value)) { // noinspection unchecked - if (filter.accept(name, (T) value)) { - // noinspection unchecked - result.put(name, (T) value); - } + result.put(name, (T) value); } } 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 1ba2fe0aecc..ac4e29c53c2 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 @@ -83,10 +83,8 @@ private static Map currentScriptSessionNamedTables() { // getVariables() is inefficient // See SQLTODO(catalog-reader-implementation) final QueryScope queryScope = ExecutionContext.getContext().getQueryScope(); - return queryScope.toMap(wrapped -> { - final Object value = queryScope.unwrapObject(wrapped); - return value instanceof Table ? (Table) value : null; - }, (n, t) -> t != null); + // noinspection unchecked,rawtypes + return (Map) (Map) queryScope.toMap(queryScope::unwrapObject, (n, t) -> t instanceof Table); } private static TableHeader adapt(TableDefinition tableDef) { 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 08f4c88bb52..988dc6d702c 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 @@ -5,7 +5,6 @@ import com.github.f4b6a3.uuid.UuidCreator; import io.deephaven.UncheckedDeephavenException; -import io.deephaven.api.util.NameValidator; import io.deephaven.base.FileUtils; import io.deephaven.configuration.CacheDir; import io.deephaven.engine.context.*; @@ -30,7 +29,6 @@ import java.nio.file.Path; import java.util.*; import java.util.function.Function; -import java.util.function.Predicate; import static io.deephaven.engine.table.Table.NON_DISPLAY_TABLE; @@ -264,10 +262,9 @@ 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 + * @return a caller-owned mutable set of variable names */ - protected abstract Set getVariableNames(Predicate allowName); + protected abstract Set getVariableNames(); /** * Check if the scope has the given variable name. @@ -315,12 +312,12 @@ public ScriptSession scriptSession() { @Override public Set getParamNames() { - return getVariableNames(NameValidator::isValidQueryParameterName); + return getVariableNames(); } @Override public boolean hasParamName(String name) { - return NameValidator.isValidQueryParameterName(name) && hasVariable(name); + return hasVariable(name); } @Override @@ -359,9 +356,14 @@ public void putParam(final String name, final T value) { } } + @Override + public Map toMap(@NotNull final ParamFilter filter) { + return AbstractScriptSession.this.getAllValues(null, filter); + } + @Override public Map toMap( - @Nullable final Function valueMapper, + @NotNull final Function valueMapper, @NotNull final ParamFilter filter) { return AbstractScriptSession.this.getAllValues(valueMapper, filter); } 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 1c5bc4907da..58f33db9243 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 @@ -18,7 +18,6 @@ import io.deephaven.engine.context.*; import io.deephaven.configuration.Configuration; import io.deephaven.engine.exceptions.CancellationException; -import io.deephaven.api.util.NameValidator; import io.deephaven.engine.rowset.RowSet; import io.deephaven.engine.rowset.TrackingRowSet; import io.deephaven.engine.table.ColumnSource; @@ -71,7 +70,6 @@ import java.util.*; import java.util.concurrent.ConcurrentHashMap; import java.util.function.Function; -import java.util.function.Predicate; import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Collectors; @@ -744,9 +742,9 @@ public void close() { } @Override - protected Set getVariableNames(Predicate allowName) { + protected Set getVariableNames() { synchronized (bindingBackingMap) { - return bindingBackingMap.keySet().stream().filter(allowName).collect(Collectors.toUnmodifiableSet()); + return new HashSet<>(bindingBackingMap.keySet()); } } @@ -757,8 +755,6 @@ protected boolean hasVariable(String name) { @Override 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 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 278ca5badd0..8c162ae06f2 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 @@ -11,8 +11,6 @@ import java.util.*; import java.util.function.Function; -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 @@ -75,9 +73,9 @@ protected void evaluate(String command, @Nullable String scriptName) { } @Override - protected Set getVariableNames(Predicate allowName) { + protected Set getVariableNames() { synchronized (variables) { - return variables.keySet().stream().filter(allowName).collect(Collectors.toUnmodifiableSet()); + return new HashSet<>(variables.keySet()); } } diff --git a/engine/table/src/main/java/io/deephaven/engine/util/PythonScope.java b/engine/table/src/main/java/io/deephaven/engine/util/PythonScope.java index 51e5f8b9fdc..c5b205acf43 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/PythonScope.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/PythonScope.java @@ -6,11 +6,7 @@ import org.jpy.PyDictWrapper; import org.jpy.PyObject; -import java.util.Collection; -import java.util.Map.Entry; import java.util.Optional; -import java.util.stream.Collectors; -import java.util.stream.Stream; /** * A collection of methods around retrieving objects from the given Python scope. @@ -30,26 +26,6 @@ public interface PythonScope { */ Optional getValueRaw(String name); - /** - * Retrieves all keys from the give scope. - *

- * No conversion is done. - *

- * Technically, the keys can be tuples... - * - * @return the keys - */ - Stream getKeysRaw(); - - /** - * Retrieves all keys and values from the given scope. - *

- * No conversion is done. - * - * @return the keys and values - */ - Stream> getEntriesRaw(); - /** * The helper method to turn a raw key into a string key. *

@@ -120,26 +96,6 @@ default Optional getValueUnchecked(String name) { .map(x -> (T) x); } - /** - * Equivalent to {@link #getKeysRaw()}.map({@link #convertStringKey(Object)}) - * - * @return the string keys - */ - default Stream getKeys() { - return getKeysRaw() - .map(this::convertStringKey); - } - - /** - * Equivalent to {@link #getKeys()}.collect(someCollector) - * - * @return the string keys, as a collection - */ - default Collection getKeysCollection() { - return getKeys() - .collect(Collectors.toList()); - } - /** * @return the Python's __main__ module namespace */ diff --git a/engine/table/src/main/java/io/deephaven/engine/util/PythonScopeJpyImpl.java b/engine/table/src/main/java/io/deephaven/engine/util/PythonScopeJpyImpl.java index 117eff8b145..6da26fa5b35 100644 --- a/engine/table/src/main/java/io/deephaven/engine/util/PythonScopeJpyImpl.java +++ b/engine/table/src/main/java/io/deephaven/engine/util/PythonScopeJpyImpl.java @@ -13,10 +13,8 @@ import java.util.ArrayDeque; import java.util.Deque; -import java.util.Map.Entry; import java.util.Optional; import java.util.concurrent.ExecutionException; -import java.util.stream.Stream; public class PythonScopeJpyImpl implements PythonScope { private static volatile boolean cacheEnabled = @@ -56,16 +54,6 @@ public Optional getValueRaw(String name) { return Optional.ofNullable(currentScope().get(name)); } - @Override - public Stream getKeysRaw() { - return currentScope().keySet().stream(); - } - - @Override - public Stream> getEntriesRaw() { - return currentScope().entrySet().stream(); - } - @Override public boolean containsKey(String name) { return currentScope().containsKey(name); 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 3458b5ed7b0..8f04dc497de 100644 --- a/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java +++ b/server/src/main/java/io/deephaven/server/console/ScopeTicketResolver.java @@ -72,11 +72,8 @@ public SessionState.ExportObject flightInfoFor( @Override public void forAllFlightInfo(@Nullable final SessionState session, final Consumer visitor) { final QueryScope queryScope = ExecutionContext.getContext().getQueryScope(); - queryScope.toMap(wrapped -> { - final Object value = queryScope.unwrapObject(wrapped); - return value instanceof Table ? (Table) value : null; - }, (n, t) -> t != null).forEach((name, table) -> visitor - .accept(TicketRouter.getFlightInfo(table, descriptorForName(name), flightTicketForName(name)))); + queryScope.toMap(queryScope::unwrapObject, (n, t) -> t instanceof Table).forEach((name, table) -> visitor + .accept(TicketRouter.getFlightInfo((Table) table, descriptorForName(name), flightTicketForName(name)))); } @Override