Skip to content

Commit 771205b

Browse files
tadejakou
andauthored
GH-49435: [CI][C++] Fix macOS build with Homebrew protobuf v34 (#49491)
### Rationale for this change Closes #49435 ### What changes are included in this PR? a) Add brew update to `cpp.yml` and `python.yml` for GitHub Actions runner to use updated formulae b) Handle 'nodiscard' return values in Flight and Substrait See grpc/grpc#41755 (See also previous discussion in [#49436](#49436 (comment))) ### Are these changes tested? Yes, macOS C++ and Python CI jobs pass. ### Are there any user-facing changes? No. * GitHub Issue: #49435 Lead-authored-by: Tadeja Kadunc <tadeja.kadunc@gmail.com> Co-authored-by: tadeja <tadeja@users.noreply.github.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Raúl Cumplido <raulcumplido@gmail.com>
1 parent 59e0810 commit 771205b

6 files changed

Lines changed: 31 additions & 5 deletions

File tree

.github/workflows/cpp.yml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,9 @@ jobs:
229229
# pkg-config formula is removed from GitHub Actions runner.
230230
brew uninstall pkg-config || :
231231
brew uninstall pkg-config@0.29.2 || :
232+
# Workaround for https://github.com/grpc/grpc/issues/41755
233+
# Remove once the runner ships a newer Homebrew.
234+
brew update
232235
brew bundle --file=cpp/Brewfile
233236
- name: Install MinIO
234237
run: |

.github/workflows/python.yml

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -199,8 +199,10 @@ jobs:
199199
# pkg-config formula is removed from GitHub Actions runner.
200200
brew uninstall pkg-config || :
201201
brew uninstall pkg-config@0.29.2 || :
202+
# Workaround for https://github.com/grpc/grpc/issues/41755
203+
# Remove once the runner ships a newer Homebrew.
204+
brew update
202205
brew bundle --file=cpp/Brewfile
203-
204206
python -m pip install \
205207
-r python/requirements-build.txt \
206208
-r python/requirements-test.txt

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -804,7 +804,13 @@ struct ScalarToProtoImpl {
804804
auto user_defined = std::make_unique<Lit::UserDefined>();
805805
user_defined->set_type_reference(anchor);
806806
auto value_any = std::make_unique<google::protobuf::Any>();
807+
#if PROTOBUF_VERSION >= 3015000
808+
if (!value_any->PackFrom(value)) {
809+
return Status::IOError("Failed to pack user-defined type value");
810+
}
811+
#else
807812
value_any->PackFrom(value);
813+
#endif
808814
user_defined->set_allocated_value(value_any.release());
809815
lit_->set_allocated_user_defined(user_defined.release());
810816
return Status::OK();

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

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,17 +68,23 @@ class DefaultExtensionProvider : public BaseExtensionProvider {
6868
const ExtensionSet& ext_set) override {
6969
if (rel.Is<substrait_ext::AsOfJoinRel>()) {
7070
substrait_ext::AsOfJoinRel as_of_join_rel;
71-
rel.UnpackTo(&as_of_join_rel);
71+
if (!rel.UnpackTo(&as_of_join_rel)) {
72+
return Status::IOError("Failed to unpack AsOfJoinRel");
73+
}
7274
return MakeAsOfJoinRel(inputs, as_of_join_rel, ext_set);
7375
}
7476
if (rel.Is<substrait_ext::NamedTapRel>()) {
7577
substrait_ext::NamedTapRel named_tap_rel;
76-
rel.UnpackTo(&named_tap_rel);
78+
if (!rel.UnpackTo(&named_tap_rel)) {
79+
return Status::IOError("Failed to unpack NamedTapRel");
80+
}
7781
return MakeNamedTapRel(conv_opts, inputs, named_tap_rel, ext_set);
7882
}
7983
if (rel.Is<substrait_ext::SegmentedAggregateRel>()) {
8084
substrait_ext::SegmentedAggregateRel seg_agg_rel;
81-
rel.UnpackTo(&seg_agg_rel);
85+
if (!rel.UnpackTo(&seg_agg_rel)) {
86+
return Status::IOError("Failed to unpack SegmentedAggregateRel");
87+
}
8288
return MakeSegmentedAggregateRel(conv_opts, inputs, seg_agg_rel, ext_set);
8389
}
8490
return Status::NotImplemented("Unrecognized extension in Substrait plan: ",

cpp/src/arrow/flight/sql/server.cc

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -537,7 +537,13 @@ arrow::Result<std::string> CreateStatementQueryTicket(
537537
ticket_statement_query.set_statement_handle(statement_handle);
538538

539539
google::protobuf::Any ticket;
540+
#if PROTOBUF_VERSION >= 3015000
541+
if (!ticket.PackFrom(ticket_statement_query)) {
542+
return Status::IOError("Failed to pack ticket");
543+
}
544+
#else
540545
ticket.PackFrom(ticket_statement_query);
546+
#endif
541547

542548
std::string ticket_string;
543549

cpp/src/arrow/flight/transport/grpc/serialization_internal.cc

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -353,7 +353,10 @@ ::grpc::Status FlightDataDeserialize(ByteBuffer* buffer,
353353
// Can't use ParseFromCodedStream as this reads the entire
354354
// rest of the stream into the descriptor command field.
355355
std::string buffer;
356-
pb_stream.ReadString(&buffer, length);
356+
if (!pb_stream.ReadString(&buffer, length)) {
357+
return {::grpc::StatusCode::INTERNAL,
358+
"Unable to read FlightDescriptor from protobuf"};
359+
}
357360
if (!pb_descriptor.ParseFromString(buffer)) {
358361
return {::grpc::StatusCode::INTERNAL, "Unable to parse FlightDescriptor"};
359362
}

0 commit comments

Comments
 (0)