MINOR: [C++] Remove std::move when calling ProcessEmit and ProcessExtensionEmit #34777

icexelloss · 2023-03-29T18:36:36Z

Rationale for this change

I noticed a few places in relation_internal.cc that std::move is used incorrectly.

What changes are included in this PR?

Currently, std::move is used when passing arguments to ProcessEmit and ProcessExtensionEmit, however these two methods takes in const reference and therefore the std::move is not needed.
(orthogonal) Removed unneeded FromProto from header

Are these changes tested?

With existing tests

Are there any user-facing changes?

No

…ensionEmit

icexelloss · 2023-03-30T14:44:13Z

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

 Result<FieldRef> DirectReferenceFromProto(const substrait::Expression::FieldReference*,
                                          const ExtensionSet&, const ConversionOptions&);

-ARROW_ENGINE_EXPORT


Per this comment:
#34627 (comment)

I noticed and removed it here. Although unrelated to the original PR, I figured this is small enough change to piggy back here. If there are objection I can revert this change and put in separate PR.

icexelloss · 2023-03-30T14:44:59Z

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

  }
  emit_info.expressions = std::move(proj_field_refs);
  emit_info.schema = schema(std::move(emit_fields));
-  return std::move(emit_info);


I don't think std::move of return value is good practice. (Since this prevents NRVO) so I removed them here.

I recall that one of the older compilers (I think mingw) does not recognize that emit_info is the return value because the return type is Result<EmitInfo> and the type of emit_info is EmitInfo. See, for example, https://stackoverflow.com/questions/74459462/why-c-does-not-perform-rvo-to-stdoptional . However, maybe it was one of the older gcc compilers that we no longer support now that we upgraded to C++17.

We should detect it, if it is a problem, if the nightly tests pass. However, the existing convention in the code base is still to use std::move when the return type is Result<T> and the object is T.

Got it. I reverted this.

icexelloss · 2023-03-30T14:47:29Z

@westonpace Minor changes to clean up the code a bit - no rush

westonpace

Thanks. I definitely agree with removing std::move when calling ProcessEmit. I'm not sure about the return statement though (see my reply to your comment).

westonpace · 2023-03-31T17:09:30Z

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

 Result<FieldRef> DirectReferenceFromProto(const substrait::Expression::FieldReference*,
                                          const ExtensionSet&, const ConversionOptions&);

-ARROW_ENGINE_EXPORT


icexelloss · 2023-04-05T15:17:12Z

Thanks. I definitely agree with removing std::move when calling ProcessEmit. I'm not sure about the return statement though (see my reply to your comment).

Got it. I do see in other places where std::move is used for return. I guess it is a larger scope discussion whether we want to change that or not. Will revert that part in this PR.

westonpace

Thanks for the cleanup.

ursabot · 2023-04-11T06:23:32Z

Benchmark runs are scheduled for baseline = 2fe1733 and contender = a7c4e05. a7c4e05 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️1.25% ⬆️0.03%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.37% ⬆️0.03%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] a7c4e05a ec2-t3-xlarge-us-east-2
[Finished] a7c4e05a test-mac-arm
[Finished] a7c4e05a ursa-i9-9960x
[Finished] a7c4e05a ursa-thinkcentre-m75q
[Finished] 2fe17338 ec2-t3-xlarge-us-east-2
[Finished] 2fe17338 test-mac-arm
[Finished] 2fe17338 ursa-i9-9960x
[Finished] 2fe17338 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

…ensionEmit (apache#34777) ### Rationale for this change I noticed a few places in relation_internal.cc that std::move is used incorrectly. ### What changes are included in this PR? * Currently, `std::move` is used when passing arguments to `ProcessEmit` and `ProcessExtensionEmit`, however these two methods takes in const reference and therefore the `std::move` is not needed. * (orthogonal) Removed unneeded `FromProto` from header ### Are these changes tested? With existing tests ### Are there any user-facing changes? No Authored-by: Li Jin <ice.xelloss@gmail.com> Signed-off-by: Weston Pace <weston.pace@gmail.com>

icexelloss requested a review from westonpace as a code owner March 29, 2023 18:36

github-actions bot added Component: C++ awaiting committer review Awaiting committer review labels Mar 29, 2023

kou changed the title ~~MINOR: [C++] Remove std::move when calling ProcessEmit and ProcessExt…~~ MINOR: [C++] Remove std::move when calling ProcessEmit and ProcessExtensionEmit Mar 29, 2023

icexelloss added 2 commits March 30, 2023 10:16

MINOR: [C++] Remove std::move when calling ProcessEmit and ProcessExt…

ba56188

…ensionEmit

Remove unneeded FromProto from expression_internal.h

fb49302

icexelloss force-pushed the acero-emit-process-move branch from e00d5e5 to fb49302 Compare March 30, 2023 14:41

icexelloss commented Mar 30, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Mar 30, 2023

westonpace reviewed Mar 31, 2023

View reviewed changes

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 5, 2023

Revert std::move in return statement

e5c2118

icexelloss force-pushed the acero-emit-process-move branch from b49f836 to e5c2118 Compare April 5, 2023 18:18

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Apr 5, 2023

westonpace approved these changes Apr 10, 2023

View reviewed changes

westonpace merged commit a7c4e05 into apache:main Apr 10, 2023

github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Apr 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINOR: [C++] Remove std::move when calling ProcessEmit and ProcessExtensionEmit #34777

MINOR: [C++] Remove std::move when calling ProcessEmit and ProcessExtensionEmit #34777

Uh oh!

icexelloss commented Mar 29, 2023 •

edited

Loading

Uh oh!

icexelloss Mar 30, 2023

Uh oh!

westonpace Mar 31, 2023

Uh oh!

icexelloss Mar 30, 2023

Uh oh!

westonpace Mar 31, 2023

Uh oh!

icexelloss Apr 5, 2023

Uh oh!

icexelloss commented Mar 30, 2023

Uh oh!

westonpace left a comment

Uh oh!

westonpace Mar 31, 2023

Uh oh!

icexelloss commented Apr 5, 2023 •

edited

Loading

Uh oh!

westonpace left a comment

Uh oh!

ursabot commented Apr 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MINOR: [C++] Remove std::move when calling ProcessEmit and ProcessExtensionEmit #34777

MINOR: [C++] Remove std::move when calling ProcessEmit and ProcessExtensionEmit #34777

Uh oh!

Conversation

icexelloss commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

icexelloss Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

westonpace Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

icexelloss Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

westonpace Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

icexelloss Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

icexelloss commented Mar 30, 2023

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

westonpace Mar 31, 2023

Choose a reason for hiding this comment

Uh oh!

icexelloss commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

ursabot commented Apr 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

icexelloss commented Mar 29, 2023 •

edited

Loading

icexelloss commented Apr 5, 2023 •

edited

Loading