[GitHub] [arrow] mapleFU commented on a diff in pull request #34606: GH-34605: [C++] Don't use std::move when passing shared_ptr to named table …

["mapleFU (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org