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

Extract variable api from ScriptSession, let ScriptSession guard reads #4970

Merged
merged 27 commits into from
Jan 22, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
afcddf7
Extract variable api from ScriptSession, let ScriptSession guard reads
niloc132 Dec 20, 2023
4e2beee
Further shift from script session to query scope, including liveness
niloc132 Dec 20, 2023
7041746
Restore method needed for python local scope hack
niloc132 Dec 20, 2023
06ce7f0
Fix test setup, correct unwrapping in sql
niloc132 Dec 21, 2023
7b57fa2
Partial review feedback
niloc132 Dec 21, 2023
5a1a7cf
Remove VariableProvider, use QueryScope instead
niloc132 Dec 21, 2023
d309e35
Remove dead method, dead cleanup code in flight round trip test
niloc132 Dec 21, 2023
8b7c820
Remove more dead code, fix potential lock order issue
niloc132 Dec 21, 2023
bd2abcc
Field ordering
niloc132 Dec 21, 2023
8f8102e
Correct comment layout
niloc132 Dec 21, 2023
4efb27e
Review feedback:
niloc132 Dec 22, 2023
c049476
Revert attempt to use optional (incompatible with null returns)
niloc132 Dec 22, 2023
ff6a3ba
Merge branch 'main' into 4040-cleanup-5
niloc132 Jan 8, 2024
deb0a77
Review feedback
niloc132 Jan 8, 2024
c5c1ce7
Old requested rename, and added missing transferTo
niloc132 Jan 8, 2024
42ba9be
Finish implementation (don't trust IJ's refactor tools..?)
niloc132 Jan 8, 2024
0b6caa8
Review feedback
niloc132 Jan 9, 2024
d3f0472
Merge branch 'main' into 4040-cleanup-5
niloc132 Jan 9, 2024
e3a0583
Merge branch 'main' into 4040-cleanup-5
niloc132 Jan 9, 2024
cb8646d
Live code review
niloc132 Jan 11, 2024
abb9dcb
Live review feedback
niloc132 Jan 12, 2024
25e4da7
Clean up standalone impl
niloc132 Jan 12, 2024
7843871
Merge branch 'main' into 4040-cleanup-5
niloc132 Jan 12, 2024
d2208c3
spotless
niloc132 Jan 12, 2024
7598e14
Review feedback
niloc132 Jan 18, 2024
668596d
Review feedback
niloc132 Jan 19, 2024
1e32a0e
Add missing queryScope.unwrapObject() calls
niloc132 Jan 19, 2024
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
Prev Previous commit
Next Next commit
Partial review feedback
  • Loading branch information
niloc132 committed Dec 21, 2023
commit 7b57fa217d8ad008b8807f0e0b86b85e04aff241
Original file line number Diff line number Diff line change
Expand Up @@ -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> T getVariable(String name) throws QueryScope.MissingVariableException {
return (T) scope
.getValue(name)
.orElseThrow(() -> new QueryScope.MissingVariableException("No variable for: " + name));
}

@Override
public <T> T getVariable(String name, T defaultValue) {
return scope
.<T>getValueUnchecked(name)
.orElse(defaultValue);
}

@SuppressWarnings("unused")
@ScriptApi
Expand Down Expand Up @@ -210,13 +204,6 @@ protected void evaluate(String command, String scriptName) {
}
}

