GRPC support for MultiJoin, improved support for RangeJoin #5153
Conversation
lbooker42
requested review from
devinrsmith,
nbauernfeind,
niloc132,
rcaudy,
chipkent and
jmao-denver
as code owners
lbooker42
added
NoDocumentationNeeded
NoReleaseNotesNeeded
proto/proto-backplane-grpc/src/main/proto/deephaven/proto/table.proto
Outdated
Show resolved
Hide resolved
|
|
||
| message MultiJoinTablesRequest { | ||
| Ticket result_id = 1; | ||
| // It is considered an error if both `source_ids` and `multi_join_inputs` are provided. |
There was a problem hiding this comment.
As a comment, this will appear on the source_ids field in each language, rather than be general for the body of the message. Consider moving to just before the message itself, or duplicate it to the multi_join_inputs as well?
There was a problem hiding this comment.
...that said, this might be better expressed as a oneof? Reading the logic in the java server impl, if multi_join_inputs is provided, neither source_ids nor columns_to_match can be specified (i.e. "will be empty")?
I'm not above just making the runtime logic encode this instead of the message format, but there is something to be said for making the serialization api simply not support invalid states.
There was a problem hiding this comment.
as @devinrsmith pointed out, it's really (source_ids + columns_to_match) ^ multi_join_inputs that needs to be enforced. oneof doesn't seem to work here without adding another nesting layer which I think would break backward compatibility.
There was a problem hiding this comment.
What do you mean "break backward compatibility" - this is ostensibly a brand new message, yeah?
There was a problem hiding this comment.
Good point. RangeJoin is published, MultiJoin is new.
proto/proto-backplane-grpc/src/main/proto/deephaven/proto/table.proto
Outdated
Show resolved
Hide resolved
proto/proto-backplane-grpc/src/main/proto/deephaven/proto/table.proto
Outdated
Show resolved
Hide resolved
proto/proto-backplane-grpc/src/main/proto/deephaven/proto/table.proto
Outdated
Show resolved
Hide resolved
server/src/main/java/io/deephaven/server/table/ops/MultiJoinGrpcImpl.java
Show resolved
Hide resolved
server/src/main/java/io/deephaven/server/table/ops/RangeJoinGrpcImpl.java
Outdated
Show resolved
Hide resolved
server/src/main/java/io/deephaven/server/table/ops/RangeJoinGrpcImpl.java
Outdated
Show resolved
Hide resolved
server/src/main/java/io/deephaven/server/table/ops/RangeJoinGrpcImpl.java
Outdated
Show resolved
Hide resolved
| // Specifies the range match parameters as a parseable string. Providing `range_match` in the GRPC call is the | ||
| // alternative to detailed range match parameters provided in the `range_start_column`, `range_start_rule`, | ||
| // `right_range_column`, `range_end_rule`, and `left_end_column` fields. | ||
| string range_match = 11; |
There was a problem hiding this comment.
Do we want to expose this to io.deephaven.qst.table.RangeJoinTable? (The answer may be no.)
There was a problem hiding this comment.
I don't think so, this adds a shortcut / additional way to populate the RangeJoinMatch but doesn't replace it.
| repeated string columns_to_add = 4; | ||
| } | ||
|
|
||
| message MultiJoinTablesRequest { |
There was a problem hiding this comment.
Is now the time to plumb this through io.deephaven.qst / io.deephaven.api.TableOperations? If not, please create a ticket.
server/src/main/java/io/deephaven/server/table/ops/MultiJoinGrpcImpl.java
Outdated
Show resolved
Hide resolved
This PR adds GRPC support for
MultiJoin, allowing clients to efficiently join multiple tables. It also improves support forRangeJoinby exposing astringinput for specifying the match criteria.Note that this implementation directly returns a
Tableas opposed to the server-side calls which returnMultiJoinTableobjects. The more complex interface can be exposed to GRPC along-side this implementation when we add some additional desired functionality toMultiJoinTable.Closes #4281