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.

Sign up for GitHub

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

Replace some wrapper funcs in the deephaven.time module with Java wrapped method calls when used in formulas #4095

Closed
wants to merge 2 commits into from
Closed

Replace some wrapper funcs in the deephaven.time module with Java wrapped method calls when used in formulas #4095

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
776f097
Enable Py func replacement for now, lower/upper_bin
jmao-denver Jun 28, 2023
f411a0f
Add Py constants to reserved keywords
jmao-denver Jul 13, 2023
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
1 change: 1 addition & 0 deletions Integrations/src/main/java/io/deephaven/integrations/python/PythonBiFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/
package io.deephaven.integrations.python;

import io.deephaven.engine.util.PythonObjectWrapper;
import io.deephaven.util.annotations.ScriptApi;
import org.jpy.PyObject;

Expand Down
1 change: 1 addition & 0 deletions Integrations/src/main/java/io/deephaven/integrations/python/PythonFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/
package io.deephaven.integrations.python;

import io.deephaven.engine.util.PythonObjectWrapper;
import io.deephaven.util.annotations.ScriptApi;
import org.jpy.PyObject;

Expand Down
1 change: 1 addition & 0 deletions Integrations/src/main/java/io/deephaven/integrations/python/PythonValueGetter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package io.deephaven.integrations.python;

import io.deephaven.engine.util.PythonObjectWrapper;
import io.deephaven.util.QueryConstants;
import org.jpy.PyObject;

Expand Down
90 changes: 89 additions & 1 deletion engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,12 @@
import io.deephaven.engine.context.QueryScope;
import io.deephaven.engine.table.impl.MatchPair;
import io.deephaven.engine.table.impl.ShiftedColumnsFactory;
import io.deephaven.engine.util.AbstractScriptSession;
import io.deephaven.engine.util.PyCallableWrapper.ColumnChunkArgument;
import io.deephaven.engine.util.PyCallableWrapper.ConstantChunkArgument;
import io.deephaven.engine.util.PyCallableWrapper;
import io.deephaven.engine.util.PythonDeephavenSession;
import io.deephaven.engine.util.PythonScopeJpyImpl;
import io.deephaven.internal.log.LoggerFactory;
import io.deephaven.io.logger.Logger;
import io.deephaven.util.type.TypeUtils;
Expand All @@ -109,6 +112,8 @@
import io.deephaven.vector.Vector;
import org.apache.commons.lang3.ClassUtils;
import org.apache.commons.lang3.exception.ExceptionUtils;
import org.jpy.PyInputMode;
import org.jpy.PyListWrapper;
import org.jpy.PyObject;

import java.lang.reflect.Array;
Expand Down Expand Up @@ -465,6 +470,66 @@ private Class<?> findNestedClass(Class<?> enclosingClass, String nestedClassName
return m.get(nestedClassName);
}

