ARROW-17520: [C++] Implement SubStrait SetRel (UnionAll) #14186

vibhatha · 2022-09-21T03:26:16Z

This PR includes the initial version of union operator support for Substrait->Acero.

cpp/src/arrow/engine/substrait/relation_internal.cc

vibhatha · 2022-09-21T03:27:50Z

cc @westonpace @jvanstraten

github-actions · 2022-09-21T04:01:15Z

https://issues.apache.org/jira/browse/ARROW-17520

github-actions · 2022-09-21T04:01:17Z

⚠️ Ticket has no components in JIRA, make sure you assign one.

westonpace

A few minor suggestions but this seems pretty solid to me

westonpace · 2022-09-23T18:54:33Z

cpp/src/arrow/engine/substrait/relation_internal.cc

+      // Note: at the moment Acero only supports UNION_ALL operation
+      switch (op) {
+        case substrait::SetRel::SET_OP_UNSPECIFIED:
+          return Status::NotImplemented("NotImplemented union type");


Maybe you can use EnumToString to have a better error message.

To use this, I did a minor refactor. I will point it out below.

cpp/src/arrow/engine/substrait/relation_internal.cc

cpp/src/arrow/engine/substrait/serde_test.cc

vibhatha · 2022-09-24T02:57:33Z

cpp/src/arrow/engine/substrait/expression_internal.cc

-std::string EnumToString(int value, const google::protobuf::EnumDescriptor* descriptor) {
-  const google::protobuf::EnumValueDescriptor* value_desc =
-      descriptor->FindValueByNumber(value);
-  if (value_desc == nullptr) {
-    return "unknown";
-  }
-  return value_desc->name();
-}
-


I removed this method from expression_internal.cc since this can be a generic util. And put it to the util.cc

cpp/src/arrow/engine/substrait/relation_internal.cc

vibhatha · 2022-09-24T02:58:16Z

cpp/src/arrow/engine/substrait/serde_test.cc

@@ -179,7 +179,8 @@ void CheckRoundTripResult(const std::shared_ptr<Schema> output_schema,
                          compute::ExecContext& exec_context,
                          std::shared_ptr<Buffer>& buf,
                          const std::vector<int>& include_columns = {},
-                          const ConversionOptions& conversion_options = {}) {
+                          const ConversionOptions& conversion_options = {},
+                          const compute::SortOptions* sort_options = NULLPTR) {


the logic here is the same as the glob PR.

vibhatha · 2022-09-24T02:58:44Z

cpp/src/arrow/engine/substrait/util.cc

+std::string EnumToString(int value, const google::protobuf::EnumDescriptor* descriptor) {
+  const google::protobuf::EnumValueDescriptor* value_desc =
+      descriptor->FindValueByNumber(value);
+  if (value_desc == nullptr) {
+    return "unknown";
+  }
+  return value_desc->name();
+}
+


I moved the EnumToString function to util.h

vibhatha · 2022-09-24T02:59:47Z

cpp/src/arrow/engine/substrait/util.h

@@ -25,6 +25,8 @@
 #include "arrow/engine/substrait/options.h"
 #include "arrow/util/iterator.h"

+#include "substrait/algebra.pb.h"  // IWYU pragma: export


This part is not very sure. We could include proto headers which is required for this, but what is the best practice here? Since there would be more utils like this which could reference the interfaces in the algebra.pb.h, we could use it. But as per this PR what is the best?

Unfortunately, we can't do this. I made the same mistake playing around with extension rels. However, this is part of the public API (these methods are used by python).

I agree it would be useful to have this method in a common spot. Can you make a util_internal.h for this?

@westonpace I added and internal file, could you please check?

vibhatha · 2022-09-24T03:00:18Z

@westonpace thanks for the review, and I updated it with a few changes out of scope. Please check it.

westonpace

The include in util.h will have to change. But I think we can solve it with util_internal.h

cpp/src/arrow/engine/substrait/relation_internal.cc

westonpace · 2022-11-15T06:04:18Z

cpp/src/arrow/engine/substrait/util.h

@@ -25,6 +25,8 @@
 #include "arrow/engine/substrait/options.h"
 #include "arrow/util/iterator.h"

+#include "substrait/algebra.pb.h"  // IWYU pragma: export


Unfortunately, we can't do this. I made the same mistake playing around with extension rels. However, this is part of the public API (these methods are used by python).

I agree it would be useful to have this method in a common spot. Can you make a util_internal.h for this?

westonpace

Looks good. A few nits and one slight change needed to how we handle the new internal header.

cpp/src/arrow/engine/substrait/util_internal.h

cpp/src/arrow/engine/substrait/util.h

vibhatha · 2022-12-12T07:13:24Z

@westonpace I updated the PR.

westonpace

Looks good to me now. Thanks for your persistence.

vibhatha · 2022-12-14T09:54:35Z

Looks good to me now. Thanks for your persistence.

Thank you for keeping up with the modifications. 🙂

ursabot · 2022-12-15T03:33:28Z

Benchmark runs are scheduled for baseline = f668537 and contender = 4647739. 4647739 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.66% ⬆️0.07%] test-mac-arm
[Finished ⬇️0.27% ⬆️0.27%] ursa-i9-9960x
[Finished ⬇️0.93% ⬆️0.07%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 4647739e ec2-t3-xlarge-us-east-2
[Finished] 4647739e test-mac-arm
[Finished] 4647739e ursa-i9-9960x
[Finished] 4647739e ursa-thinkcentre-m75q
[Finished] f6685371 ec2-t3-xlarge-us-east-2
[Finished] f6685371 test-mac-arm
[Finished] f6685371 ursa-i9-9960x
[Finished] f6685371 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

vibhatha commented Sep 21, 2022

View reviewed changes

cpp/src/arrow/engine/substrait/relation_internal.cc Show resolved Hide resolved

github-actions bot added the Component: C++ label Sep 21, 2022

westonpace requested changes Sep 23, 2022

View reviewed changes

vibhatha commented Sep 24, 2022

View reviewed changes

cpp/src/arrow/engine/substrait/relation_internal.cc Outdated Show resolved Hide resolved

vibhatha commented Sep 24, 2022

View reviewed changes

vibhatha requested a review from westonpace September 24, 2022 02:59

vibhatha force-pushed the arrow-17520 branch from 4e02ca6 to 6c9de91 Compare November 2, 2022 12:22

westonpace requested changes Nov 15, 2022

View reviewed changes

vibhatha requested a review from westonpace November 17, 2022 05:10

vibhatha force-pushed the arrow-17520 branch from f055b7b to d85b388 Compare November 17, 2022 08:32

westonpace requested changes Dec 11, 2022

View reviewed changes

cpp/src/arrow/engine/substrait/util_internal.h Outdated Show resolved Hide resolved

cpp/src/arrow/engine/substrait/util_internal.h Outdated Show resolved Hide resolved

cpp/src/arrow/engine/substrait/util.h Outdated Show resolved Hide resolved

vibhatha added 9 commits December 12, 2022 11:58

feat(initial)

2f5b068

feat(test): union basic

8c99978

fix(reviews): added utils and updated tests

603c9cd

fix(minor): chaining similar cases

6257561

fix(rebase)

82caef3

fix(reviews

dd9ed0c

fix(import)

4012401

fix(test-import): appveyor issue

a2bb3a1

fix(reviews)

2541256

vibhatha force-pushed the arrow-17520 branch from d85b388 to 2541256 Compare December 12, 2022 07:12

westonpace approved these changes Dec 14, 2022

View reviewed changes

westonpace merged commit 4647739 into apache:master Dec 14, 2022

asfimport mentioned this pull request Dec 15, 2022

[C++] Implement SubStrait SetRel (UnionAll) #32778

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-17520: [C++] Implement SubStrait SetRel (UnionAll) #14186

ARROW-17520: [C++] Implement SubStrait SetRel (UnionAll) #14186

vibhatha commented Sep 21, 2022

vibhatha commented Sep 21, 2022

github-actions bot commented Sep 21, 2022

github-actions bot commented Sep 21, 2022

westonpace left a comment

westonpace Sep 23, 2022

vibhatha Sep 24, 2022

vibhatha Sep 24, 2022

vibhatha Sep 24, 2022

vibhatha Sep 24, 2022

vibhatha Sep 24, 2022

westonpace Nov 15, 2022

vibhatha Nov 17, 2022

vibhatha commented Sep 24, 2022

westonpace left a comment

westonpace Nov 15, 2022

westonpace left a comment

vibhatha commented Dec 12, 2022

westonpace left a comment

vibhatha commented Dec 14, 2022

ursabot commented Dec 15, 2022

ARROW-17520: [C++] Implement SubStrait SetRel (UnionAll) #14186

ARROW-17520: [C++] Implement SubStrait SetRel (UnionAll) #14186

Conversation

vibhatha commented Sep 21, 2022

vibhatha commented Sep 21, 2022

github-actions bot commented Sep 21, 2022

github-actions bot commented Sep 21, 2022

westonpace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vibhatha commented Sep 24, 2022

westonpace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonpace left a comment

Choose a reason for hiding this comment

vibhatha commented Dec 12, 2022

westonpace left a comment

Choose a reason for hiding this comment

vibhatha commented Dec 14, 2022

ursabot commented Dec 15, 2022