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

[Python] Integers greater than 32 bits don't resolve properly in the query engine #1461

Closed
jakemulf opened this issue Oct 19, 2021 · 5 comments
Assignees
Labels
bug Something isn't working jpy query engine
Milestone

Comments

@jakemulf
Copy link
Contributor

jakemulf commented Oct 19, 2021

Description

The following python code is supposed to make a table with 2 columns, one containing a date-time and the other containing the date-time floored via the lowerBin() method, and print the equivalent result of lowerBin() to console.

from deephaven.DBTimeUtils import expressionToNanos, convertDateTime, lowerBin
from deephaven.TableTools import newTable, dateTimeCol

nanosBin = expressionToNanos("T15M")
dateTime = convertDateTime("2020-01-01T00:35:00 NY")

source = newTable(
    dateTimeCol("DateTimes", dateTime)
)

result = source.update("DateTimesBinned = lowerBin(DateTimes, nanosBin)")
print(lowerBin(dateTime, nanosBin))

However, the print statement and the table result in 2 different results

2020-01-01T00:30:00.000000000 NY vs 2020-01-01T00:34:59.016

Screen Shot 2021-10-18 at 10 57 06 PM

Steps to reproduce

Described above

Expected results

The output of the print statement for lowerBin() should match the column computed by lowerBin().

Actual results

The output of the print statement for lowerBin() appears correct, while the column computed by lowerBin() is not correct.

Additional details and attachments

This does not happen in groovy.

This may be the same issue as #1348. If that's the case, feel free to close this to avoid duplicated issues.

Versions

  • Deephaven: latest
  • OS: OSX
  • Browser: Chrome
  • Docker: Docker version 20.10.8, build 3967b7d
@jakemulf jakemulf added bug Something isn't working triage labels Oct 19, 2021
@jakemulf
Copy link
Contributor Author

jakemulf commented Oct 19, 2021

Casting nanosBin to a long does not work either

from deephaven.DBTimeUtils import expressionToNanos, convertDateTime
from deephaven.TableTools import newTable, dateTimeCol

nanosBin = expressionToNanos("T15M")
dateTime = convertDateTime("2020-01-01T00:35:00 NY")

source = newTable(
    dateTimeCol("DateTimes", dateTime)
)

result = source.update("DateTimesBinned = lowerBin(DateTimes, (long)nanosBin)")

I'm guessing it's an int vs long issue, since expressionToNanos("T2S") (which can be stored in a 32 bit number) bins the time correctly

@rcaudy rcaudy added this to the Jan 2022 milestone Jan 18, 2022
@jakemulf jakemulf changed the title [Python] lowerBin() computes incorrect result when used in a query [Python] Integers greater than 32 bits don't resolve properly in the query engine Jan 18, 2022
@rcaudy
Copy link
Member

rcaudy commented Jan 18, 2022

from deephaven.DateTimeUtils import expressionToNanos, convertDateTime
from deephaven.TableTools import newTable, dateTimeCol

nanosBin = expressionToNanos("T15M")
dateTime = convertDateTime("2020-01-01T00:35:01 NY")

source = newTable(dateTimeCol("DateTimes", dateTime))
result1 = source.update("DateTimesBinned = upperBin(DateTimes, nanosBin)")
result2 = source.update("DateTimesBinned = upperBin(DateTimes, 900000000000)")

result2 is fine, result1 is bad.

@jakemulf
Copy link
Contributor Author

jakemulf commented Jan 18, 2022

Update

This issue is not related to the lowerBin method. It appears that the query scope doesn't resolve variables that contain python integers greater than 32 bits (take note that python doesn't have a concept of a "long" anymore)

from deephaven.TableTools import emptyTable

result = emptyTable(1)

my_int = 2**32 + 5

result = result.update("X = my_int")

This code results in the same issue, with X == 5 in the resulting table.

Raw numbers in the update query (using Ryan's example above) work properly

@devinrsmith
Copy link
Member

This may be a result of PyObject.getValue(), which ultimately runs this sort of logic:

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);
}

The scale of the python int isn't being checked before converting to an java Integer.

@devinrsmith
Copy link
Member

We can work around this (without needing to modify jpy) by using more type-appropriate return values, either by checking PyObject.getType(), or PyObject.isLong() (isLong and isInt are the same in python 3+), and then calling PyObject.getLongValue() (instead of PyObject.getValue()). Of course, looking at this code, I think it has issues if there are more than 64 bits in the python int.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working jpy query engine
Projects
None yet
Development

No branches or pull requests

4 participants