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

A workaround to a JPY issue with long values, fixes #1461 #1843

Merged
merged 5 commits into from
Jan 20, 2022

Conversation

jmao-denver
Copy link
Contributor

In JPY, PyObject.getObjectValue() seems always to select Java Integer
for a Python int object which is not restricted by the number
of bits in Python 3

In JPY, PyObject.getObjectValue() seems always to select Java Integer
for a Python int object (3.00+) which is not restricted by the number
of bits
@jmao-denver
Copy link
Contributor Author

jmao-denver commented Jan 18, 2022

DHE suffers from the same problem, added @cpwright as a reviewer to loop him in.

@cpwright
Copy link
Contributor

Should we simply look further up the chain at JPy and fix that. I expect the problem relates to these lines in
JType_ConvertJavaToPythonObject.

} else if (JPy_IS_CLONG(pyArg) && (type == JPy_JObject || ((*jenv)->IsAssignableFrom(jenv, JPy_Integer_JClass, type->classRef)))) {
    return JType_CreateJavaIntegerObject(jenv, type, pyArg, objectRef);
} else if (JPy_IS_CLONG(pyArg) && (type == JPy_JObject || ((*jenv)->IsAssignableFrom(jenv, JPy_Long_JClass, type->classRef)))) {
    return JType_CreateJavaLongObject(jenv, type, pyArg, objectRef);

@devinrsmith
Copy link
Member

Should we simply look further up the chain at JPy and fix that. I expect the problem relates to these lines in JType_ConvertJavaToPythonObject.

} else if (JPy_IS_CLONG(pyArg) && (type == JPy_JObject || ((*jenv)->IsAssignableFrom(jenv, JPy_Integer_JClass, type->classRef)))) {
    return JType_CreateJavaIntegerObject(jenv, type, pyArg, objectRef);
} else if (JPy_IS_CLONG(pyArg) && (type == JPy_JObject || ((*jenv)->IsAssignableFrom(jenv, JPy_Long_JClass, type->classRef)))) {
    return JType_CreateJavaLongObject(jenv, type, pyArg, objectRef);

I made this same comment here #1461 (comment). Ultimately, I think the implicit type conversion layer of python objects to to java objects needs to be examined, and we may want to provide the (java) caller with better hooks on how to handle the different cases.

These types of problems may also be better solved by pushing application logic further down into python instead of trying to be as general as we can w/ PythonScope. I've made some similar changes for FetchObject (landing soon) where script session changes are handled with .py logic instead of numerous JNI boundary crossings.

@jmao-denver
Copy link
Contributor Author

@devinrsmith @cpwright

I think you meant JType_ConvertPythonToJavaObject (not JType_ConvertJavaToPythonObject).

The straightforward changes I could see are:

  1. to switch the order of the two blocks (or simply remove the one check assignability of Integer, which amounts to the alternative Devin mentioned about, and with that, potentially double the memory used for a column unnecessarily

  2. to examine the actual value and select the appropriate Java type, which is almost the same (hacky) as the solution in this PR but with one fewer JNI/Python boundary crossing

I am not strongly inclined towards any one of the 3 solutions, but this PR is still my favorite simply because it is higher level and less likely to break other things (?)

devinrsmith
devinrsmith previously approved these changes Jan 19, 2022
Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

I'm OK with this approach. Based on @rcaudy feedback thought, he may prefer the pyObject.isLong(), pyObject.getLongValue(), and seeing if it fits into int. It's an extra case, but it fits the pattern established here.

@jmao-denver jmao-denver merged commit c5214cf into deephaven:main Jan 20, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jan 20, 2022
@jmao-denver jmao-denver deleted the bug-1461 branch February 8, 2023 18:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants