feat: add ExchangeRel as a type in Rel #518
Conversation
|
|
|
ACTION NEEDED Substrait follows the Conventional Commits The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. |
whutjs
force-pushed
the
feat/add_exchange_rel
branch
from
d50ec60 to
f0a4783
Compare
| @@ -369,6 +369,7 @@ message Rel { | |||
| //Physical relations | |||
| HashJoinRel hash_join = 13; | |||
| MergeJoinRel merge_join = 14; | |||
| ExchangeRel exchange = 15; | |||
There was a problem hiding this comment.
There are three write relations that should be in a section (they are not physical relations). These are ExchangeRel, DdlRel, and WriteRel. It should look something like:
// Write relations ExchangeRel exchange = 100; DdlRel ddl = 101; WriteRel write = 102;
There was a problem hiding this comment.
There are three write relations that should be in a section (they are not physical relations). These are ExchangeRel, DdlRel, and WriteRel. It should look something like:
// Write relations ExchangeRel exchange = 100; DdlRel ddl = 101; WriteRel write = 102;
@EpsilonPrime but in https://substrait.io/relations/physical_relations/#exchange-operator , the exchange is classified as a physical relation.
There was a problem hiding this comment.
It is indeed listed as a physical relation in the docs so I think this placement is fine. There are arguments to be made that "exchange" is a different sort of physical than "hash_join" (exchange is less about engine-specific optimization and more about distribution) but we'd need a lot more nuance than the docs currently have. Right now I think our working definition of "logical" is "is this something an end-user would put in a query?" and our definition of "physical" is "everything else".
This structure is largely meaningless anyways (as in, it's just a comment and grouping for readability, it makes no difference to the programs themselves since they see this whole list of options as one thing).
There was a problem hiding this comment.
Agreed, we can go with this PR as is.
|
Since this is a specification modification we will need two SMC votes. @jacques-n @cpcloud can one of you chime in here? It's a one line change so hopefully quick. |
The
ExchangeRelis already in the spec, but it is not a type ofRelyet.