ARROW-15582: [C++] Add support for registering tricky functions with the Substrait consumer (or add a bunch of substrait meta functions) #13285

sanjibansg · 2022-06-01T15:07:56Z

[WIP]
This PR adds function mappings for compute functions from substrait to arrow and vice-versa. This introduces a FunctionMapping class to register and store the mappings and supply when required. Registering a function includes encoding the various options and arguments in the respective mapping function's definition.

github-actions · 2022-06-01T15:25:04Z

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

github-actions · 2022-06-01T15:25:05Z

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

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

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

westonpace

I think we can make a pass at simplifying some things. A lot of these lambdas seem to follow a consistent pattern. This is a good start however! Excited to see it.

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

westonpace · 2022-06-02T05:50:02Z

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

+Status FunctionMapping::AddArrowToSubstrait(std::string arrow_function_name, ArrowToSubstrait conversion_func){
+  if (arrow_to_substrait.find(arrow_function_name) != arrow_to_substrait.end()){
+    arrow_to_substrait[arrow_function_name] =  conversion_func;
+  }


Should we return an invalid status as an else clause here?

Made the change.

Returning an AlreadyExist status.

westonpace · 2022-06-02T05:50:15Z

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

+Status FunctionMapping::AddSubstraitToArrow(std::string substrait_function_name, SubstraitToArrow conversion_func){
+  if (substrait_to_arrow.find(substrait_function_name) != substrait_to_arrow.end()){
+    substrait_to_arrow[substrait_function_name] =  conversion_func;
+  }


Same, perhaps the else should be an invalid status.

Returning a AlreadyExist status here, will that be better?

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

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

…egistered

westonpace

Some suggestions from a quick scan.

westonpace · 2022-06-17T00:14:54Z

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

-        return compute::call(func_name, std::move(arguments), std::move(cast_options));
-      }
+    case substrait::Expression::kEnum: {
+      auto enum_expr = expr.enum_();


Does this convert to the string value of the enum? Can you add a small comment here explaining that.

westonpace · 2022-06-17T00:32:01Z

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

@@ -204,6 +209,521 @@ const int* GetIndex(const KeyToIndex& key_to_index, const Key& key) {
  return &it->second;
 }

+Status FunctionMapping::AddArrowToSubstrait(std::string arrow_function_name, ArrowToSubstrait conversion_func){
+  if (arrow_to_substrait.find(arrow_function_name) != arrow_to_substrait.end()){


This logic seems backwards to me...wouldn't umap.find(...) != umap.end() mean the item already existed?

westonpace · 2022-06-17T00:46:23Z

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

+}
+
+Status FunctionMapping::AddSubstraitToArrow(std::string substrait_function_name, SubstraitToArrow conversion_func){
+  if (substrait_to_arrow.find(substrait_function_name) != substrait_to_arrow.end()){


Same as above. This seems backwards (but maybe I'm just not thinking right)

westonpace · 2022-06-17T00:47:32Z

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

+   }
+ }
+
+std::vector<arrow::compute::Expression> substrait_convert_arguments(const substrait::Expression::ScalarFunction& call){


Suggested change

std::vector<arrow::compute::Expression> substrait_convert_arguments(const substrait::Expression::ScalarFunction& call){

std::vector<arrow::compute::Expression> ConvertSubstraitArguments(const substrait::Expression::ScalarFunction& call){

westonpace · 2022-06-17T00:47:45Z

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

+
+std::vector<arrow::compute::Expression> substrait_convert_arguments(const substrait::Expression::ScalarFunction& call){
+  substrait::Expression value;
+  ExtensionSet ext_set_;


Suggested change

ExtensionSet ext_set_;

ExtensionSet ext_set;

This seems strange. Wouldn't this function take in an extension set as an argument?

westonpace · 2022-06-17T00:50:40Z

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

+
+substrait::Expression::ScalarFunction arrow_convert_enum_arguments(const arrow::compute::Expression::Call& call, substrait::Expression::ScalarFunction& substrait_call, ExtensionSet* ext_set_, std::string overflow_handling){
+  substrait::Expression::Enum options;
+  options.set_specified(overflow_handling);


overflow_handling seems like an odd name given this is a generic function

westonpace · 2022-06-17T00:56:49Z

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

+  return arrow::compute::call("abs", substrait_convert_arguments(call));
+};
+
+ArrowToSubstrait arrow_add_to_substrait = [] (const arrow::compute::Expression::Call& call, ExtensionSet* ext_set_) -> Result<substrait::Expression::ScalarFunction> {


There's a lot of places where you have ext_set_ and it should probably be ext_set. For the sake of brevity I'm not going to mark them all.

westonpace · 2022-06-17T01:00:58Z

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

+  substrait::Expression::ScalarFunction substrait_call;
+  ARROW_ASSIGN_OR_RAISE(auto function_reference, ext_set_->EncodeFunction("extract"));
+  substrait_call.set_function_reference(function_reference);


All of these calls to EncodeFunction seem pretty repetitive. Is there any way we can move this into the part that calls GetArrowToSubstrait? Also, I don't see anything today that calls GetArrowToSubstrait

westonpace · 2022-06-17T01:02:37Z

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

+  }
+};
+
+ArrowToSubstrait arrow_year_to_arrow = [] (const arrow::compute::Expression::Call& call, ExtensionSet* ext_set_) -> Result<substrait::Expression::ScalarFunction> {


arrow_...to_arrow?

westonpace · 2022-06-17T01:06:37Z

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

+    DCHECK_OK(functions_map.AddSubstraitToArrow(id.name.to_string(), conversion_func));  
+    return RegisterFunction(id, id.name.to_string());


It seems a little odd that we need two maps. What happens if two functions exist with the same name but different URIs? Thinking on this longer, maybe substrait_to_arrow should replace the map in the extension id registry (that gets updated by the call to RegisterFunction?)

…ctions (#13613) This picks up where #13285 has left off. It mostly focuses on the Substrait->Arrow direction at the moment. In addition, basic support is added for named tables. This makes it possible to create unit tests that read from in-memory tables instead of requiring unit tests to do a scan. The PR creates some utilities in `test_plan_builder.h` which allow for the construction of simple Substrait plans programmatically. This is used to create unit tests for the function mapping. The PR extracts id "ownership" out of the `ExtensionIdRegistry` and into its own `IdStorage` class. The PR gets rid of `NestedExtensionIdRegistryImpl` and instead makes `ExtensionIdRegistryImpl` nested if `parent_ != nullptr`. Authored-by: Weston Pace <[email protected]> Signed-off-by: Weston Pace <[email protected]>

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

westonpace self-requested a review June 2, 2022 01:57

sanjibansg commented Jun 2, 2022

View reviewed changes

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

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

westonpace requested changes Jun 2, 2022

View reviewed changes

sanjibansg added 7 commits June 3, 2022 15:44

feat: compute functions mapping for substrait

7830d16

feat: using function mappings in extension set

91740f8

feat: variadic boolean function mapping

5cbdcf5

remove: enum for all function mappings

ffc77d9

feat: Get functions to return status invalid if function not found

754b3a3

feat: registration api to return invalid status if function already r…

23f37f5

…egistered

feat: helpers for conversion functions

b28e5b1

sanjibansg force-pushed the substrait/compute_functions branch from a56a556 to b28e5b1 Compare June 3, 2022 10:31

sanjibansg added 5 commits June 4, 2022 01:04

feat: support for subtract and multiply functions

3b939c1

feat: mapped modulus function

76891e6

feat: remaining comparison function mapping

99cab94

feat: mappings for string functions

a27e03f

feat: mapping for cast function

93c16b5

westonpace mentioned this pull request Jun 9, 2022

fix: add overflow behavior to integer division substrait-io/substrait#223

Merged

feat: mapping for datetime function

7e27a7f

sanjibansg force-pushed the substrait/compute_functions branch from 41fc8fe to 7e27a7f Compare June 9, 2022 21:13

sanjibansg added 3 commits June 10, 2022 02:48

feat: mappings for divide functions

d0f8ff3

feat: mappings for sum & avg aggregate functions

a716c30

feat: register functions to use function mappings

56cbfff

westonpace self-requested a review June 16, 2022 15:47

westonpace requested changes Jun 17, 2022

View reviewed changes

review changes

3f39eae

westonpace mentioned this pull request Jul 14, 2022

ARROW-15582: [C++] Add support for registering standard Substrait functions #13613

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-15582: [C++] Add support for registering tricky functions with the Substrait consumer (or add a bunch of substrait meta functions) #13285

ARROW-15582: [C++] Add support for registering tricky functions with the Substrait consumer (or add a bunch of substrait meta functions) #13285

sanjibansg commented Jun 1, 2022

github-actions bot commented Jun 1, 2022

github-actions bot commented Jun 1, 2022

westonpace left a comment

westonpace Jun 2, 2022

sanjibansg Jun 7, 2022

sanjibansg Jun 7, 2022

westonpace Jun 2, 2022

sanjibansg Jun 7, 2022

westonpace left a comment

westonpace Jun 17, 2022

westonpace Jun 17, 2022

westonpace Jun 17, 2022

westonpace Jun 17, 2022

westonpace Jun 17, 2022

westonpace Jun 17, 2022

westonpace Jun 17, 2022

westonpace Jun 17, 2022

westonpace Jun 17, 2022

westonpace Jun 17, 2022

westonpace Jun 17, 2022

	std::vector<arrow::compute::Expression> substrait_convert_arguments(const substrait::Expression::ScalarFunction& call){
	std::vector<arrow::compute::Expression> ConvertSubstraitArguments(const substrait::Expression::ScalarFunction& call){

		DCHECK_OK(functions_map.AddSubstraitToArrow(id.name.to_string(), conversion_func));
		return RegisterFunction(id, id.name.to_string());

ARROW-15582: [C++] Add support for registering tricky functions with the Substrait consumer (or add a bunch of substrait meta functions) #13285

Are you sure you want to change the base?

ARROW-15582: [C++] Add support for registering tricky functions with the Substrait consumer (or add a bunch of substrait meta functions) #13285

Conversation

sanjibansg commented Jun 1, 2022

github-actions bot commented Jun 1, 2022

github-actions bot commented Jun 1, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment