-
Notifications
You must be signed in to change notification settings - Fork 80
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
Remove java execution logic during table.py module init #2872
Remove java execution logic during table.py module init #2872
Conversation
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.
LGTM
…ead which java takes care of
Related #2867 |
py/server/tests/test_filters.py
Outdated
@@ -11,10 +11,12 @@ | |||
|
|||
class FilterTestCase(BaseTestCase): | |||
def setUp(self): | |||
BaseTestCase.setUp(self) |
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.
super().setUp()?
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.
Will do.
py/server/deephaven/table.py
Outdated
if not j_query_scope: | ||
raise DHError("ExecutionContext does not have QueryScope") |
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.
check for PoisonedQueryScope?
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.
The following cast will throw an appropriate exception, essentially saying that a poisoned query scope can't be cast into the type we are wanting to.
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.
That said; this may have been me fumbling around trying to figure out why my DHError wasn't working. Looking at the code, I don't think any of the values in ExecutionContext can/should be null. I'll add a not null check to ExecutionContext, and I think we can get rid of this extraneous check.
py/server/tests/test_table.py
Outdated
|
||
t = empty_table(1).update("X = i").update("TableString = inner_func(X + 10)") | ||
with make_user_exec_ctx(): | ||
def inner_func(p) -> str: |
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 feel that it is more clear if the method is not defined within the open context. Only the update
calls need it.
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.
Good idea.
py/server/tests/test_table.py
Outdated
open_ctx.close() | ||
return t.to_string().split()[2] | ||
with make_user_exec_ctx(): | ||
def inner_func(p) -> str: |
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.
same on this inner function
LGTM, too (yah, I know it's already merged) |
Fixes #2853