feat: Support Multijoin in the Python client #6020
Conversation
jmao-denver
added
DocumentationNeeded
python-client
ReleaseNotesNeeded
py/client/pydeephaven/session.py
Outdated
| table: Table | ||
| on: Union[str, Sequence[str]] | ||
| joins: Union[str, Sequence[str]] = None |
There was a problem hiding this comment.
Not present on the server MultiJoinInput. Should they be made private? Should they be added to the server side?
There was a problem hiding this comment.
Server side is a wrapper around a Java MJI. I'm ambivalent about whether these are user-facing, but they should not be mutable.
There was a problem hiding this comment.
This is still an incompatibility in the API.
py/client/pydeephaven/session.py
Outdated
| @@ -734,3 +737,60 @@ def plugin_client(self, exportable_obj: ticket_pb2.TypedTicket) -> PluginClient: | |||
|
|
|||
| Part of the experimental plugin API.""" | |||
| return PluginClient(self, exportable_obj) | |||
|
|
|||
| def multi_join(self, input: Union[Table, Sequence[Table], MultiJoinInput, Sequence[MultiJoinInput]], | |||
There was a problem hiding this comment.
Users have very very strongly wanted the server and client APIs to be the same. This has a number of deviations compared to the server that I can see at a glance.
multi_joinis a method on session instead of just being a function.multi_joinis in a different submodule (sessioninstead oftable).- The return type is
Tableinstead ofMultiJoinTable.
How similar can the APIs be?
There was a problem hiding this comment.
Other clients have taken the approach of returning Table for now, instead of MultiJoinTable. MJT is more of a future-looking API, and given that its potential (adding more joins to an existing MJ and thus getting out a new Table) hasn't been realized yet there's no real benefit to building an object type plugin for MJT.
There was a problem hiding this comment.
I have moved the new method to table.py as a function and have also relocated the MultiJoinInput class there as well.
@chipkent I totally understand the desire to have the interface be the same for the two APIs, but the client isn't limited to working with just one server and making a 'session' level function a method on the Session object has the benefit of clear isolation/readability etc. So this change has a trade-off which I am quite ambivalent about.
| def multi_join(input: Union[Table, Sequence[Table], MultiJoinInput, Sequence[MultiJoinInput]], | ||
| on: Union[str, Sequence[str]] = None) -> Table: |
There was a problem hiding this comment.
The return type here is Table where the deephaven method returns MultiJoinTable. MultiJoinTable does not inherit from Table and has a different API. Users of MultiJoinTable must call table() to get a table to use. I think I complained about this during that review. Without adding a MultiJoinTable on the client side, the APIs are incompatible.
There was a problem hiding this comment.
See #6020 (comment)
py/client/pydeephaven/session.py
Outdated
| table: Table | ||
| on: Union[str, Sequence[str]] | ||
| joins: Union[str, Sequence[str]] = None |
There was a problem hiding this comment.
This is still an incompatibility in the API.
Fixes #5884