ARROW-16816: [C++] Upgrade Substrait to v0.6.0 #13468

jvanstraten · 2022-06-29T18:38:46Z

Note: I actually upgraded to v0.6.0; it didn't make much sense to me to not just go to the latest release. I guess I'll downgrade if there was a specific reason for going to exactly v0.3.0 that I'm not aware of.

Stuff that broke:

relations.proto and expressions.proto were merged into algebra.proto in refactor: combine relations and expressions into algebra.proto substrait-io/substrait#136
Breaking change in how file formats are specified in read relations: feat: add Arrow and Orc file formats substrait-io/substrait#169
Deprecation in specification of function arguments, switched to the new format (supporting the old one as well would be a bit more work, which I'm not sure is worthwhile at this stage): Add support for alternative argument type support in the protobuf representation (type and option arguments) substrait-io/substrait#161
Deprecation of user_defined_type_reference in Type, replacing it with a message that also supports nullability: feat: support nullable and non-default variation user-defined types substrait-io/substrait#217

github-actions · 2022-06-29T18:39:14Z

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

github-actions · 2022-06-29T18:39:16Z

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

kou · 2022-06-29T23:04:36Z

Could you update v0.3.0 in pull request and JIRA issue to v0.6.0?

westonpace · 2022-06-30T19:30:37Z

@vibhatha do you mind taking a look at this PR?

vibhatha · 2022-07-02T00:58:11Z

@vibhatha do you mind taking a look at this PR?

Of course 👍

vibhatha · 2022-07-02T01:04:14Z

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

+        switch (item.file_format_case()) {
+          case substrait::ReadRel_LocalFiles_FileOrFiles::kParquet:
+            format = std::make_shared<dataset::ParquetFileFormat>();
+            break;
+          case substrait::ReadRel_LocalFiles_FileOrFiles::kArrow:
+            format = std::make_shared<dataset::IpcFileFormat>();
+            break;
+          default:
+            return Status::NotImplemented(
+                "unknown substrait::ReadRel::LocalFiles::FileOrFiles::file_format");


What about feather format?

I think we are ok. The feather format (v2) and the arrow IPC format are the same thing. Sometimes people use the extension .arrow and sometimes they use the extension .feather. However, in both cases they should be specifying kArrow here.

That make sense.

vibhatha · 2022-07-02T01:07:17Z

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

@@ -185,7 +185,8 @@ TEST(Substrait, SupportedExtensionTypes) {
    ASSERT_OK_AND_ASSIGN(
        auto buf,
        internal::SubstraitFromJSON(
-            "Type", "{\"user_defined_type_reference\": " + std::to_string(anchor) + "}"));
+            "Type", "{\"user_defined\": { \"type_reference\": " + std::to_string(anchor) +
+                        ", \"nullability\": \"NULLABILITY_NULLABLE\" } }"));


nullability did we missed including this last time? Or are we planning to expose this too? My knowledge is not quite adequate here, appreciate some comments.

No, Substrait itself didn't consider user-defined types to conceptually have nullability, for no particular reason I can think of. See substrait-io/substrait#217

I see. Let’s go with what you have now.

vibhatha · 2022-07-02T01:10:40Z

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

-      return std::make_pair(std::move(type_record.type), true);
+      return std::make_pair(std::move(type_record.type), IsNullable(user_defined));


@jvanstraten
Before it was set to true always. Not sure why.

I think @bkietz just set it to true in the initial PR because nullable types are the default in Arrow and he had nowhere to get the flag from.

vibhatha · 2022-07-02T01:12:33Z

@jvanstraten thank you for working on this one. I added a few comments.

@westonpace Looks good to me.

vibhatha · 2022-07-02T02:50:43Z

@jvanstraten some R test cases are failing. May be it need some work there.

jvanstraten · 2022-07-02T10:17:54Z

I noticed, but I have seen exactly 0 lines of R code in my life before, nor do I have a toolchain set up for it. If the R bindings are using Substrait already and this isn't some weird CI thing, I'm afraid I'll need considerable help from someone in that subcommunity. Do you know anyone I can ping for that?

How does the Arrow community generally deal with upgrading dependencies past breaking changes that require interdisciplinary work? I don't think these will be the last breaking changes we'll see from Substrait in the foreseeable future, and considering the fact that Arrow is one of the leading projects that's requesting changes to Substrait it seems prudent to upgrade regularly.

vibhatha · 2022-07-02T10:20:31Z

@jvanstraten I will take a look at the R component and see what’s exactly going on. Later on I will tag for support.

jvanstraten · 2022-07-05T10:34:26Z

I spent the time to get R up and running on my machine and (depending on what CI will say) have fixed it already. The problem was just that the plan in the one R test case had to be upgraded to use the new file format specification.

westonpace

Looks great. Thanks for creating this. We had been falling quite far behind.

westonpace · 2022-07-05T16:13:35Z

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

+            return Status::NotImplemented(
+                "only value arguments are currently supported for functions");


Minor nit: If possible, it might be nice to include the string value of what was specified? E.g. "Only value is supported but got "enum". But I don't remember if it is easy to go from arg_type_case to a string.

I don't think that's a thing, at least not automatically. I could list the options, but if Substrait adds something later it's not going to upgrade automatically. The same applies to plenty of other of switch statements in the code currently. Also, if a plan with a function is passed to Arrow that uses argument features that Arrow doesn't even understand, the bigger issue will be that Arrow would have no idea what the function means to begin with. So I guess I could add the function name to the message.

If there is no easy enum->string then let's not worry about it. I think this is fine.

:ate to the party, but in case it helps, I see there is EnumDescriptor.

Ah, good to know that it does exist. It still wouldn't be very helpful to users if a new option is added since the last time Arrow bumped Substrait, but I guess that's shifting the goalposts a little.

The Arrow code could include in the message the value converted to a string if it is known given the Substrait version the code was built with, and otherwise include the number of the value and explain it is unknown and coming from a different version of Substrait. All this enum-handling is for a separate issue, of course.

westonpace · 2022-07-05T16:17:23Z

@sanjibansg This PR will likely have impacts on function mapping work.

@rtpsw You may be interested in these changes as well. I'm not sure how you've been creating plans so far but this change from the old style of specifying arguments to the new style is likely to have impacts.

rtpsw · 2022-07-05T17:53:27Z

Great! Thanks for letting me know, @westonpace.

Note: I actually upgraded to v0.6.0; it didn't make much sense to me to not just go to the latest release. I guess I'll downgrade if there was a specific reason for going to exactly v0.3.0 that I'm not aware of. Stuff that broke: - `relations.proto` and `expressions.proto` were merged into `algebra.proto` in substrait-io/substrait#136 - Breaking change in how file formats are specified in read relations: substrait-io/substrait#169 - Deprecation in specification of function arguments, switched to the new format (supporting the old one as well would be a bit more work, which I'm not sure is worthwhile at this stage): substrait-io/substrait#161 - Deprecation of `user_defined_type_reference` in `Type`, replacing it with a message that also supports nullability: substrait-io/substrait#217 Authored-by: Jeroen van Straten <[email protected]> Signed-off-by: Weston Pace <[email protected]>

