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

Reply via email to