mapleFU commented on code in PR #34606:
URL: https://github.com/apache/arrow/pull/34606#discussion_r1140697842
##########
cpp/src/arrow/engine/substrait/options.cc:
##########
@@ -161,10 +161,9 @@ class DefaultExtensionProvider : public
BaseExtensionProvider {
ARROW_ASSIGN_OR_RAISE(auto renamed_schema, schema->WithNames(columns));
auto input_decls = MakeDeclarationInputs(inputs);
ARROW_ASSIGN_OR_RAISE(
- auto decl,
- conv_opts.named_tap_provider(named_tap_rel.kind(), input_decls,
- named_tap_rel.name(),
std::move(renamed_schema)));
- return RelationInfo{{std::move(decl), std::move(renamed_schema)},
std::nullopt};
+ auto decl, conv_opts.named_tap_provider(named_tap_rel.kind(),
input_decls,
+ named_tap_rel.name(),
renamed_schema));
+ return RelationInfo{{std::move(decl), renamed_schema}, std::nullopt};
Review Comment:
`NamedTapProvider` takes `std::shared_ptr` as input.
```c++
using NamedTapProvider = std::function<Result<compute::Declaration>(
const std::string&, std::vector<compute::Declaration::Input>, const
std::string&,
std::shared_ptr<Schema>)>;
```
`std::shared_ptr` is a smart pointer, which is consitute from a control
block(with refcount) and data block. Calling copy to a `std::shared_ptr` would
copy the data block, copy the and inc the refcount. When calling move, it will
"move" control block and data block from moved pointer.
In this case, `conv_opts.named_tap_provider( ..., renamed_schema))` will
**copy** the shared_ptr. When it's copied, the original is not changed. And
previously, when move is called, the `renamed_schema` would be set to null.
On the second place, calling move will move the `renamed_schema` to
`RelationInfo`'s constructor. It's ok to move, because `renamed_schema` will
not be used in the remaining part of this function .
The previous copied `renamed_schema` will not be changed, because it's
control block and data block would not be changed.
If `named_tap_provider` just keeps `const std::shared_ptr<T>&`, rather than
a `std::shared_ptr<T>`, we need to copy. Otherwise, move the last
`renamed_schema` is ok
--
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]