Update Substrait version to current latest (v0.6.0)

4aac68c

github-actions bot added the Component: C++ label Jun 29, 2022

jvanstraten changed the title ~~ARROW-16816: Upgrade Substrait to v0.3.0~~ ARROW-16816: Upgrade Substrait to v0.6.0 Jun 30, 2022

westonpace self-requested a review June 30, 2022 19:30

kou changed the title ~~ARROW-16816: Upgrade Substrait to v0.6.0~~ ARROW-16816: [C++] Upgrade Substrait to v0.6.0 Jul 1, 2022

vibhatha reviewed Jul 2, 2022

View reviewed changes

Update plan in R test case to Substrait v0.6.0

8392929

github-actions bot added the Component: R label Jul 5, 2022

jvanstraten added 2 commits July 5, 2022 13:24

Fix cmake formatting

e459eaa

Fix Substrait plan in Python test case

d5b1b74

github-actions bot added the Component: Python label Jul 5, 2022

westonpace approved these changes Jul 5, 2022

View reviewed changes

westonpace merged commit ad78a31 into apache:master Jul 5, 2022

jvanstraten deleted the ARROW-16816-Upgrade-Substrait-to-v0-3-0 branch July 5, 2022 16:44

JkSelf mentioned this pull request Aug 24, 2022

Upgrade substrait to 0.7.0 oap-project/arrow#148

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-16816: [C++] Upgrade Substrait to v0.6.0 #13468

ARROW-16816: [C++] Upgrade Substrait to v0.6.0 #13468

jvanstraten commented Jun 29, 2022

github-actions bot commented Jun 29, 2022

github-actions bot commented Jun 29, 2022

kou commented Jun 29, 2022

westonpace commented Jun 30, 2022

vibhatha commented Jul 2, 2022

vibhatha Jul 2, 2022

westonpace Jul 2, 2022 •

edited

Loading

vibhatha Jul 2, 2022

vibhatha Jul 2, 2022

jvanstraten Jul 2, 2022

vibhatha Jul 2, 2022

vibhatha Jul 2, 2022

jvanstraten Jul 2, 2022

vibhatha Jul 2, 2022

vibhatha commented Jul 2, 2022

vibhatha commented Jul 2, 2022

jvanstraten commented Jul 2, 2022

vibhatha commented Jul 2, 2022

jvanstraten commented Jul 5, 2022

westonpace left a comment

westonpace Jul 5, 2022

jvanstraten Jul 5, 2022

westonpace Jul 5, 2022

rtpsw Jul 5, 2022

jvanstraten Jul 5, 2022

rtpsw Jul 5, 2022

westonpace commented Jul 5, 2022

rtpsw commented Jul 5, 2022

		return std::make_pair(std::move(type_record.type), true);
		return std::make_pair(std::move(type_record.type), IsNullable(user_defined));

		return Status::NotImplemented(
		"only value arguments are currently supported for functions");

ARROW-16816: [C++] Upgrade Substrait to v0.6.0 #13468

ARROW-16816: [C++] Upgrade Substrait to v0.6.0 #13468

Conversation

jvanstraten commented Jun 29, 2022

github-actions bot commented Jun 29, 2022

github-actions bot commented Jun 29, 2022

kou commented Jun 29, 2022

westonpace commented Jun 30, 2022

vibhatha commented Jul 2, 2022

Choose a reason for hiding this comment

westonpace Jul 2, 2022 • edited Loading

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 Jul 2, 2022

vibhatha commented Jul 2, 2022

jvanstraten commented Jul 2, 2022

vibhatha commented Jul 2, 2022

jvanstraten commented Jul 5, 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

westonpace commented Jul 5, 2022

rtpsw commented Jul 5, 2022

westonpace Jul 2, 2022 •

edited

Loading