From ba561882db813f88b2e299c8f4e0eed71914cdcc Mon Sep 17 00:00:00 2001 From: Li Jin Date: Wed, 29 Mar 2023 14:34:31 -0400 Subject: [PATCH 1/3] MINOR: [C++] Remove std::move when calling ProcessEmit and ProcessExtensionEmit --- .../engine/substrait/relation_internal.cc | 36 ++++++++----------- 1 file changed, 14 insertions(+), 22 deletions(-) diff --git a/cpp/src/arrow/engine/substrait/relation_internal.cc b/cpp/src/arrow/engine/substrait/relation_internal.cc index 7f9c4c289fb..bcecc9a745c 100644 --- a/cpp/src/arrow/engine/substrait/relation_internal.cc +++ b/cpp/src/arrow/engine/substrait/relation_internal.cc @@ -89,7 +89,7 @@ Result GetEmitInfo(const RelMessage& rel, } emit_info.expressions = std::move(proj_field_refs); emit_info.schema = schema(std::move(emit_fields)); - return std::move(emit_info); + return emit_info; } Result ProcessEmitProject( @@ -420,9 +420,8 @@ Result 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()) { @@ -568,8 +567,7 @@ Result 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: { @@ -594,8 +592,7 @@ Result 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: { @@ -648,8 +645,7 @@ Result 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: { @@ -755,8 +751,7 @@ Result 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(); @@ -824,9 +819,7 @@ Result 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: @@ -870,7 +863,7 @@ Result 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); } @@ -913,8 +906,7 @@ Result 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: @@ -971,7 +963,7 @@ Result> NamedTableRelationConverter( } read_rel->set_allocated_named_table(read_rel_tn.release()); - return std::move(read_rel); + return read_rel; } Result> ScanRelationConverter( @@ -1015,7 +1007,7 @@ Result> ScanRelationConverter( read_rel_lfs->mutable_items()->AddAllocated(read_rel_lfs_ffs.release()); } read_rel->set_allocated_local_files(read_rel_lfs.release()); - return std::move(read_rel); + return read_rel; } Result> FilterRelationConverter( @@ -1045,7 +1037,7 @@ Result> FilterRelationConverter( ARROW_ASSIGN_OR_RAISE(auto subs_expr, ToProto(bound_expression, ext_set, conversion_options)); filter_rel->set_allocated_condition(subs_expr.release()); - return std::move(filter_rel); + return filter_rel; } } // namespace @@ -1094,7 +1086,7 @@ Result> ToProto( const ConversionOptions& conversion_options) { auto rel = std::make_unique(); RETURN_NOT_OK(SerializeAndCombineRelations(declr, ext_set, &rel, conversion_options)); - return std::move(rel); + return rel; } } // namespace engine From fb49302ff937538041964543f2a381c378063d88 Mon Sep 17 00:00:00 2001 From: Li Jin Date: Thu, 30 Mar 2023 10:40:56 -0400 Subject: [PATCH 2/3] Remove unneeded FromProto from expression_internal.h --- cpp/src/arrow/engine/substrait/expression_internal.h | 5 ----- cpp/src/arrow/engine/substrait/relation_internal.cc | 3 ++- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/cpp/src/arrow/engine/substrait/expression_internal.h b/cpp/src/arrow/engine/substrait/expression_internal.h index cddc46066f1..2ce2ee76af2 100644 --- a/cpp/src/arrow/engine/substrait/expression_internal.h +++ b/cpp/src/arrow/engine/substrait/expression_internal.h @@ -39,11 +39,6 @@ ARROW_ENGINE_EXPORT Result DirectReferenceFromProto(const substrait::Expression::FieldReference*, const ExtensionSet&, const ConversionOptions&); -ARROW_ENGINE_EXPORT -Result FromProto(const substrait::Expression::FieldReference*, - const ExtensionSet&, const ConversionOptions&, - std::optional); - ARROW_ENGINE_EXPORT Result FromProto(const substrait::Expression&, const ExtensionSet&, const ConversionOptions&); diff --git a/cpp/src/arrow/engine/substrait/relation_internal.cc b/cpp/src/arrow/engine/substrait/relation_internal.cc index bcecc9a745c..768b9c4c9f3 100644 --- a/cpp/src/arrow/engine/substrait/relation_internal.cc +++ b/cpp/src/arrow/engine/substrait/relation_internal.cc @@ -819,7 +819,8 @@ Result 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)); - return ProcessEmit(aggregate, aggregate_declaration, aggregate_declaration.output_schema); + return ProcessEmit(aggregate, aggregate_declaration, + aggregate_declaration.output_schema); } case substrait::Rel::RelTypeCase::kExtensionLeaf: From e5c21188294896d13b6adb9494b6f2f7817928f7 Mon Sep 17 00:00:00 2001 From: Li Jin Date: Wed, 5 Apr 2023 14:16:57 -0400 Subject: [PATCH 3/3] Revert std::move in return statement --- cpp/src/arrow/engine/substrait/relation_internal.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cpp/src/arrow/engine/substrait/relation_internal.cc b/cpp/src/arrow/engine/substrait/relation_internal.cc index 768b9c4c9f3..b00d2420863 100644 --- a/cpp/src/arrow/engine/substrait/relation_internal.cc +++ b/cpp/src/arrow/engine/substrait/relation_internal.cc @@ -89,7 +89,7 @@ Result GetEmitInfo(const RelMessage& rel, } emit_info.expressions = std::move(proj_field_refs); emit_info.schema = schema(std::move(emit_fields)); - return emit_info; + return std::move(emit_info); } Result ProcessEmitProject( @@ -964,7 +964,7 @@ Result> NamedTableRelationConverter( } read_rel->set_allocated_named_table(read_rel_tn.release()); - return read_rel; + return std::move(read_rel); } Result> ScanRelationConverter( @@ -1008,7 +1008,7 @@ Result> ScanRelationConverter( read_rel_lfs->mutable_items()->AddAllocated(read_rel_lfs_ffs.release()); } read_rel->set_allocated_local_files(read_rel_lfs.release()); - return read_rel; + return std::move(read_rel); } Result> FilterRelationConverter( @@ -1038,7 +1038,7 @@ Result> FilterRelationConverter( ARROW_ASSIGN_OR_RAISE(auto subs_expr, ToProto(bound_expression, ext_set, conversion_options)); filter_rel->set_allocated_condition(subs_expr.release()); - return filter_rel; + return std::move(filter_rel); } } // namespace @@ -1087,7 +1087,7 @@ Result> ToProto( const ConversionOptions& conversion_options) { auto rel = std::make_unique(); RETURN_NOT_OK(SerializeAndCombineRelations(declr, ext_set, &rel, conversion_options)); - return rel; + return std::move(rel); } } // namespace engine