Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
westonpace merged 3 commits into from
Apr 10, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ba56188
MINOR: [C++] Remove std::move when calling ProcessEmit and ProcessExt…
icexelloss Mar 29, 2023
fb49302
Remove unneeded FromProto from expression_internal.h
icexelloss Mar 30, 2023
e5c2118
Revert std::move in return statement
icexelloss Apr 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions cpp/src/arrow/engine/substrait/expression_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,6 @@ ARROW_ENGINE_EXPORT
Result<FieldRef> DirectReferenceFromProto(const substrait::Expression::FieldReference*,
const ExtensionSet&, const ConversionOptions&);

ARROW_ENGINE_EXPORT
Copy link
Contributor Author

@icexelloss icexelloss Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@westonpace westonpace Mar 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Result<compute::Expression> FromProto(const substrait::Expression::FieldReference*,
const ExtensionSet&, const ConversionOptions&,
std::optional<compute::Expression>);

ARROW_ENGINE_EXPORT
Result<compute::Expression> FromProto(const substrait::Expression&, const ExtensionSet&,
const ConversionOptions&);
Expand Down
27 changes: 10 additions & 17 deletions cpp/src/arrow/engine/substrait/relation_internal.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,9 +420,8 @@ Result<DeclarationInfo> FromProto(const substrait::Rel& rel, const ExtensionSet&
return Status::Invalid("Invalid NamedTable Source");
}

return ProcessEmit(std::move(read),
DeclarationInfo{std::move(source_decl), base_schema},
std::move(base_schema));
return ProcessEmit(read, DeclarationInfo{std::move(source_decl), base_schema},
base_schema);
}

if (!read.has_local_files()) {
Expand Down Expand Up @@ -568,8 +567,7 @@ Result<DeclarationInfo> FromProto(const substrait::Rel& rel, const ExtensionSet&
compute::Declaration{"scan", dataset::ScanNodeOptions{ds, scan_options}},
base_schema};

return ProcessEmit(std::move(read), std::move(scan_declaration),
std::move(base_schema));
return ProcessEmit(read, scan_declaration, base_schema);
}

case substrait::Rel::RelTypeCase::kFilter: {
Expand All @@ -594,8 +592,7 @@ Result<DeclarationInfo> FromProto(const substrait::Rel& rel, const ExtensionSet&
}),
input.output_schema};

return ProcessEmit(std::move(filter), std::move(filter_declaration),
input.output_schema);
return ProcessEmit(filter, filter_declaration, input.output_schema);
}

case substrait::Rel::RelTypeCase::kProject: {
Expand Down Expand Up @@ -648,8 +645,7 @@ Result<DeclarationInfo> FromProto(const substrait::Rel& rel, const ExtensionSet&
}),
project_schema};

return ProcessEmit(std::move(project), std::move(project_declaration),
std::move(project_schema));
return ProcessEmit(project, project_declaration, project_schema);
}

case substrait::Rel::RelTypeCase::kJoin: {
Expand Down Expand Up @@ -755,8 +751,7 @@ Result<DeclarationInfo> FromProto(const substrait::Rel& rel, const ExtensionSet&

DeclarationInfo join_declaration{std::move(join_dec), join_schema};

return ProcessEmit(std::move(join), std::move(join_declaration),
std::move(join_schema));
return ProcessEmit(join, join_declaration, join_schema);
}
case substrait::Rel::RelTypeCase::kAggregate: {
const auto& aggregate = rel.aggregate();
Expand Down Expand Up @@ -824,9 +819,8 @@ Result<DeclarationInfo> FromProto(const substrait::Rel& rel, const ExtensionSet&
std::move(aggregates), std::move(agg_src_fieldsets), std::move(keys),
std::move(key_field_ids), {}, {}, ext_set, conversion_options));

auto aggregate_schema = aggregate_declaration.output_schema;
return ProcessEmit(std::move(aggregate), std::move(aggregate_declaration),
std::move(aggregate_schema));
return ProcessEmit(aggregate, aggregate_declaration,
aggregate_declaration.output_schema);
}

case substrait::Rel::RelTypeCase::kExtensionLeaf:
Expand Down Expand Up @@ -870,7 +864,7 @@ Result<DeclarationInfo> FromProto(const substrait::Rel& rel, const ExtensionSet&
emit_order.push_back(emit_idx);
}
}
return ProcessExtensionEmit(std::move(ext_decl_info), emit_order,
return ProcessExtensionEmit(ext_decl_info, emit_order,
*ext_rel_info.field_output_indices);
}

Expand Down Expand Up @@ -913,8 +907,7 @@ Result<DeclarationInfo> FromProto(const substrait::Rel& rel, const ExtensionSet&
}

auto set_declaration = DeclarationInfo{union_declr, union_schema};
return ProcessEmit(std::move(set), std::move(set_declaration),
std::move(union_schema));
return ProcessEmit(set, set_declaration, union_schema);
}

default:
Expand Down