westonpace commented on code in PR #13078:
URL: https://github.com/apache/arrow/pull/13078#discussion_r879808662
##########
cpp/src/arrow/engine/substrait/relation_internal.cc:
##########
@@ -225,6 +225,76 @@ Result<compute::Declaration> FromProto(const
substrait::Rel& rel,
});
}
+ case substrait::Rel::RelTypeCase::kJoin: {
+ const auto& join = rel.join();
+ RETURN_NOT_OK(CheckRelCommon(join));
+
+ if (!join.has_left()) {
+ return Status::Invalid("substrait::JoinRel with no left relation");
+ }
+
+ if (!join.has_right()) {
+ return Status::Invalid("substrait::JoinRel with no right relation");
+ }
+
+ compute::JoinType join_type;
+ switch (join.type()) {
+ case 0:
+ return Status::NotImplemented("Unspecified join type is not
supported");
+ case 1:
+ join_type = compute::JoinType::INNER;
+ break;
+ case 2:
+ return Status::NotImplemented("Outer join type is not supported");
+ case 3:
+ return Status::NotImplemented("Left join type is not supported");
+ case 4:
+ return Status::NotImplemented("Right join type is not supported");
+ case 5:
+ return Status::NotImplemented("Semi join type is not supported");
+ case 6:
+ return Status::NotImplemented("Anti join type is not supported");
Review Comment:
| Substrait | Arrow |
| --------- | ----- |
| JOIN_TYPE_INNER | INNER |
| JOIN_TYPE_OUTER | FULL_OUTER |
| JOIN_TYPE_LEFT | LEFT_OUTER |
| JOIN_TYPE_RIGHT | RIGHT_OUTER |
| JOIN_TYPE_SEMI | LEFT_SEMI |
| JOIN_TYPE_ANTI | LEFT_ANTI |
| JOIN_TYPE_SINGLE | - |
| - | RIGHT_SEMI |
| - | RIGHT_ANTI |
`JOIN_TYPE_SINGLE` is interesting and I'd almost interpret that as an
optional bool that could be added to any existing join. I'll create an issue
to seek clarification. Let's add support for the rest in this PR since it
should be straightforward enough.
Substrait doesn't have `RIGHT_SEMI` or `RIGHT_ANTI` and both of these can
easily be created by just swapping the inputs so I'm not sure it is critical
right now. Although it might help (for performance reasons) to keep the
probe/build side separate from the side we want to keep the output for so they
may come in handy in the future. I'm honestly not entirely sure. Either way,
let's not worry about those for this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
