westonpace commented on code in PR #13375:
URL: https://github.com/apache/arrow/pull/13375#discussion_r907801495
##########
cpp/src/arrow/engine/substrait/extension_set.cc:
##########
@@ -315,6 +315,11 @@ struct ExtensionIdRegistryImpl : ExtensionIdRegistry {
return Status::OK();
}
+ Status RegisterFunction(std::string uri, std::string name,
Review Comment:
I'm pretty sure the current configuration actually requires *more* copies
and not fewer. Consider the two implementations:
A - Take in string_view, make a copy of it, and store the copy into `uris_`
and `names_` (current impl)
B - Take in string, move it into `uris_` and `names_` (proposed impl)
If the caller no longer needs the string then they can do...
```
RegisterFunction(std::move(uri), std::move(name), std::move(function_name));
```
...no copy would be needed for approach B but a copy would be needed for
approach A. If the caller needs a copy to be made (they still need to use uri
or name) then they simply would not move and one copy would be made.
That being said, the performance of one extra copy is probably negligible
here, and, as you pointed out, this is how the code was before. I don't think
we have to fix this if you don't want to do so right now.
--
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]