private boolean pyToJavaReplaced(final Class<?> scope, final MethodCallExpr n) {
Copy link
Member

Choose a reason for hiding this comment

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

This should have a JavaDoc, and clarify that it is mutating n.

final String methodName = n.getNameAsString();
final QueryScope queryScope = ExecutionContext.getContext().getQueryScope();

if (scope == null) {
final Class<?> methodClass = variables.get(methodName);
if (methodClass == PyCallableWrapper.class) {
final Object paramValueRaw = queryScope.readParamValue(methodName, null);
Copy link
Member

Choose a reason for hiding this comment

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

You can't do this 100% correctly from the information you have available. That is, you don't know that the name is coming from the query scope, since column names take precedence. Your argument seems to hinge on "how could there be a column of PyCallableWrappers?" Some user will find a way...

Maybe we should add more typing information from the caller, to tell us what's a column ref, etc?

final PyCallableWrapper pyCallableWrapper = (PyCallableWrapper) paramValueRaw;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a null check, just in case users are mutating the scope concurrently?

try {
String javaMethodName = pyCallableWrapper.getAttribute("_j_simple_name", String.class);
n.setName(javaMethodName);
int nargs = pyCallableWrapper.getAttribute("_nargs", int.class);
PyListWrapper defArgList = (PyListWrapper) pyCallableWrapper.getAttribute("_def_args").asList();
int defArgsToAdd = nargs - n.getArguments().size();
if (defArgsToAdd > defArgList.size()) {
throw new IllegalArgumentException("Missing args for " + methodName);
Copy link
Member

Choose a reason for hiding this comment

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

This might need more detail. Maybe try to give a string for the method call itself, with the args?

}
if (defArgsToAdd < 0) {
// do nothing, could be valid if the java method has a vararg
}
// Since the func call follows the Java syntax, we don't do name matching for the keyword args
Copy link
Member

Choose a reason for hiding this comment

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

I think a more detailed comment would be helpful (to people me like me):

  1. Right now, users cannot specify keyword arguments when calling Python functions in the query language
  2. So, it's safe to expect that we only need to supply a trailing suffix of defaulted arguments.

for (int i = defArgList.size() - (nargs - n.getArguments().size()); i < defArgList.size(); i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (int i = defArgList.size() - (nargs - n.getArguments().size()); i < defArgList.size(); i++) {
for (int i = defArgList.size() - defArgsToAdd; i < defArgList.size(); i++) {

Object v = PythonScopeJpyImpl.convert(defArgList.get(i));
n.addArgument(v.getClass() == String.class ? "\"" + v.toString() + "\"" : v.toString());
Copy link
Member

Choose a reason for hiding this comment

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

Any chance you unwrap None to null and then NPE on the v.toString()?

}
return true;
} catch (IllegalArgumentException iae) {
throw iae;
} catch (RuntimeException e) {
// Not a Java replaceable callable, can safely ignore
Comment on lines +502 to +503
Copy link
Member

Choose a reason for hiding this comment

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

Can we use hasAttribute to avoid using failed getAttribute calls for control flow?

}
}
} else if (scope == org.jpy.PyObject.class || scope == PyCallableWrapper.class) {
String pythonExpr = n.getScope().get().toString() + "." + methodName;
PythonDeephavenSession pds =
(PythonDeephavenSession) ((AbstractScriptSession.UnsynchronizedScriptSessionQueryScope) queryScope)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(PythonDeephavenSession) ((AbstractScriptSession.UnsynchronizedScriptSessionQueryScope) queryScope)
(PythonDeephavenSession) ((AbstractScriptSession.ScriptSessionQueryScope) queryScope)

.scriptSession();
Map<String, PyObject> scopeVars = pds.getVariablesRaw();
PyObject pyobj;
try {
pyobj = PyObject.executeCode(pythonExpr, PyInputMode.EXPRESSION, scopeVars, null);
} catch (RuntimeException e) {
throw new RuntimeException("Cannot find Python callable: " + pythonExpr);
Copy link
Member

Choose a reason for hiding this comment

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

Throw something else. UncheckedDeephavenException or a subclass.

}
try {
Object obj = PythonScopeJpyImpl.convert(pyobj);
if (obj.getClass() == (PyCallableWrapper.class)) {
String javaMethodName = ((PyCallableWrapper) obj).getAttribute("_j_simple_name", String.class);
n.setScope(null);
n.setName(javaMethodName);
return true;
Comment on lines +520 to +524
Copy link
Member

Choose a reason for hiding this comment

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

Don't we want the same default handling? That's missing...

}
} catch (Exception e) {
// Not a Java replaceable callable, can safely ignore
}
}
return false;
}

private Method getMethod(final Class<?> scope, final String methodName, final Class<?>[] paramTypes,
final Class<?>[][] parameterizedTypes) {
final ArrayList<CandidateExecutable<Method>> acceptableMethods = new ArrayList<>();
Expand Down Expand Up @@ -1140,6 +1205,16 @@ public Class<?> visit(NameExpr n, VisitArgs printer) {
* throw them to 'findClass()'. Many details are not relevant here. For example, field access is handled by a
* different method: visit(FieldAccessExpr, StringBuilder).
*/
Map<String, String> pyConstantsMap = Map.of(
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be very late in the method, so that we don't try to provide a type for an absent variable in a non-python language.

Copy link
Member

Choose a reason for hiding this comment

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

Should be conditional on the expression coming from Python, I think.

"True", "true",
"False", "false",
"None", "null");
final String name = n.getNameAsString();
String jConstant = pyConstantsMap.get(name);
if (jConstant != null) {
printer.append(jConstant);
return name.equals("None") ? NULL_CLASS : boolean.class;
Copy link
Member

Choose a reason for hiding this comment

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

This is terribly hacky. Why not keep pairs in the map's values, so the type is associated with the constant?

}
printer.append(n.getNameAsString());

Class<?> ret = variables.get(n.getNameAsString());
Expand Down Expand Up @@ -1865,6 +1940,13 @@ public Class<?> visit(MethodCallExpr n, VisitArgs printer) {
return result;
}).orElse(null);

if (pyToJavaReplaced(scope, n)) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename method, maybeReplace?

if (scope != null) {
scope = null;
innerPrinter.reset();
}
}
Comment on lines +1943 to +1948
Copy link
Member

Choose a reason for hiding this comment

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

So, it looks to me like this means we can only replace things that are statically imported. Should change the scope in n and re-order this to be before the block above? That way, we wouldn't be setting null scope, or resetting our printer, and we might be able to handle more classes of expression.


Expression[] expressions = n.getArguments() == null ? new Expression[0]
: n.getArguments().toArray(new Expression[0]);

Expand All @@ -1873,7 +1955,6 @@ public Class<?> visit(MethodCallExpr n, VisitArgs printer) {
Class<?>[][] parameterizedTypes = getParameterizedTypes(expressions);

Method method = getMethod(scope, n.getNameAsString(), expressionTypes, parameterizedTypes);

Class<?>[] argumentTypes = method.getParameterTypes();

// now do some parameter conversions...
Expand Down Expand Up @@ -1913,6 +1994,7 @@ public Class<?> visit(MethodCallExpr n, VisitArgs printer) {
}
} else { // Groovy or Java method call
printer.append(innerPrinter);
n.setName(method.getName());
Copy link
Member

Choose a reason for hiding this comment

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

Why aren't we doing all of our n mutation in one place?

printer.append(n.getNameAsString());
}

Expand Down Expand Up @@ -2538,6 +2620,12 @@ public boolean hasStringBuilder() {
return builder != null;
}

public void reset() {
Copy link
Member

Choose a reason for hiding this comment

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

Hopefully my proposed change enables us to delete this.

if (hasStringBuilder()) {
builder.setLength(0);
}
}

/**
* Convenience method: forwards argument to 'builder' if 'builder' is not null
*/
Expand Down
20 changes: 12 additions & 8 deletions ...ations/python/PythonDeephavenSession.java → ...n/engine/util/PythonDeephavenSession.java
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

We should find a way to not move this. How about we have an interface in engine.util PythonMethodExpressionResolver or something more appropriate, that just takes a pair of strings?

public interface PythonMethodExpressionResolver {
     PyObject resolve(@NotNull final String scope, @NotNull final String name);
}

Then, just have the PythonDeephavenScriptSession implement that interface.

Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* Copyright (c) 2016-2022 Deephaven Data Labs and Patent Pending
*/
package io.deephaven.integrations.python;
package io.deephaven.engine.util;

import io.deephaven.base.FileUtils;
import io.deephaven.base.verify.Assert;
Expand All @@ -10,13 +10,7 @@
import io.deephaven.engine.exceptions.CancellationException;
import io.deephaven.engine.context.QueryScope;
import io.deephaven.engine.updategraph.UpdateGraph;
import io.deephaven.engine.util.AbstractScriptSession;
import io.deephaven.engine.util.PythonEvaluator;
import io.deephaven.engine.util.PythonEvaluatorJpy;
import io.deephaven.engine.util.PythonScope;
import io.deephaven.engine.util.ScriptFinder;
import io.deephaven.engine.util.ScriptSession;
import io.deephaven.integrations.python.PythonDeephavenSession.PythonSnapshot;
import io.deephaven.engine.util.PythonDeephavenSession.PythonSnapshot;
import io.deephaven.engine.util.scripts.ScriptPathLoader;
import io.deephaven.engine.util.scripts.ScriptPathLoaderState;
import io.deephaven.internal.log.LoggerFactory;
Expand All @@ -40,14 +34,18 @@
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.AbstractMap;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.function.Function;
import java.util.function.Supplier;
import java.util.stream.Collectors;

import static java.util.stream.Collectors.toMap;

/**
* A ScriptSession that uses a JPy cpython interpreter internally.
* <p>
Expand Down Expand Up @@ -207,6 +205,12 @@ public Map<String, Object> getVariables() {
return outMap;
}

public Map<String, PyObject> getVariablesRaw() {
return scope.getEntriesRaw()
.map(e -> new AbstractMap.SimpleImmutableEntry<>(scope.convertStringKey(e.getKey()), e.getValue()))
.collect(toMap(Map.Entry::getKey, Map.Entry::getValue));
}

protected static class PythonSnapshot implements Snapshot, SafeCloseable {

private final PyDictWrapper dict;
Expand Down
2 changes: 1 addition & 1 deletion ...egrations/python/PythonObjectWrapper.java → ...aven/engine/util/PythonObjectWrapper.java
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

This class didn't need to move.

Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* Copyright (c) 2016-2022 Deephaven Data Labs and Patent Pending
*/
package io.deephaven.integrations.python;
package io.deephaven.engine.util;

import org.jpy.PyModule;
import org.jpy.PyObject;
Expand Down
Loading