Skip to content

Commit 7d16a78

Browse files
jvanstratenwestonpace
authored andcommitted
ARROW-15700: [C++] Compilation error on Ubuntu 18.04
Modified Substrait consumer interaction with libprotobuf to support versions down to 3.0.0, which is the minimum required due to Substrait's usage of proto3 syntax. Tested locally with: ``` export ARROW_PROTOBUF_URL=https://github.com/protocolbuffers/protobuf/releases/download/v3.0.0/protobuf-cpp-3.0.0.tar.gz cmake \ --preset ninja-debug \ -DProtobuf_SOURCE=BUNDLED \ -DARROW_PROTOBUF_BUILD_VERSION=v3.0.0 \ -DARROW_PROTOBUF_BUILD_SHA256_CHECKSUM=318e8f375fb4e5333975a40e0d1215e855b4a8c581d692eb0eb7df70db1a8d4e ``` (Is there an easier way to do this without modifying versions.txt or 751fb9d? Also, the env var is needed only because Google isn't at all consistent with their release file naming that far back.) It'd also be nice to add this to CI, but it's probably excessive to always run for a PR, unless combined with some other run. Closes apache#12448 from jvanstraten/ARROW-15700-Compilation-error-on-Ubuntu-18-04 Authored-by: Jeroen van Straten <jeroen.van.straten@gmail.com> Signed-off-by: Weston Pace <weston.pace@gmail.com>
1 parent faef18b commit 7d16a78

File tree

6 files changed

+31
-14
lines changed

6 files changed

+31
-14
lines changed

cpp/cmake_modules/ThirdpartyToolchain.cmake

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1530,6 +1530,9 @@ if(ARROW_WITH_PROTOBUF)
15301530
# google::protobuf::MessageLite::ByteSize() is deprecated since
15311531
# Protobuf 3.4.0.
15321532
set(ARROW_PROTOBUF_REQUIRED_VERSION "3.4.0")
1533+
elseif(ARROW_ENGINE)
1534+
# Substrait protobuf files use proto3 syntax
1535+
set(ARROW_PROTOBUF_REQUIRED_VERSION "3.0.0")
15331536
else()
15341537
set(ARROW_PROTOBUF_REQUIRED_VERSION "2.6.1")
15351538
endif()

cpp/src/arrow/engine/CMakeLists.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,8 @@ if(MSVC)
6666
list(APPEND SUBSTRAIT_SUPPRESSED_WARNINGS "/wd4251")
6767
endif()
6868

69+
file(MAKE_DIRECTORY "${SUBSTRAIT_GEN_DIR}/substrait")
70+
6971
set(SUBSTRAIT_PROTO_GEN_ALL)
7072
foreach(SUBSTRAIT_PROTO ${SUBSTRAIT_PROTOS})
7173
set(SUBSTRAIT_PROTO_GEN "${SUBSTRAIT_GEN_DIR}/substrait/${SUBSTRAIT_PROTO}.pb")

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

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -443,10 +443,10 @@ struct ScalarToProtoImpl {
443443
return Status::OK();
444444
}
445445

