Skip to content

Commit

Permalink
Ryan's feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@nbauernfeind
nbauernfeind committed Feb 9, 2024
1 parent 847aa4b commit 900310d
Show file tree
Hide file tree
Showing 14 changed files with 83 additions and 86 deletions.
4 changes: 2 additions & 2 deletions Integrations/src/main/java/io/deephaven/integrations/python/PythonDeephavenSession.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ protected synchronized Object setVariable(String name, @Nullable Object newValue
protected Map<String, Object> getAllValues() {
return PyLib
.ensureGil(() -> scope.getEntries()
.collect(Collectors.toUnmodifiableMap(Map.Entry::getKey, Map.Entry::getValue)));
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)));
}

@Override
Expand All @@ -315,7 +315,7 @@ public String scriptType() {
// TODO core#41 move this logic into the python console instance or scope like this - can go further and move
// isWidget too
@Override
public Object unwrapObject(Object object) {
public Object unwrapObject(@Nullable Object object) {
if (object instanceof PyObject) {
final PyObject pyObject = (PyObject) object;
final Object unwrapped = module.javaify(pyObject);
Expand Down
13 changes: 7 additions & 6 deletions engine/context/src/main/java/io/deephaven/engine/context/QueryScope.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import io.deephaven.base.log.LogOutput;
import io.deephaven.base.log.LogOutputAppendable;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

import java.util.Collection;
import java.util.Map;
Expand Down Expand Up @@ -69,8 +70,8 @@ private MissingVariableException(final Throwable cause) {
* @return A newly-constructed array of newly-constructed Params.
* @throws QueryScope.MissingVariableException If any of the named scope variables does not exist.
*/
default QueryScopeParam[] getParams(final Collection<String> names) throws MissingVariableException {
final QueryScopeParam[] result = new QueryScopeParam[names.size()];
default QueryScopeParam<?>[] getParams(final Collection<String> names) throws MissingVariableException {
final QueryScopeParam<?>[] result = new QueryScopeParam[names.size()];
int pi = 0;
for (final String name : names) {
result[pi++] = createParam(name);
Expand Down Expand Up @@ -131,10 +132,10 @@ default QueryScopeParam[] getParams(final Collection<String> names) throws Missi
<T> void putParam(final String name, final T value);

/**
* 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.
* 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.
*
* @return an immutable map with all known variables and their values.
* @return a caller-owned mutable map with all known variables and their values.
*/
Map<String, Object> toMap();

Expand All @@ -145,7 +146,7 @@ default QueryScopeParam[] getParams(final Collection<String> names) throws Missi
* @param object the scoped object
* @return an obj which can be consumed by a client
*/
default Object unwrapObject(Object object) {
default Object unwrapObject(@Nullable Object object) {
return object;
}

Expand Down
2 changes: 0 additions & 2 deletions engine/context/src/main/java/io/deephaven/engine/context/QueryScopeParam.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@

public class QueryScopeParam<T> {

public static final QueryScopeParam<?>[] ZERO_LENGTH_PARAM_ARRAY = new QueryScopeParam[0];

private final String name;
private final T value;

Expand Down
2 changes: 1 addition & 1 deletion engine/context/src/main/java/io/deephaven/engine/context/StandaloneQueryScope.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public <T> void putParam(final String name, final T value) {
@Override
public Map<String, Object> toMap() {
return valueRetrievers.entrySet().stream()
.collect(Collectors.toUnmodifiableMap(Map.Entry::getKey, e -> e.getValue().value));
.collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().value));
}

private static class ValueRetriever<T> {
Expand Down
43 changes: 20 additions & 23 deletions engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@
import io.deephaven.engine.util.PyCallableWrapperJpyImpl;
import io.deephaven.internal.log.LoggerFactory;
import io.deephaven.io.logger.Logger;
import io.deephaven.util.annotations.TestUseOnly;
import io.deephaven.util.type.TypeUtils;
import io.deephaven.vector.ByteVector;
import io.deephaven.vector.CharVector;
Expand Down Expand Up @@ -143,7 +144,7 @@ public final class QueryLanguageParser extends GenericVisitorAdapter<Class<?>, Q

private final Map<String, Class<?>> variables;
private final Map<String, Class<?>[]> variableTypeArguments;
private final Map<String, QueryScopeParam<?>> possibleParams;
private final Map<String, Object> possibleParams;

private final HashSet<String> variablesUsed = new HashSet<>();

Expand Down Expand Up @@ -189,7 +190,8 @@ public final class QueryLanguageParser extends GenericVisitorAdapter<Class<?>, Q
* @param variableTypeArguments A map of the names of scope variables to their type arguments
* @throws QueryLanguageParseException If any exception or error is encountered
*/
public QueryLanguageParser(
@TestUseOnly
QueryLanguageParser(
String expression,
Collection<Package> packageImports,
Collection<Class<?>> classImports,
Expand All @@ -212,7 +214,7 @@ public QueryLanguageParser(
* imported.
* @param variables A map of the names of scope variables to their types
* @param variableTypeArguments A map of the names of scope variables to their type arguments
* @param possibleParams A map of the names of query scope variables to their values
* @param possibleParams A mutable map of the names of query scope variables to their values
* @throws QueryLanguageParseException If any exception or error is encountered
*/
public QueryLanguageParser(
Expand All @@ -222,7 +224,7 @@ public QueryLanguageParser(
Collection<Class<?>> staticImports,
Map<String, Class<?>> variables,
Map<String, Class<?>[]> variableTypeArguments,
Map<String, QueryScopeParam<?>> possibleParams) throws QueryLanguageParseException {
Map<String, Object> possibleParams) throws QueryLanguageParseException {
this(expression, packageImports, classImports, staticImports, variables,
variableTypeArguments, possibleParams, true);
}
Expand All @@ -240,7 +242,7 @@ public QueryLanguageParser(
* @param variables A map of the names of scope variables to their types
* @param variableTypeArguments A map of the names of scope variables to their type arguments
* @param unboxArguments If true it will unbox the query scope arguments
* @param possibleParams A map of the names of query scope variables to their values
* @param possibleParams A mutable map of the names of query scope variables to their values
* @throws QueryLanguageParseException If any exception or error is encountered
*/
public QueryLanguageParser(
Expand All @@ -250,7 +252,7 @@ public QueryLanguageParser(
Collection<Class<?>> staticImports,
Map<String, Class<?>> variables,
Map<String, Class<?>[]> variableTypeArguments,
Map<String, QueryScopeParam<?>> possibleParams,
Map<String, Object> possibleParams,
boolean unboxArguments)
throws QueryLanguageParseException {
this(
Expand All @@ -273,7 +275,7 @@ public QueryLanguageParser(
final Collection<Class<?>> staticImports,
final Map<String, Class<?>> variables,
final Map<String, Class<?>[]> variableTypeArguments,
final Map<String, QueryScopeParam<?>> possibleParams,
final Map<String, Object> possibleParams,
final boolean unboxArguments,
final boolean verifyIdempotence,
final @NotNull String pyCallableWrapperImplName) throws QueryLanguageParseException {
Expand All @@ -283,7 +285,7 @@ public QueryLanguageParser(
this.variables = variables == null ? Collections.emptyMap() : Map.copyOf(variables);
this.variableTypeArguments =
variableTypeArguments == null ? Collections.emptyMap() : Map.copyOf(variableTypeArguments);
this.possibleParams = possibleParams == null ? Collections.emptyMap() : Map.copyOf(possibleParams);
this.possibleParams = possibleParams == null ? new HashMap<>() : possibleParams;
this.unboxArguments = unboxArguments;

Assert.nonempty(pyCallableWrapperImplName, "pyCallableWrapperImplName");
Expand Down Expand Up @@ -336,7 +338,7 @@ public QueryLanguageParser(
// make sure the parser has no problem reparsing its own output and makes no changes to it.
final QueryLanguageParser validationQueryLanguageParser = new QueryLanguageParser(
printedSource, packageImports, classImports, staticImports, variables,
variableTypeArguments, possibleParams, unboxArguments, false, pyCallableWrapperImplName);
variableTypeArguments, possibleParams, false, false, pyCallableWrapperImplName);

final String reparsedSource = validationQueryLanguageParser.result.source;
Assert.equals(
Expand Down Expand Up @@ -2536,11 +2538,7 @@ private Optional<CastExpr> makeCastExpressionForPyCallable(Class<?> retType, Met
private Optional<Class<?>> pyCallableReturnType(@NotNull MethodCallExpr n) {
final PyCallableDetails pyCallableDetails = n.getData(QueryLanguageParserDataKeys.PY_CALLABLE_DETAILS);
final String pyMethodName = pyCallableDetails.pythonMethodName;
final QueryScopeParam<?> queryScopeParam = possibleParams.get(pyMethodName);
if (queryScopeParam == null) {
return Optional.empty();
}
final Object paramValueRaw = queryScopeParam.getValue();
final Object paramValueRaw = possibleParams.get(pyMethodName);
if (!(paramValueRaw instanceof PyCallableWrapper)) {
return Optional.empty();
}
Expand Down Expand Up @@ -2632,11 +2630,10 @@ private void tryVectorizePythonCallable(@NotNull MethodCallExpr n,
// Note: "paramValueRaw" needs to be the *actual PyCallableWrapper* corresponding to the method.
// TODO: Support vectorization of instance methods of constant objects
// ^^ i.e., create a constant for `new PyCallableWrapperImpl(pyScopeObj.getAttribute("pyMethodName"))`
final QueryScopeParam<?> queryScopeParam = possibleParams.get(pyMethodName);
if (queryScopeParam == null) {
if (!possibleParams.containsKey(pyMethodName)) {
throw new IllegalStateException("Resolved Python function name " + pyMethodName + " not found");
}
final Object paramValueRaw = queryScopeParam.getValue();
final Object paramValueRaw = possibleParams.get(pyMethodName);
if (!(paramValueRaw instanceof PyCallableWrapper)) {
throw new IllegalStateException("Resolved Python function name " + pyMethodName + " not callable");
}
Expand Down Expand Up @@ -2746,12 +2743,12 @@ private void prepareVectorizationArgs(
addLiteralArg(expressions[i], argTypes[i], pyCallableWrapper);
} else if (expressions[i] instanceof NameExpr) {
String name = expressions[i].asNameExpr().getNameAsString();
final QueryScopeParam<?> param = possibleParams.get(name);
if (param == null) {
if (!possibleParams.containsKey(name)) {
// A column name or one of the special variables
pyCallableWrapper.addChunkArgument(new ColumnChunkArgument(name, argTypes[i]));
} else {
pyCallableWrapper.addChunkArgument(new ConstantChunkArgument(param.getValue(), argTypes[i]));
pyCallableWrapper.addChunkArgument(
new ConstantChunkArgument(possibleParams.get(name), argTypes[i]));
}
} else {
throw new IllegalStateException("Vectorizability check failed: " + n);
Expand Down Expand Up @@ -3216,15 +3213,15 @@ public static class Result {
private final Class<?> type;
private final String source;
private final HashSet<String> variablesUsed;
private final Map<String, QueryScopeParam<?>> possibleParams;
private final Map<String, Object> possibleParams;
private final boolean isConstantValueExpression;
private final Pair<String, Map<Long, List<MatchPair>>> formulaShiftColPair;

Result(
Class<?> type,
String source,
HashSet<String> variablesUsed,
Map<String, QueryScopeParam<?>> possibleParams,
Map<String, Object> possibleParams,
boolean isConstantValueExpression,
Pair<String, Map<Long, List<MatchPair>>> formulaShiftColPair) {
this.type = Objects.requireNonNull(type, "type");
Expand All @@ -3251,7 +3248,7 @@ public HashSet<String> getVariablesUsed() {
return variablesUsed;
}

public Map<String, QueryScopeParam<?>> getPossibleParams() {
public Map<String, Object> getPossibleParams() {
return possibleParams;
}

Expand Down
13 changes: 6 additions & 7 deletions ...ne/table/src/main/java/io/deephaven/engine/table/impl/select/AbstractConditionFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,18 +97,17 @@ public synchronized void init(TableDefinition tableDefinition) {
final Map<String, Class<?>[]> possibleVariableParameterizedTypes = new HashMap<>();

try {
final Map<String, QueryScopeParam<?>> possibleParams = new HashMap<>();
final QueryScope queryScope = ExecutionContext.getContext().getQueryScope();
for (QueryScopeParam<?> param : queryScope.getParams(queryScope.getParamNames())) {
possibleParams.put(param.getName(), param);
possibleVariables.put(param.getName(), QueryScopeParamTypeUtil.getDeclaredClass(param.getValue()));
final Map<String, Object> possibleParams = queryScope.toMap();
for (Map.Entry<String, Object> param : possibleParams.entrySet()) {
possibleVariables.put(param.getKey(), QueryScopeParamTypeUtil.getDeclaredClass(param.getValue()));
Type declaredType = QueryScopeParamTypeUtil.getDeclaredType(param.getValue());
if (declaredType instanceof ParameterizedType) {
ParameterizedType pt = (ParameterizedType) declaredType;
Class<?>[] paramTypes = Arrays.stream(pt.getActualTypeArguments())
.map(QueryScopeParamTypeUtil::classFromType)
.toArray(Class<?>[]::new);
possibleVariableParameterizedTypes.put(param.getName(), paramTypes);
possibleVariableParameterizedTypes.put(param.getKey(), paramTypes);
}
}

Expand Down Expand Up @@ -207,10 +206,10 @@ public synchronized void init(TableDefinition tableDefinition) {
} else if (arrayColumnToFind != null && tableDefinition.getColumn(arrayColumnToFind) != null) {
usedColumnArrays.add(arrayColumnOuterName);
} else if (result.getPossibleParams().containsKey(variable)) {
paramsList.add(result.getPossibleParams().get(variable));
paramsList.add(new QueryScopeParam<>(variable, result.getPossibleParams().get(variable)));
}
}
params = paramsList.toArray(QueryScopeParam.ZERO_LENGTH_PARAM_ARRAY);
params = paramsList.toArray(QueryScopeParam[]::new);

checkAndInitializeVectorization(result, paramsList);
if (!initialized) {
Expand Down
6 changes: 3 additions & 3 deletions engine/table/src/main/java/io/deephaven/engine/table/impl/select/AbstractFormulaColumn.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public void validateSafeForRefresh(BaseTable<?> sourceTable) {
protected void applyUsedVariables(
@NotNull final Map<String, ColumnDefinition<?>> columnDefinitionMap,
@NotNull final Set<String> variablesUsed,
@NotNull final Map<String, QueryScopeParam<?>> possibleParams) {
@NotNull final Map<String, Object> possibleParams) {
// the column definition map passed in is being mutated by the caller, so we need to make a copy
columnDefinitions = Map.copyOf(columnDefinitionMap);

Expand All @@ -127,12 +127,12 @@ protected void applyUsedVariables(
if (variable.endsWith(COLUMN_SUFFIX) && columnDefinitions.get(strippedColumnName) != null) {
usedColumnArrays.add(strippedColumnName);
} else if (possibleParams.containsKey(variable)) {
paramsList.add(possibleParams.get(variable));
paramsList.add(new QueryScopeParam<>(variable, possibleParams.get(variable)));
}
}
}

params = paramsList.toArray(QueryScopeParam.ZERO_LENGTH_PARAM_ARRAY);
params = paramsList.toArray(QueryScopeParam[]::new);
}

protected void onCopy(final AbstractFormulaColumn copy) {
Expand Down
Loading

0 comments on commit 900310d

Please sign in to comment.