ARROW-17966: [C++] Adjust to new format for Substrait optional arguments #14415

westonpace · 2022-10-14T12:54:32Z

No description provided.

westonpace · 2022-10-14T12:54:38Z

This PR currently depends on a non-official (PR fork) version of Substrait. We probably should not merge until the corresponding Substrait PR has merged.

substrait-io/substrait#342

github-actions · 2022-10-14T12:54:56Z

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

github-actions · 2022-10-14T12:54:59Z

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

bkietz · 2022-10-19T14:34:56Z

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

+std::optional<std::vector<std::string> const*> SubstraitCall::GetOption(
+    std::string_view option_name) const {
+  auto opt = options_.find(std::string(option_name));
+  if (opt == options_.end()) {
+    return std::nullopt;


Nit: this could be simpler

Suggested change

std::optional<std::vector<std::string> const*> SubstraitCall::GetOption(

std::string_view option_name) const {

auto opt = options_.find(std::string(option_name));

if (opt == options_.end()) {

return std::nullopt;

const std::vector<std::string>* SubstraitCall::GetOption(

std::string_view option_name) const {

auto opt = options_.find(std::string(option_name));

if (opt == options_.end()) {

return nullptr;

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

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

bkietz · 2022-10-19T15:22:35Z

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

@@ -132,10 +133,29 @@ Result<ExtensionSet> GetExtensionSetFromPlan(const substrait::Plan& plan,
                            conversion_options, registry);
 }

+namespace {
+
+// FIXME Is there some way to get these from the cmake files?


There does not appear to be a way to inspect a substrait clone and get the version numbers apart from parsing CHANGELOG.md. You could add #cmakedefine ARROW_SUBSTRAIT_BUILD_VERSION to config.h.cmake then parse that here, or parse it in cmake and add #cmakedefine ARROW_SUBSTRAIT_BUILD_VERSION_MAJOR etc to config.h.cmake. This is for example how ThirdpartyToolchain.cmake provides a #define to indicate that jemalloc is vendored https://github.com/drin/arrow/blob/dea465396f981f9bd9862e501dc6750ca4fee6d9/cpp/cmake_modules/ThirdpartyToolchain.cmake#L1864-L1865 -> https://github.com/drin/arrow/blob/dea465396f981f9bd9862e501dc6750ca4fee6d9/cpp/src/arrow/util/config.h.cmake#L48

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

bkietz · 2022-10-19T15:53:22Z

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

@@ -645,22 +663,19 @@ struct ExtensionIdRegistryImpl : ExtensionIdRegistry {
 };

 template <typename Enum>
-using EnumParser = std::function<Result<Enum>(std::optional<std::string_view>)>;
+using EnumParser = std::function<Result<Enum>(std::string_view)>;

 template <typename Enum>
 EnumParser<Enum> GetEnumParser(const std::vector<std::string>& options) {
  std::unordered_map<std::string, Enum> parse_map;
  for (std::size_t i = 0; i < options.size(); i++) {
    parse_map[options[i]] = static_cast<Enum>(i + 1);


It's probably not necessary to reify "UNSPECIFIED" in all the enumerations. We will always either coerce to a default or error because a real value was required. That could be expressed in EnumParser as:

template <typename Enum> using EnumParser = std::function<Result<Enum>(std::string_view repr, std::optional<Enum> default)>;

... and it would save you some one-based indexing headache.

Probably out of scope for this PR

Agreed to leave out of scope for this PR. I also agree it would probably be a good idea. The "unspecified" thing is a relic from the old behavior where "unspecified" was something you had to explicitly specify.

westonpace · 2022-10-20T00:48:47Z

@bkietz Thanks for the review. I still need to take a look at the cmake changes but I'll try that out later.

westonpace · 2022-10-24T16:52:15Z

Deferring the cmake changes to https://issues.apache.org/jira/browse/ARROW-18145

westonpace · 2022-10-24T17:00:51Z

@bkietz I'm moving this to ready-for-review. @rtpsw has been trying to get Ibis / Arrow compatibility working and this PR enables that, even though it temporarily breaks us from the main branch of Substrait while we wait on substrait-io/substrait#342 to merge. However, there is some work that is stacking up on top of this and branch management has started to cause some headaches.

Curious to get your thoughts on moving forward as-is with this PR, to make it easier for people working on integration, and addressing any remaining discrepancies (e.g. Enum vs string) in follow-up PRs.

bkietz · 2022-10-31T14:52:59Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

@@ -657,6 +657,13 @@ else()
           "${THIRDPARTY_MIRROR_URL}/snappy-${ARROW_SNAPPY_BUILD_VERSION}.tar.gz")
 endif()

+# Remove these two lines once https://github.com/substrait-io/substrait/pull/342 merges


Moving forward sounds fine to me; this is why we're pulling this dependency from a URL after all. I've added https://issues.apache.org/jira/browse/ARROW-18201 to track the reversion. In the meantime, please move this modification to versions.txt, annotated with // TODO(ARROW-18201)

westonpace · 2022-11-14T19:39:26Z

The feature has now merged into Substrait. Unfortunately, it seems releases are automated on Sundays so this probably can't merge until after Sunday at which time I will change the version numbers to be 0.20.0.

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

rok · 2022-11-22T13:18:19Z

Can I help moving this along somehow? If not please ignore.

…nal enum args to proper options. Added check for minimum Substrait version

…ndling to check major version and not just minor

Co-authored-by: Benjamin Kietzman <[email protected]>

… substrait option that Acero doesn't support

…nces for an option

…0.20.0

westonpace · 2022-11-22T17:53:25Z

@rok @bkietz Now that substrait version 0.20.0 is out, with this change, I think this is good to go.

westonpace · 2022-11-22T23:54:50Z

The mingw failures appear to be unrelated

ursabot · 2022-11-26T15:41:43Z

Benchmark runs are scheduled for baseline = 7276c35 and contender = 63f013c. 63f013c 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.2% ⬆️0.07%] test-mac-arm
[Finished ⬇️0.54% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.38% ⬆️0.03%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 63f013cd ec2-t3-xlarge-us-east-2
[Finished] 63f013cd test-mac-arm
[Finished] 63f013cd ursa-i9-9960x
[Finished] 63f013cd ursa-thinkcentre-m75q
[Finished] 7276c359 ec2-t3-xlarge-us-east-2
[Finished] 7276c359 test-mac-arm
[Finished] 7276c359 ursa-i9-9960x
[Finished] 7276c359 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

github-actions bot added Component: C++ Component: Python labels Oct 14, 2022

bkietz requested changes Oct 19, 2022

View reviewed changes

westonpace force-pushed the feature/ARROW-17966--dont-require-optional-args branch from 26fd452 to f03fcd9 Compare October 20, 2022 00:57

westonpace marked this pull request as ready for review October 24, 2022 16:54

rtpsw mentioned this pull request Oct 24, 2022

ARROW-17980: [C++] As-of-Join Substrait extension #14485

Merged

bkietz requested changes Oct 31, 2022

View reviewed changes

westonpace force-pushed the feature/ARROW-17966--dont-require-optional-args branch from 0b929f2 to 3858293 Compare November 14, 2022 19:25

bkietz requested changes Nov 15, 2022

View reviewed changes

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

westonpace force-pushed the feature/ARROW-17966--dont-require-optional-args branch from a94979a to e158f2d Compare November 22, 2022 03:27

westonpace and others added 11 commits November 22, 2022 09:07

ARROW-17966: Updated to latest Substrait version. Switched from optio…

f52939a

…nal enum args to proper options. Added check for minimum Substrait version

ARROW-17966: Add version to python substrait examples. Fix version ha…

8523535

…ndling to check major version and not just minor

ARROW-17966: Update cpp/src/arrow/engine/substrait/extension_set.cc

5c123c1

Co-authored-by: Benjamin Kietzman <[email protected]>

ARROW-17966: Update cpp/src/arrow/engine/substrait/extension_set.cc

3596a7d

Co-authored-by: Benjamin Kietzman <[email protected]>

ARROW-17966: Update cpp/src/arrow/engine/substrait/extension_set.cc

2b99035

Co-authored-by: Benjamin Kietzman <[email protected]>

ARROW-17966: Update cpp/src/arrow/engine/substrait/extension_set.cc

d1c44fb

Co-authored-by: Benjamin Kietzman <[email protected]>

ARROW-17966: Display the available choices when a user enters a valid…

988dcd2

… substrait option that Acero doesn't support

ARROW-17966: Simplify parsing boilerplate per review comments

45e7fb3

ARROW-17966: Gracefully error if the user does not supply any prefere…

a6dac84

…nces for an option

ARROW-17966: Prefer range loops where possible

3446801

ARROW-17966: Rebase cleanup

e2da03b

bkietz and others added 3 commits November 22, 2022 09:07

ARROW-17966: Minor fix to failing unit tests: remove enum="unspecified"

e642091

ARROW-17966: Switched from temp substrait hard-coded hash to version …

5e01780

…0.20.0

ARROW-17966: Adjusted newly added functions after rebase

be691d4

westonpace force-pushed the feature/ARROW-17966--dont-require-optional-args branch from e158f2d to be691d4 Compare November 22, 2022 17:52

westonpace requested a review from bkietz November 22, 2022 17:53

westonpace merged commit 63f013c into apache:master Nov 25, 2022

thisisnic mentioned this pull request Nov 28, 2022

CI failures due to Arrow code update voltrondata/substrait-r#220

Open

asfimport mentioned this pull request Nov 26, 2022

[C++] Adjust to new format for Substrait optional arguments #33174

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-17966: [C++] Adjust to new format for Substrait optional arguments #14415

ARROW-17966: [C++] Adjust to new format for Substrait optional arguments #14415

westonpace commented Oct 14, 2022

westonpace commented Oct 14, 2022 •

edited by bkietz

Loading

github-actions bot commented Oct 14, 2022

github-actions bot commented Oct 14, 2022

bkietz Oct 19, 2022

bkietz Oct 19, 2022

bkietz Oct 19, 2022

westonpace Oct 20, 2022

westonpace commented Oct 20, 2022

westonpace commented Oct 24, 2022

westonpace commented Oct 24, 2022

bkietz Oct 31, 2022

westonpace commented Nov 14, 2022

rok commented Nov 22, 2022

westonpace commented Nov 22, 2022

westonpace commented Nov 22, 2022

ursabot commented Nov 26, 2022

ARROW-17966: [C++] Adjust to new format for Substrait optional arguments #14415

ARROW-17966: [C++] Adjust to new format for Substrait optional arguments #14415

Conversation

westonpace commented Oct 14, 2022

westonpace commented Oct 14, 2022 • edited by bkietz Loading

github-actions bot commented Oct 14, 2022

github-actions bot commented Oct 14, 2022

bkietz Oct 19, 2022

Choose a reason for hiding this comment

bkietz Oct 19, 2022

Choose a reason for hiding this comment

bkietz Oct 19, 2022

Choose a reason for hiding this comment

westonpace Oct 20, 2022

Choose a reason for hiding this comment

westonpace commented Oct 20, 2022

westonpace commented Oct 24, 2022

westonpace commented Oct 24, 2022

bkietz Oct 31, 2022

Choose a reason for hiding this comment

westonpace commented Nov 14, 2022

rok commented Nov 22, 2022

westonpace commented Nov 22, 2022

westonpace commented Nov 22, 2022

ursabot commented Nov 26, 2022

westonpace commented Oct 14, 2022 •

edited by bkietz

Loading