446-
template <typename ScalarWithBufferValue>
447-
Status FromBuffer(void (substrait::Expression::Literal::*set)(std::string&&),
446+
template <typename LiteralSetter, typename ScalarWithBufferValue>
447+
Status FromBuffer(LiteralSetter&& set_lit,
448448
const ScalarWithBufferValue& scalar_with_buffer) {
449-
(lit_->*set)(scalar_with_buffer.value->ToString());
449+
set_lit(lit_, scalar_with_buffer.value->ToString());
450450
return Status::OK();
451451
}
452452

@@ -466,11 +466,18 @@ struct ScalarToProtoImpl {
466466
Status Visit(const FloatScalar& s) { return Primitive(&Lit::set_fp32, s); }
467467
Status Visit(const DoubleScalar& s) { return Primitive(&Lit::set_fp64, s); }
468468

469-
Status Visit(const StringScalar& s) { return FromBuffer(&Lit::set_string, s); }
470-
Status Visit(const BinaryScalar& s) { return FromBuffer(&Lit::set_binary, s); }
469+
Status Visit(const StringScalar& s) {
470+
return FromBuffer([](Lit* lit, std::string&& s) { lit->set_string(std::move(s)); },
471+
s);
472+
}
473+
Status Visit(const BinaryScalar& s) {
474+
return FromBuffer([](Lit* lit, std::string&& s) { lit->set_binary(std::move(s)); },
475+
s);
476+
}
471477

472478
Status Visit(const FixedSizeBinaryScalar& s) {
473-
return FromBuffer(&Lit::set_fixed_binary, s);
479+
return FromBuffer(
480+
[](Lit* lit, std::string&& s) { lit->set_fixed_binary(std::move(s)); }, s);
474481
}
475482

476483
Status Visit(const Date32Scalar& s) { return Primitive(&Lit::set_date, s); }
@@ -594,13 +601,14 @@ struct ScalarToProtoImpl {
594601

595602
Status Visit(const ExtensionScalar& s) {
596603
if (UnwrapUuid(*s.type)) {
597-
return FromBuffer(&Lit::set_uuid,
604+
return FromBuffer([](Lit* lit, std::string&& s) { lit->set_uuid(std::move(s)); },
598605
checked_cast<const FixedSizeBinaryScalar&>(*s.value));
599606
}
600607

601608
if (UnwrapFixedChar(*s.type)) {
602-
return FromBuffer(&Lit::set_fixed_char,
603-
checked_cast<const FixedSizeBinaryScalar&>(*s.value));
609+
return FromBuffer(
610+
[](Lit* lit, std::string&& s) { lit->set_fixed_char(std::move(s)); },
611+
checked_cast<const FixedSizeBinaryScalar&>(*s.value));
604612
}
605613

606614
if (auto length = UnwrapVarChar(*s.type)) {
@@ -857,7 +865,8 @@ Result<std::unique_ptr<substrait::Expression>> ToProto(const compute::Expression
857865
// catch the special case of calls convertible to a ListElement
858866
if (arguments[0]->has_selection() &&
859867
arguments[0]->selection().has_direct_reference()) {
860-
if (arguments[1]->has_literal() && arguments[1]->literal().has_i32()) {
868+
if (arguments[1]->has_literal() && arguments[1]->literal().literal_type_case() ==
869+
substrait::Expression_Literal::kI32) {
861870
return MakeListElementReference(std::move(arguments[0]),
862871
arguments[1]->literal().i32());
863872
}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323
#include "arrow/util/make_unique.h"
2424
#include "arrow/util/unreachable.h"
2525

26+
#include <unordered_map>
27+
2628
namespace arrow {
2729

2830
using internal::checked_cast;

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,8 @@ Result<compute::Declaration> FromProto(const substrait::Rel& rel,
9090
std::vector<std::shared_ptr<dataset::FileFragment>> fragments;
9191

9292
for (const auto& item : read.local_files().items()) {
93-
if (!item.has_uri_file()) {
93+
if (item.path_type_case() !=
94+
substrait::ReadRel_LocalFiles_FileOrFiles::kUriFile) {
9495
return Status::NotImplemented(
9596
"substrait::ReadRel::LocalFiles::FileOrFiles with "
9697
"path_type other than uri_file");

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -79,16 +79,16 @@ Result<FieldVector> FieldsFromProto(int size, const Types& types,
7979
std::shared_ptr<DataType> type;
8080
bool nullable;
8181

82-
if (types[i].has_struct_()) {
83-
const auto& struct_ = types[i].struct_();
82+
if (types.Get(i).has_struct_()) {
83+
const auto& struct_ = types.Get(i).struct_();
8484

8585
ARROW_ASSIGN_OR_RAISE(
8686
type, FieldsFromProto(struct_.types_size(), struct_.types(), next_name, ext_set)
8787
.Map(arrow::struct_));
8888

8989
nullable = IsNullable(struct_);
9090
} else {
91-
ARROW_ASSIGN_OR_RAISE(std::tie(type, nullable), FromProto(types[i], ext_set));
91+
ARROW_ASSIGN_OR_RAISE(std::tie(type, nullable), FromProto(types.Get(i), ext_set));
9292
}
9393

9494
fields[i] = field(std::move(name), std::move(type), nullable);

0 commit comments

Comments
 (0)