-
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
Allow string duration/period parameters for GRPC time table creation #4085
Conversation
server/src/main/java/io/deephaven/server/table/ops/TimeTableGrpcImpl.java
Outdated
Show resolved
Hide resolved
proto/proto-backplane-grpc/src/main/proto/deephaven/proto/table.proto
Outdated
Show resolved
Hide resolved
This PR requires some changes to the Go client before it can merge. |
proto/proto-backplane-grpc/src/main/proto/deephaven/proto/table.proto
Outdated
Show resolved
Hide resolved
py/client/pydeephaven/_table_ops.py
Outdated
@@ -53,7 +53,7 @@ def __init__(self, table): | |||
|
|||
|
|||
class TimeTableOp(TableOp): | |||
def __init__(self, start_time: int = 0, period: int = 1000000000): | |||
def __init__(self, start_time: Union[int, str] = 0, period: Union[int, str] = 1000000000): |
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 see a few problems here:
- In the python API, we put period first in the args list, because it should never have a default. Here it is second AND has a default.
- The default zero start time argument is problematic, because 0 nanoseconds is a valid but unusual start time. The default argument should be some form of "now"
Suggestions:
- reverse the argument order.
- remove defaults for period
- pick a different null value for start time.
@jmao-denver should comment.
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 discussed this with @jmao-denver and this is the internal API. We supply None
as a default argument in the user-facing API in session.py
. This default is never used.
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.
If we never use the defaults and it is internal, at a minimum, I would remove the defaults at a minimum. If it is possible that this gets exposed in the future, I would also reverse the parameter order.
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.
In particular, the default period bothers me. The default value there is completely arbitrary and users will always specify a value they want.
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 have removed the internal defaults and now handle the None
value by setting start_time = 0
.
The order of the parameters matches the GRPC member structure, it can be changed but unsure the benefit it will bring.
Closes #4050