westonpace commented on code in PR #13078:
URL: https://github.com/apache/arrow/pull/13078#discussion_r881409083


##########
cpp/src/arrow/engine/substrait/relation_internal.cc:
##########
@@ -225,6 +225,88 @@ 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:

Review Comment:
   ```suggestion
           case substrait::JoinRel::JOIN_TYPE_UNSPECIFIED:
   ```



##########
cpp/src/arrow/engine/substrait/relation_internal.cc:
##########
@@ -225,6 +225,88 @@ 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:
+          join_type = compute::JoinType::FULL_OUTER;
+          break;
+        case 3:
+          join_type = compute::JoinType::LEFT_OUTER;
+          break;
+        case 4:

Review Comment:
   ```suggestion
           case substrait::JoinRel::JOIN_TYPE_RIGHT:
   ```



##########
cpp/src/arrow/engine/substrait/relation_internal.cc:
##########
@@ -225,6 +225,88 @@ 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:
+          join_type = compute::JoinType::FULL_OUTER;
+          break;
+        case 3:

Review Comment:
   ```suggestion
           case substrait::JoinRel::JOIN_TYPE_LEFT:
   ```



##########
cpp/src/arrow/engine/substrait/relation_internal.cc:
##########
@@ -225,6 +225,88 @@ 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:

Review Comment:
   ```suggestion
           case substrait::JoinRel::JOIN_TYPE_OUTER:
   ```



##########
cpp/src/arrow/engine/substrait/relation_internal.cc:
##########
@@ -225,6 +225,88 @@ 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:
+          join_type = compute::JoinType::FULL_OUTER;
+          break;
+        case 3:
+          join_type = compute::JoinType::LEFT_OUTER;
+          break;
+        case 4:
+          join_type = compute::JoinType::RIGHT_OUTER;
+          break;
+        case 5:

Review Comment:
   ```suggestion
           case substrait::JoinRel::JOIN_TYPE_SEMI:
   ```



##########
cpp/src/arrow/engine/substrait/relation_internal.cc:
##########
@@ -225,6 +225,88 @@ 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:

Review Comment:
   ```suggestion
           case substrait::JoinRel::JOIN_TYPE_INNER:
   ```



##########
cpp/src/arrow/engine/substrait/relation_internal.cc:
##########
@@ -225,6 +225,88 @@ 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:
+          join_type = compute::JoinType::FULL_OUTER;
+          break;
+        case 3:
+          join_type = compute::JoinType::LEFT_OUTER;
+          break;
+        case 4:
+          join_type = compute::JoinType::RIGHT_OUTER;
+          break;
+        case 5:
+          join_type = compute::JoinType::LEFT_SEMI;
+          break;
+        case 6:

Review Comment:
   ```suggestion
           case substrait::JoinRel::JOIN_TYPE_ANTI:
   ```



##########
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");
+        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

Review Comment:
   Can you add the JIRA number in the comment?



-- 
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]

Reply via email to