@Override
public Map<String, Object> getVariables() {
final Map<String, Object> 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;
Expand Down Expand Up @@ -281,17 +268,17 @@ private Object maybeUnwrap(PyObject o) {
}

@Override
public Set<String> getVariableNames() {
return Collections.unmodifiableSet(scope.getKeys().collect(Collectors.toSet()));
protected Set<String> getVariableNames() {
return scope.getKeys().collect(Collectors.toUnmodifiableSet());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As is, we end up making three copies of this key-set! PyDictWrapper#keySet creates a copy of the keys (instead of a view). Then we copy it a second time in PythonDeephavenSession#getVariableNames using a collector. Finally, we copy it a third time in ScriptSessionQueryScope#getParamNames to filter out non-valid names.

We should pass a Predicate to getVariableNames so that the Groovy impl can filter under the param-map's object monitor. That makes python a two-copy impl and the groovy one a single-copy. (instead of three and two respectively)

Do we need to synchronize w/setVariable to avoid changing the map while PyDictWrapper#keySet is making the initial copy? It looks like if you some setVariable to null while iterating that the result set could include the removed variable and not the variable that was next in the dict. Also, note that PyListWrapper#iterator() is not protected from size changes and will appears to return null for the last item in the dict (if the size is decreased).

It looks like this would cause an NPE in the LinkedHashSet construction within PyDictWrapper#keySet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points, will pass a predicate, stream results into the unmodifiable set.

Racing with setVariable is somewhat fraught - after all, setVariable isn't the only way to assign values into the collection, python itself could be assigning/removing something. I think technically I can use PyLib.ensureGil() here, to guarantee that reads of python structures can't change while reading, in place of the synchronized blocks that Groovy's impl uses). Any AbstractScriptSession override that makes more than one call into python should use this (and note that it is not safe to use this to call into arbitrary python, as that python code could end up dropping the GIL despite our efforts - ensureGil only lets us be sure that we have the GIL in Java, JNI, and our own C).

}

@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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
nbauernfeind marked this conversation as resolved.
Show resolved Hide resolved
// A column name or one of the special variables
pyCallableWrapper.addChunkArgument(new ColumnChunkArgument(name, argTypes[i]));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -277,46 +281,27 @@ private Class<?> getVariableType(final String var) {


public TableDefinition getTableDefinition(final String var) {
niloc132 marked this conversation as resolved.
Show resolved Hide resolved
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.
* <p/>
* 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.
* <p>
* 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 <T> the type of the variable
* @return the value of the variable, or defaultValue if not present
*/
protected abstract <T> 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<String, Object> getVariables();
protected abstract <T> 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<String> getVariableNames();

Expand Down Expand Up @@ -363,7 +348,10 @@ public Class<?> getVariableType(String var) {
public <T> 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();
}
Expand Down Expand Up @@ -418,7 +406,7 @@ public Set<String> getParamNames() {
result.add(name);
}
}
return result;
return Collections.unmodifiableSet(result);
}

@Override
Expand All @@ -432,15 +420,17 @@ protected <T> QueryScopeParam<T> 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
public <T> T readParamValue(final String name) throws QueryScope.MissingVariableException {
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);
niloc132 marked this conversation as resolved.
Show resolved Hide resolved
}
Expand All @@ -455,10 +445,11 @@ 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);
}
getVariableProvider().setVariable(NameValidator.validateQueryParameterName(name), value);
getVariableProvider().setVariable(name, value);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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> 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> 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);
}
Expand Down Expand Up @@ -704,12 +693,6 @@ private static boolean isAnInteger(final String s) {
}
}

@Override
public Map<String, Object> getVariables() {
// noinspection unchecked
return Collections.unmodifiableMap(groovyShell.getContext().getVariables());
}

@Override
protected GroovySnapshot emptySnapshot() {
return new GroovySnapshot(Collections.emptyMap());
Expand Down Expand Up @@ -755,18 +738,19 @@ public void close() {
}
}

public Set<String> getVariableNames() {
@Override
protected Set<String> 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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> T getVariable(String name, T defaultValue) {
try {
// noinspection unchecked
return (T) getVariable(name);
} catch (QueryScope.MissingVariableException e) {
return defaultValue;
protected <T> T getVariable(String name) throws QueryScope.MissingVariableException {
if (!variables.containsKey(name)) {
niloc132 marked this conversation as resolved.
Show resolved Hide resolved
throw new QueryScope.MissingVariableException("No global variable for: " + name);
}
// noinspection unchecked
return (T) variables.get(name);
}

@Override
Expand All @@ -81,22 +70,17 @@ protected void evaluate(String command, @Nullable String scriptName) {
}

@Override
public Map<String, Object> getVariables() {
return Collections.unmodifiableMap(variables);
}

@Override
public Set<String> getVariableNames() {
return Collections.unmodifiableSet(variables.keySet());
protected Set<String> getVariableNames() {
return Set.copyOf(variables.keySet());
niloc132 marked this conversation as resolved.
Show resolved Hide resolved
}

@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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading
Loading