-
Notifications
You must be signed in to change notification settings - Fork 78
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
Check arg type against Py UDF signature at query compile time #5254
Check arg type against Py UDF signature at query compile time #5254
Conversation
@@ -1968,6 +1968,39 @@ static boolean isWideningPrimitiveConversion(Class<?> original, Class<?> target) | |||
return false; | |||
} | |||
|
|||
public static boolean isLosslessWideningPrimitiveConversion(Class<?> original, Class<?> target) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is interesting. Historically we've allowed widening from Long to Float. I see how this is lossy.
Is this used to disable vectorization? Or is it used to disable lossy widening?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 5.1.2 on https://docs.oracle.com/javase/specs/jls/se9/html/jls-5.html. The intention here is to ensure lossless conversion.
0b49a64
to
ccc392f
Compare
engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java
Show resolved
Hide resolved
1f9167e
to
b6d8cfc
Compare
engine/table/src/main/java/io/deephaven/engine/table/impl/lang/QueryLanguageParser.java
Outdated
Show resolved
Hide resolved
engine/table/src/main/java/io/deephaven/engine/util/PyCallableWrapperJpyImpl.java
Show resolved
Hide resolved
return false; | ||
} | ||
|
||
public static boolean isLosslessWideningPrimitiveConversion(Class<?> original, Class<?> target) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A unit test for this would be a good addition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This particular class is not being tested in the JUnit tests probably due to the fact that it needs a non-trivial amount of work to properly wire everything up to test it in Java.
- The Python unit tests indirectly test this method in a pretty comprehensive way. Maybe we can do a bit more from the Python side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This is a static method that takes
Class
parameters. It should be very easy to unit test in Java. - Unit tests like this should happen in Java when possible. That way things like coverage can be assessed.
- Additional Python tests should support the Java testing and not replace it.
- I suspect the lack of unit tests resulted from a lack of diligence at some point in the last 15 years. Now we are at a point where it looks like a lot of work to test everything. This is why I suggested testing a single static function.
engine/table/src/main/java/io/deephaven/engine/util/PyCallableWrapperJpyImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm approving but still think there should be a Java unit test as noted.
Labels indicate documentation is required. Issues for documentation have been opened: Community: deephaven/deephaven-docs-community#179 |
Fixes #5221