Skip to content

Commit

Permalink
ARROW-15700: [C++] Compilation error on Ubuntu 18.04
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
jvanstraten authored and westonpace committed Feb 17, 2022
1 parent faef18b commit 7d16a78
Show file tree
Hide file tree
Showing 6 changed files with 31 additions and 14 deletions.
3 changes: 3 additions & 0 deletions cpp/cmake_modules/ThirdpartyToolchain.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -1530,6 +1530,9 @@ if(ARROW_WITH_PROTOBUF)
# google::protobuf::MessageLite::ByteSize() is deprecated since
# Protobuf 3.4.0.
set(ARROW_PROTOBUF_REQUIRED_VERSION "3.4.0")
elseif(ARROW_ENGINE)
# Substrait protobuf files use proto3 syntax
set(ARROW_PROTOBUF_REQUIRED_VERSION "3.0.0")
else()
set(ARROW_PROTOBUF_REQUIRED_VERSION "2.6.1")
endif()
Expand Down
2 changes: 2 additions & 0 deletions cpp/src/arrow/engine/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ if(MSVC)
list(APPEND SUBSTRAIT_SUPPRESSED_WARNINGS "/wd4251")
endif()

file(MAKE_DIRECTORY "${SUBSTRAIT_GEN_DIR}/substrait")

set(SUBSTRAIT_PROTO_GEN_ALL)
foreach(SUBSTRAIT_PROTO ${SUBSTRAIT_PROTOS})
set(SUBSTRAIT_PROTO_GEN "${SUBSTRAIT_GEN_DIR}/substrait/${SUBSTRAIT_PROTO}.pb")
Expand Down
29 changes: 19 additions & 10 deletions cpp/src/arrow/engine/substrait/expression_internal.cc
Original file line number Diff line number Diff line change
Expand Up @@ -443,10 +443,10 @@ struct ScalarToProtoImpl {
return Status::OK();
}

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

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

Status Visit(const StringScalar& s) { return FromBuffer(&Lit::set_string, s); }
Status Visit(const BinaryScalar& s) { return FromBuffer(&Lit::set_binary, s); }
Status Visit(const StringScalar& s) {
return FromBuffer([](Lit* lit, std::string&& s) { lit->set_string(std::move(s)); },
s);
}
Status Visit(const BinaryScalar& s) {
return FromBuffer([](Lit* lit, std::string&& s) { lit->set_binary(std::move(s)); },
s);
}

Status Visit(const FixedSizeBinaryScalar& s) {
return FromBuffer(&Lit::set_fixed_binary, s);
return FromBuffer(
[](Lit* lit, std::string&& s) { lit->set_fixed_binary(std::move(s)); }, s);
}

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

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

if (UnwrapFixedChar(*s.type)) {
return FromBuffer(&Lit::set_fixed_char,
checked_cast<const FixedSizeBinaryScalar&>(*s.value));
return FromBuffer(
[](Lit* lit, std::string&& s) { lit->set_fixed_char(std::move(s)); },
checked_cast<const FixedSizeBinaryScalar&>(*s.value));
}

if (auto length = UnwrapVarChar(*s.type)) {
Expand Down Expand Up @@ -857,7 +865,8 @@ Result<std::unique_ptr<substrait::Expression>> ToProto(const compute::Expression
// catch the special case of calls convertible to a ListElement
if (arguments[0]->has_selection() &&
arguments[0]->selection().has_direct_reference()) {
if (arguments[1]->has_literal() && arguments[1]->literal().has_i32()) {
if (arguments[1]->has_literal() && arguments[1]->literal().literal_type_case() ==
substrait::Expression_Literal::kI32) {
return MakeListElementReference(std::move(arguments[0]),
arguments[1]->literal().i32());
}
Expand Down
2 changes: 2 additions & 0 deletions cpp/src/arrow/engine/substrait/plan_internal.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
#include "arrow/util/make_unique.h"
#include "arrow/util/unreachable.h"

#include <unordered_map>

namespace arrow {

using internal::checked_cast;
Expand Down
3 changes: 2 additions & 1 deletion cpp/src/arrow/engine/substrait/relation_internal.cc
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ Result<compute::Declaration> FromProto(const substrait::Rel& rel,
std::vector<std::shared_ptr<dataset::FileFragment>> fragments;

for (const auto& item : read.local_files().items()) {
if (!item.has_uri_file()) {
if (item.path_type_case() !=
substrait::ReadRel_LocalFiles_FileOrFiles::kUriFile) {
return Status::NotImplemented(
"substrait::ReadRel::LocalFiles::FileOrFiles with "
"path_type other than uri_file");
Expand Down
6 changes: 3 additions & 3 deletions cpp/src/arrow/engine/substrait/type_internal.cc
Original file line number Diff line number Diff line change
Expand Up @@ -79,16 +79,16 @@ Result<FieldVector> FieldsFromProto(int size, const Types& types,
std::shared_ptr<DataType> type;
bool nullable;

if (types[i].has_struct_()) {
const auto& struct_ = types[i].struct_();
if (types.Get(i).has_struct_()) {
const auto& struct_ = types.Get(i).struct_();

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

nullable = IsNullable(struct_);
} else {
ARROW_ASSIGN_OR_RAISE(std::tie(type, nullable), FromProto(types[i], ext_set));
ARROW_ASSIGN_OR_RAISE(std::tie(type, nullable), FromProto(types.Get(i), ext_set));
}

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

0 comments on commit 7d16a78

Please sign in to comment.