Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop Usage of ensureGil from PythonDeephavenSession and Copy Current Scope #5145

Merged
merged 2 commits into from
Feb 17, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -255,7 +257,10 @@ protected Changes createDiff(PythonSnapshot from, PythonSnapshot to, RuntimeExce

@Override
protected Set<String> getVariableNames(Predicate<String> allowName) {
return PyLib.ensureGil(() -> scope.getKeys().filter(allowName).collect(Collectors.toUnmodifiableSet()));
return scope.currentScope().copy().keySet().stream()
nbauernfeind marked this conversation as resolved.
Show resolved Hide resolved
.map(scope::convertStringKey)
.filter(allowName)
.collect(Collectors.toUnmodifiableSet());
}

@Override
Expand Down Expand Up @@ -299,19 +304,22 @@ protected synchronized Object setVariable(String name, @Nullable Object newValue
}

@Override
protected Map<String, Object> getAllValues(@NotNull final Predicate<Map.Entry<String, Object>> predicate) {
final HashMap<String, Object> result = PyLib.ensureGil(
() -> scope.getEntriesRaw().<Map.Entry<String, PyObject>>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<Map.Entry<String, Object>> iter = result.entrySet().iterator();
while (iter.hasNext()) {
final Map.Entry<String, Object> entry = iter.next();
entry.setValue(scope.convertValue((PyObject) entry.getValue()));
if (!predicate.test(entry)) {
iter.remove();
protected <T> Map<String, T> getAllValues(
@Nullable final Function<Object, T> valueMapper,
@NotNull final QueryScope.ParamFilter<T> filter) {
final Map<String, T> result = new HashMap<>();

for (final Map.Entry<PyObject, PyObject> 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);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
nbauernfeind marked this conversation as resolved.
Show resolved Hide resolved
import java.util.stream.Stream;

Expand Down Expand Up @@ -49,7 +51,7 @@ public <T> void putParam(String name, T value) {
}

@Override
public Map<String, Object> toMap(@NotNull Predicate<Map.Entry<String, Object>> predicate) {
public <T> Map<String, T> toMap(@Nullable Function<Object, T> valueMapper, @NotNull ParamFilter<T> filter) {
return Collections.emptyMap();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -54,7 +55,7 @@ public <T> void putParam(String name, T value) {
}

@Override
public Map<String, Object> toMap(@NotNull Predicate<Map.Entry<String, Object>> predicate) {
public <T> Map<String, T> toMap(@Nullable Function<Object, T> valueMapper, @NotNull ParamFilter<T> filter) {
return fail();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -133,25 +133,35 @@ default QueryScopeParam<?>[] getParams(final Collection<String> names) throws Mi
*/
<T> void putParam(final String name, final T value);

interface ParamFilter<T> {
nbauernfeind marked this conversation as resolved.
Show resolved Hide resolved
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<String, Object> toMap(@NotNull Predicate<Map.Entry<String, Object>> predicate);
@FinalDefault
default Map<String, Object> toMap(@NotNull ParamFilter<Object> 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.
* <p>
* 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 <T> the type of the mapped values
*/
@FinalDefault
default Map<String, Object> toMap() {
return toMap(entry -> true);
}
<T> Map<String, T> toMap(
@Nullable Function<Object, T> valueMapper, @NotNull ParamFilter<T> filter);
nbauernfeind marked this conversation as resolved.
Show resolved Hide resolved

/**
* Removes any wrapping that exists on a scope param object so that clients can fetch them. Defaults to returning
Expand All @@ -167,7 +177,7 @@ default Object unwrapObject(@Nullable Object object) {
@Override
default LogOutput append(@NotNull final LogOutput logOutput) {
logOutput.append('{');
for (final Map.Entry<String, Object> param : toMap().entrySet()) {
for (final Map.Entry<String, Object> param : toMap((name, value) -> true).entrySet()) {
logOutput.nl().append(param.getKey()).append("=");
Object paramValue = param.getValue();
if (paramValue == this) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, ValueRetriever<?>> valueRetrievers =
new KeyedObjectHashMap<>(new ValueRetrieverNameKey());
private final Map<String, ValueRetriever<?>> valueRetrievers =
Collections.synchronizedMap(new KeyedObjectHashMap<>(new ValueRetrieverNameKey()));
nbauernfeind marked this conversation as resolved.
Show resolved Hide resolved

@Override
public Set<String> getParamNames() {
Expand Down Expand Up @@ -80,12 +81,28 @@ public <T> void putParam(final String name, final T value) {
}

@Override
public Map<String, Object> toMap(@NotNull final Predicate<Map.Entry<String, Object>> predicate) {
final HashMap<String, Object> 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 <T> Map<String, T> toMap(
@Nullable final Function<Object, T> valueMapper,
@NotNull final ParamFilter<T> filter) {
final Map<String, T> result = new HashMap<>();

synchronized (valueRetrievers) {
nbauernfeind marked this conversation as resolved.
Show resolved Hide resolved
for (final Map.Entry<String, ValueRetriever<?>> 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;
}

Expand Down
15 changes: 5 additions & 10 deletions engine/sql/src/main/java/io/deephaven/engine/sql/Sql.java
Original file line number Diff line number Diff line change
Expand Up @@ -82,16 +82,11 @@ private static Scope scope(Map<String, Table> scope, Map<TicketTable, Table> out
private static Map<String, Table> currentScriptSessionNamedTables() {
// getVariables() is inefficient
// See SQLTODO(catalog-reader-implementation)
QueryScope queryScope = ExecutionContext.getContext().getQueryScope();
final Map<String, Table> 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 -> {
nbauernfeind marked this conversation as resolved.
Show resolved Hide resolved
final Object value = queryScope.unwrapObject(wrapped);
return value instanceof Table ? (Table) value : null;
}, (n, t) -> t != null);
}

private static TableHeader adapt(TableDefinition tableDef) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public synchronized void init(TableDefinition tableDefinition) {
try {
final QueryScope queryScope = ExecutionContext.getContext().getQueryScope();
final Map<String, Object> queryScopeVariables = queryScope.toMap(
NameValidator.VALID_QUERY_PARAMETER_MAP_ENTRY_PREDICATE);
(name, value) -> NameValidator.isValidQueryParameterName(name));
for (Map.Entry<String, Object> param : queryScopeVariables.entrySet()) {
possibleVariables.put(param.getKey(), QueryScopeParamTypeUtil.getDeclaredClass(param.getValue()));
Type declaredType = QueryScopeParamTypeUtil.getDeclaredType(param.getValue());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public static QueryLanguageParser.Result getCompiledFormula(Map<String, ColumnDe

final ExecutionContext context = ExecutionContext.getContext();
final Map<String, Object> queryScopeVariables = context.getQueryScope().toMap(
NameValidator.VALID_QUERY_PARAMETER_MAP_ENTRY_PREDICATE);
(name, value) -> NameValidator.isValidQueryParameterName(name));
for (Map.Entry<String, Object> param : queryScopeVariables.entrySet()) {
if (possibleVariables.containsKey(param.getKey())) {
// skip any existing matches
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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.
* <p>
* 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 <T> the type of the mapped values
*/
protected abstract Map<String, Object> getAllValues(@NotNull Predicate<Map.Entry<String, Object>> predicate);
protected abstract <T> Map<String, T> getAllValues(
@Nullable Function<Object, T> valueMapper, @NotNull QueryScope.ParamFilter<T> filter);

// -----------------------------------------------------------------------------------------------------------------
// ScriptSession-based QueryScope implementation, with no remote scope or object reflection support
Expand All @@ -325,38 +326,25 @@ public boolean hasParamName(String name) {
@Override
public <T> QueryScopeParam<T> 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> 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> 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;
}
}

@Override
public <T> void putParam(final String name, final T value) {
NameValidator.validateQueryParameterName(name);
nbauernfeind marked this conversation as resolved.
Show resolved Hide resolved
if (value instanceof LivenessReferent && DynamicNode.notDynamicOrIsRefreshing(value)) {
manage((LivenessReferent) value);
}
Expand All @@ -372,8 +360,10 @@ public <T> void putParam(final String name, final T value) {
}

@Override
public Map<String, Object> toMap(@NotNull final Predicate<Map.Entry<String, Object>> predicate) {
return AbstractScriptSession.this.getAllValues(predicate);
public <T> Map<String, T> toMap(
@Nullable final Function<Object, T> valueMapper,
@NotNull final ParamFilter<T> filter) {
return AbstractScriptSession.this.getAllValues(valueMapper, filter);
}

@Override
Expand Down
Loading
Loading