westonpace commented on code in PR #13078: URL: https://github.com/apache/arrow/pull/13078#discussion_r871867122
########## cpp/src/arrow/engine/substrait/relation_internal.cc: ########## @@ -188,6 +188,79 @@ 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"); + default: + return Status::Invalid("Unsupported join type"); + } + + ARROW_ASSIGN_OR_RAISE(auto left, FromProto(join.left(), ext_set)); + ARROW_ASSIGN_OR_RAISE(auto right, FromProto(join.right(), ext_set)); + + if (!join.has_expression()) { + return Status::Invalid("substrait::JoinRel with no expression"); + } + + ARROW_ASSIGN_OR_RAISE(auto expression, FromProto(join.expression(), ext_set)); + + const auto& callptr = expression.call(); + if (!callptr) { + return Status::Invalid( + "Only support call expressions as the join key comparison."); + } + + compute::JoinKeyCmp join_key_cmp; + if (callptr->function_name == "equal") { + join_key_cmp = compute::JoinKeyCmp::EQ; + } else if (callptr->function_name == "is_not_distinct_from") { + join_key_cmp = compute::JoinKeyCmp::IS; + } else { + return Status::Invalid( + "Only Support `equal` or `is_not_distinct_from` for join key comparison"); + } + + // TODO: Add Suffix support for Substrait + compute::HashJoinNodeOptions join_options{ + join_type, + {std::move(*callptr->arguments[0].field_ref())}, + {std::move(*callptr->arguments[1].field_ref())}, + {join_key_cmp}, + arrow::compute::literal(true), + "_l", + "_r"}; Review Comment: Maybe. I think each options object should have one constructor which only takes the minimum set of options that are absolutely required and would have no sane default. -- 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: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org