-
Notifications
You must be signed in to change notification settings - Fork 79
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 wrapper for MultiJoin feature #4356
Conversation
py/server/deephaven/multijoin.py
Outdated
@@ -0,0 +1,126 @@ | |||
# |
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 question!
- Based on the precedents of
PartitionedTable
,TreeTable
,RollupTable
, it does seem to make sense to place MultiJoinTable in table.py - I am not sure the level of 'table-ness' MultiJoinTable is. My guess is that it is at the same level as the above mentioned ones. The only difference is that those are created by methods on Table, not unbound functions. In a way, it is more similar to
table_factory.merge()
. - One of the reasons we put
PartitionedTable
etc. in table.py is to avoid cyclical imports. MultiJoinTable doesn't seem to have this problem.
Although a bit torn, my personal preference is for consistency by having MultiJoinTable in table.py. Maybe we can move the function multi_join' to
table_factory`?
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.
Please move multi_join
to table.py
Co-authored-by: Jianfeng Mao <4297243+jmao-denver@users.noreply.github.com>
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
MultiJoinTable: the result of the multi-table natural join operation. To access the underlying Table, use the | ||
table() method. | ||
""" | ||
return MultiJoinTable(input, on) |
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.
General API question. I don't know the answer, but I'm asking it....
Is there any reason the user would need to know about MultiJoinInputTable
? Right now, the query syntax looks like:
t = multi_join([t1,t2,t3]).table()
Any reason it shouldn't just be
t = multi_join([t1,t2,t3])
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.
Yes, but only because in the future we intend to allow addition (and potentially removal) of tables from this join.
mt = multi_join([t1,t2,t3]);
mt_new = mt.add_table(t4) # returns Immutable MultiJoinTable
Labels indicate documentation is required. Issues for documentation have been opened: How-to: https://github.com/deephaven/deephaven.io/issues/3127 |
The call syntax is the following:
Closes #4280