lidavidm commented on a change in pull request #11507: URL: https://github.com/apache/arrow/pull/11507#discussion_r742233667
########## File path: cpp/src/arrow/flight/flight-sql/client_test.cc ########## @@ -0,0 +1,569 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <arrow/flight/client.h> +#include <arrow/flight/flight-sql/FlightSql.pb.h> +#include <arrow/flight/flight-sql/api.h> +#include <arrow/testing/gtest_util.h> +#include <gmock/gmock.h> +#include <google/protobuf/any.pb.h> +#include <gtest/gtest.h> + +#include <utility> + +namespace pb = arrow::flight::protocol; +using ::testing::_; +using ::testing::Ref; + +namespace arrow { +namespace flight { +namespace sql { + +namespace internal { +class FlightClientImpl { + public: + ~FlightClientImpl() = default; + + MOCK_METHOD(Status, GetFlightInfo, + (const FlightCallOptions&, const FlightDescriptor&, + std::unique_ptr<FlightInfo>*)); + MOCK_METHOD(Status, DoGet, + (const FlightCallOptions& options, const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream)); + MOCK_METHOD(Status, DoPut, + (const FlightCallOptions&, const FlightDescriptor&, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>*, + std::unique_ptr<FlightMetadataReader>*)); + MOCK_METHOD(Status, DoAction, + (const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results)); +}; + +Status FlightClientImpl_GetFlightInfo(FlightClientImpl* client, + const FlightCallOptions& options, + const FlightDescriptor& descriptor, + std::unique_ptr<FlightInfo>* info) { + return client->GetFlightInfo(options, descriptor, info); +} + +Status FlightClientImpl_DoPut(FlightClientImpl* client, const FlightCallOptions& options, + const FlightDescriptor& descriptor, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>* stream, + std::unique_ptr<FlightMetadataReader>* reader) { + return client->DoPut(options, descriptor, schema, stream, reader); +} + +Status FlightClientImpl_DoGet(FlightClientImpl* client, const FlightCallOptions& options, + const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream) { + return client->DoGet(options, ticket, stream); +} + +Status FlightClientImpl_DoAction(FlightClientImpl* client, + const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results) { + return client->DoAction(options, action, results); +} + +} // namespace internal + +class FlightMetadataReaderMock : public FlightMetadataReader { + public: + std::shared_ptr<Buffer>* buffer; + + explicit FlightMetadataReaderMock(std::shared_ptr<Buffer>* buffer) { + this->buffer = buffer; + } + + Status ReadMetadata(std::shared_ptr<Buffer>* out) override { + *out = *buffer; + return Status::OK(); + } +}; + +class FlightStreamWriterMock : public FlightStreamWriter { + public: + FlightStreamWriterMock() = default; + + Status DoneWriting() override { return Status::OK(); } + + Status WriteMetadata(std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema, + const ipc::IpcWriteOptions& options) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema) override { + return MetadataRecordBatchWriter::Begin(schema); + } + + ipc::WriteStats stats() const override { return ipc::WriteStats(); } + + Status WriteWithMetadata(const RecordBatch& batch, + std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Close() override { return Status::OK(); } + + Status WriteRecordBatch(const RecordBatch& batch) override { return Status::OK(); } +}; + +FlightDescriptor getDescriptor(google::protobuf::Message& command) { + google::protobuf::Any any; + any.PackFrom(command); + + const std::string& string = any.SerializeAsString(); + return FlightDescriptor::Command(string); +} + +TEST(TestFlightSqlClient, TestGetCatalogs) { + auto client_mock = std::make_shared<internal::FlightClientImpl>(); + FlightSqlClient sql_client(client_mock); Review comment: We're defining FlightClientImpl twice and just using the fact that only one definition is visible at a time, trusting that the compiler/linker do the right thing. I'm not sure we can rely on this. ########## File path: cpp/src/arrow/flight/flight-sql/client_test.cc ########## @@ -0,0 +1,569 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <arrow/flight/client.h> +#include <arrow/flight/flight-sql/FlightSql.pb.h> +#include <arrow/flight/flight-sql/api.h> +#include <arrow/testing/gtest_util.h> +#include <gmock/gmock.h> +#include <google/protobuf/any.pb.h> +#include <gtest/gtest.h> + +#include <utility> + +namespace pb = arrow::flight::protocol; +using ::testing::_; +using ::testing::Ref; + +namespace arrow { +namespace flight { +namespace sql { + +namespace internal { +class FlightClientImpl { + public: + ~FlightClientImpl() = default; + + MOCK_METHOD(Status, GetFlightInfo, + (const FlightCallOptions&, const FlightDescriptor&, + std::unique_ptr<FlightInfo>*)); + MOCK_METHOD(Status, DoGet, + (const FlightCallOptions& options, const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream)); + MOCK_METHOD(Status, DoPut, + (const FlightCallOptions&, const FlightDescriptor&, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>*, + std::unique_ptr<FlightMetadataReader>*)); + MOCK_METHOD(Status, DoAction, + (const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results)); +}; + +Status FlightClientImpl_GetFlightInfo(FlightClientImpl* client, + const FlightCallOptions& options, + const FlightDescriptor& descriptor, + std::unique_ptr<FlightInfo>* info) { + return client->GetFlightInfo(options, descriptor, info); +} + +Status FlightClientImpl_DoPut(FlightClientImpl* client, const FlightCallOptions& options, + const FlightDescriptor& descriptor, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>* stream, + std::unique_ptr<FlightMetadataReader>* reader) { + return client->DoPut(options, descriptor, schema, stream, reader); +} + +Status FlightClientImpl_DoGet(FlightClientImpl* client, const FlightCallOptions& options, + const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream) { + return client->DoGet(options, ticket, stream); +} + +Status FlightClientImpl_DoAction(FlightClientImpl* client, + const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results) { + return client->DoAction(options, action, results); +} + +} // namespace internal + +class FlightMetadataReaderMock : public FlightMetadataReader { + public: + std::shared_ptr<Buffer>* buffer; + + explicit FlightMetadataReaderMock(std::shared_ptr<Buffer>* buffer) { + this->buffer = buffer; + } + + Status ReadMetadata(std::shared_ptr<Buffer>* out) override { + *out = *buffer; + return Status::OK(); + } +}; + +class FlightStreamWriterMock : public FlightStreamWriter { + public: + FlightStreamWriterMock() = default; + + Status DoneWriting() override { return Status::OK(); } + + Status WriteMetadata(std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema, + const ipc::IpcWriteOptions& options) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema) override { + return MetadataRecordBatchWriter::Begin(schema); + } + + ipc::WriteStats stats() const override { return ipc::WriteStats(); } + + Status WriteWithMetadata(const RecordBatch& batch, + std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Close() override { return Status::OK(); } + + Status WriteRecordBatch(const RecordBatch& batch) override { return Status::OK(); } +}; + +FlightDescriptor getDescriptor(google::protobuf::Message& command) { + google::protobuf::Any any; + any.PackFrom(command); + + const std::string& string = any.SerializeAsString(); + return FlightDescriptor::Command(string); +} + +TEST(TestFlightSqlClient, TestGetCatalogs) { + auto client_mock = std::make_shared<internal::FlightClientImpl>(); + FlightSqlClient sql_client(client_mock); Review comment: I would expect that the object files for client.cc and client_test.cc both contain symbols for FlightClientImpl, and it just so happens that the linker picks the mock when compiling the test binary. ########## File path: cpp/src/arrow/flight/flight-sql/client_test.cc ########## @@ -0,0 +1,569 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <arrow/flight/client.h> +#include <arrow/flight/flight-sql/FlightSql.pb.h> +#include <arrow/flight/flight-sql/api.h> +#include <arrow/testing/gtest_util.h> +#include <gmock/gmock.h> +#include <google/protobuf/any.pb.h> +#include <gtest/gtest.h> + +#include <utility> + +namespace pb = arrow::flight::protocol; +using ::testing::_; +using ::testing::Ref; + +namespace arrow { +namespace flight { +namespace sql { + +namespace internal { +class FlightClientImpl { + public: + ~FlightClientImpl() = default; + + MOCK_METHOD(Status, GetFlightInfo, + (const FlightCallOptions&, const FlightDescriptor&, + std::unique_ptr<FlightInfo>*)); + MOCK_METHOD(Status, DoGet, + (const FlightCallOptions& options, const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream)); + MOCK_METHOD(Status, DoPut, + (const FlightCallOptions&, const FlightDescriptor&, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>*, + std::unique_ptr<FlightMetadataReader>*)); + MOCK_METHOD(Status, DoAction, + (const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results)); +}; + +Status FlightClientImpl_GetFlightInfo(FlightClientImpl* client, + const FlightCallOptions& options, + const FlightDescriptor& descriptor, + std::unique_ptr<FlightInfo>* info) { + return client->GetFlightInfo(options, descriptor, info); +} + +Status FlightClientImpl_DoPut(FlightClientImpl* client, const FlightCallOptions& options, + const FlightDescriptor& descriptor, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>* stream, + std::unique_ptr<FlightMetadataReader>* reader) { + return client->DoPut(options, descriptor, schema, stream, reader); +} + +Status FlightClientImpl_DoGet(FlightClientImpl* client, const FlightCallOptions& options, + const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream) { + return client->DoGet(options, ticket, stream); +} + +Status FlightClientImpl_DoAction(FlightClientImpl* client, + const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results) { + return client->DoAction(options, action, results); +} + +} // namespace internal + +class FlightMetadataReaderMock : public FlightMetadataReader { + public: + std::shared_ptr<Buffer>* buffer; + + explicit FlightMetadataReaderMock(std::shared_ptr<Buffer>* buffer) { + this->buffer = buffer; + } + + Status ReadMetadata(std::shared_ptr<Buffer>* out) override { + *out = *buffer; + return Status::OK(); + } +}; + +class FlightStreamWriterMock : public FlightStreamWriter { + public: + FlightStreamWriterMock() = default; + + Status DoneWriting() override { return Status::OK(); } + + Status WriteMetadata(std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema, + const ipc::IpcWriteOptions& options) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema) override { + return MetadataRecordBatchWriter::Begin(schema); + } + + ipc::WriteStats stats() const override { return ipc::WriteStats(); } + + Status WriteWithMetadata(const RecordBatch& batch, + std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Close() override { return Status::OK(); } + + Status WriteRecordBatch(const RecordBatch& batch) override { return Status::OK(); } +}; + +FlightDescriptor getDescriptor(google::protobuf::Message& command) { + google::protobuf::Any any; + any.PackFrom(command); + + const std::string& string = any.SerializeAsString(); + return FlightDescriptor::Command(string); +} + +TEST(TestFlightSqlClient, TestGetCatalogs) { + auto client_mock = std::make_shared<internal::FlightClientImpl>(); + FlightSqlClient sql_client(client_mock); Review comment: That said, my understanding of this is shaky and maybe there is a good justification for why this works. ########## File path: cpp/src/arrow/flight/flight-sql/client_test.cc ########## @@ -0,0 +1,569 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <arrow/flight/client.h> +#include <arrow/flight/flight-sql/FlightSql.pb.h> +#include <arrow/flight/flight-sql/api.h> +#include <arrow/testing/gtest_util.h> +#include <gmock/gmock.h> +#include <google/protobuf/any.pb.h> +#include <gtest/gtest.h> + +#include <utility> + +namespace pb = arrow::flight::protocol; +using ::testing::_; +using ::testing::Ref; + +namespace arrow { +namespace flight { +namespace sql { + +namespace internal { +class FlightClientImpl { + public: + ~FlightClientImpl() = default; + + MOCK_METHOD(Status, GetFlightInfo, + (const FlightCallOptions&, const FlightDescriptor&, + std::unique_ptr<FlightInfo>*)); + MOCK_METHOD(Status, DoGet, + (const FlightCallOptions& options, const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream)); + MOCK_METHOD(Status, DoPut, + (const FlightCallOptions&, const FlightDescriptor&, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>*, + std::unique_ptr<FlightMetadataReader>*)); + MOCK_METHOD(Status, DoAction, + (const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results)); +}; + +Status FlightClientImpl_GetFlightInfo(FlightClientImpl* client, + const FlightCallOptions& options, + const FlightDescriptor& descriptor, + std::unique_ptr<FlightInfo>* info) { + return client->GetFlightInfo(options, descriptor, info); +} + +Status FlightClientImpl_DoPut(FlightClientImpl* client, const FlightCallOptions& options, + const FlightDescriptor& descriptor, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>* stream, + std::unique_ptr<FlightMetadataReader>* reader) { + return client->DoPut(options, descriptor, schema, stream, reader); +} + +Status FlightClientImpl_DoGet(FlightClientImpl* client, const FlightCallOptions& options, + const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream) { + return client->DoGet(options, ticket, stream); +} + +Status FlightClientImpl_DoAction(FlightClientImpl* client, + const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results) { + return client->DoAction(options, action, results); +} + +} // namespace internal + +class FlightMetadataReaderMock : public FlightMetadataReader { + public: + std::shared_ptr<Buffer>* buffer; + + explicit FlightMetadataReaderMock(std::shared_ptr<Buffer>* buffer) { + this->buffer = buffer; + } + + Status ReadMetadata(std::shared_ptr<Buffer>* out) override { + *out = *buffer; + return Status::OK(); + } +}; + +class FlightStreamWriterMock : public FlightStreamWriter { + public: + FlightStreamWriterMock() = default; + + Status DoneWriting() override { return Status::OK(); } + + Status WriteMetadata(std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema, + const ipc::IpcWriteOptions& options) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema) override { + return MetadataRecordBatchWriter::Begin(schema); + } + + ipc::WriteStats stats() const override { return ipc::WriteStats(); } + + Status WriteWithMetadata(const RecordBatch& batch, + std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Close() override { return Status::OK(); } + + Status WriteRecordBatch(const RecordBatch& batch) override { return Status::OK(); } +}; + +FlightDescriptor getDescriptor(google::protobuf::Message& command) { + google::protobuf::Any any; + any.PackFrom(command); + + const std::string& string = any.SerializeAsString(); + return FlightDescriptor::Command(string); +} + +TEST(TestFlightSqlClient, TestGetCatalogs) { + auto client_mock = std::make_shared<internal::FlightClientImpl>(); + FlightSqlClient sql_client(client_mock); Review comment: Both libarrow_flight_sql.so and client_test.cc.o have symbols for FlightClientImpl for me. That said, they're marked as weak symbols, and presumably the one in the program takes precedence. (Confirmed with LD_DEBUG.) This works and I'm not going to hold this up, however, it is rather magical and it's worth at least a comment explaining the intent of FlightClientImpl. ########## File path: cpp/src/arrow/flight/flight-sql/client_test.cc ########## @@ -0,0 +1,569 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <arrow/flight/client.h> +#include <arrow/flight/flight-sql/FlightSql.pb.h> +#include <arrow/flight/flight-sql/api.h> +#include <arrow/testing/gtest_util.h> +#include <gmock/gmock.h> +#include <google/protobuf/any.pb.h> +#include <gtest/gtest.h> + +#include <utility> + +namespace pb = arrow::flight::protocol; +using ::testing::_; +using ::testing::Ref; + +namespace arrow { +namespace flight { +namespace sql { + +namespace internal { +class FlightClientImpl { + public: + ~FlightClientImpl() = default; + + MOCK_METHOD(Status, GetFlightInfo, + (const FlightCallOptions&, const FlightDescriptor&, + std::unique_ptr<FlightInfo>*)); + MOCK_METHOD(Status, DoGet, + (const FlightCallOptions& options, const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream)); + MOCK_METHOD(Status, DoPut, + (const FlightCallOptions&, const FlightDescriptor&, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>*, + std::unique_ptr<FlightMetadataReader>*)); + MOCK_METHOD(Status, DoAction, + (const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results)); +}; + +Status FlightClientImpl_GetFlightInfo(FlightClientImpl* client, + const FlightCallOptions& options, + const FlightDescriptor& descriptor, + std::unique_ptr<FlightInfo>* info) { + return client->GetFlightInfo(options, descriptor, info); +} + +Status FlightClientImpl_DoPut(FlightClientImpl* client, const FlightCallOptions& options, + const FlightDescriptor& descriptor, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>* stream, + std::unique_ptr<FlightMetadataReader>* reader) { + return client->DoPut(options, descriptor, schema, stream, reader); +} + +Status FlightClientImpl_DoGet(FlightClientImpl* client, const FlightCallOptions& options, + const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream) { + return client->DoGet(options, ticket, stream); +} + +Status FlightClientImpl_DoAction(FlightClientImpl* client, + const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results) { + return client->DoAction(options, action, results); +} + +} // namespace internal + +class FlightMetadataReaderMock : public FlightMetadataReader { + public: + std::shared_ptr<Buffer>* buffer; + + explicit FlightMetadataReaderMock(std::shared_ptr<Buffer>* buffer) { + this->buffer = buffer; + } + + Status ReadMetadata(std::shared_ptr<Buffer>* out) override { + *out = *buffer; + return Status::OK(); + } +}; + +class FlightStreamWriterMock : public FlightStreamWriter { + public: + FlightStreamWriterMock() = default; + + Status DoneWriting() override { return Status::OK(); } + + Status WriteMetadata(std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema, + const ipc::IpcWriteOptions& options) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema) override { + return MetadataRecordBatchWriter::Begin(schema); + } + + ipc::WriteStats stats() const override { return ipc::WriteStats(); } + + Status WriteWithMetadata(const RecordBatch& batch, + std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Close() override { return Status::OK(); } + + Status WriteRecordBatch(const RecordBatch& batch) override { return Status::OK(); } +}; + +FlightDescriptor getDescriptor(google::protobuf::Message& command) { + google::protobuf::Any any; + any.PackFrom(command); + + const std::string& string = any.SerializeAsString(); + return FlightDescriptor::Command(string); +} + +TEST(TestFlightSqlClient, TestGetCatalogs) { + auto client_mock = std::make_shared<internal::FlightClientImpl>(); + FlightSqlClient sql_client(client_mock); Review comment: Both libarrow_flight_sql.so and client_test.cc.o have symbols for FlightClientImpl for me. That said, they're marked as weak symbols, and presumably the one in the program takes precedence. (Confirmed with LD_DEBUG.) <details> ``` > nm -C src/arrow/flight/flight_sql/CMakeFiles/arrow-flight-sql-client-test.dir/client_test.cc.o | rg FlightClientImpl::DoPut 0000000000000000 W arrow::flight::sql::internal::FlightClientImpl::DoPut(arrow::flight::FlightCallOptions const&, arrow::flight::FlightDescriptor const&, std::shared_ptr<arrow::Schema> const&, std::unique_ptr<arrow::flight::FlightStreamWriter, std::default_delete<arrow::flight::FlightStreamWriter> >*, std::unique_ptr<arrow::flight::FlightMetadataReader, std::default_delete<arrow::flight::FlightMetadataReader> >*) > nm -C debug/libarrow_flight_sql.so | rg FlightClientImpl::DoPut 00000000000f1dc0 W arrow::flight::sql::internal::FlightClientImpl::DoPut(arrow::flight::FlightCallOptions const&, arrow::flight::FlightDescriptor const&, std::shared_ptr<arrow::Schema> const&, std::unique_ptr<arrow::flight::FlightStreamWriter, std::default_delete<arrow::flight::FlightStreamWriter> >*, std::unique_ptr<arrow::flight::FlightMetadataReader, std::default_delete<arrow::flight::FlightMetadataReader> >*) > nm -C debug/arrow-flight-sql-client-test | rg FlightClientImpl::DoPut 000000000055b320 W arrow::flight::sql::internal::FlightClientImpl::DoPut(arrow::flight::FlightCallOptions const&, arrow::flight::FlightDescriptor const&, std::shared_ptr<arrow::Schema> const&, std::unique_ptr<arrow::flight::FlightStreamWriter, std::default_delete<arrow::flight::FlightStreamWriter> >*, std::unique_ptr<arrow::flight::FlightMetadataReader, std::default_delete<arrow::flight::FlightMetadataReader> >*) ``` </details> This works and I'm not going to hold this up, however, it is rather magical and it's worth at least a comment explaining the intent of FlightClientImpl. ########## File path: cpp/src/arrow/flight/flight-sql/client_test.cc ########## @@ -0,0 +1,569 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <arrow/flight/client.h> +#include <arrow/flight/flight-sql/FlightSql.pb.h> +#include <arrow/flight/flight-sql/api.h> +#include <arrow/testing/gtest_util.h> +#include <gmock/gmock.h> +#include <google/protobuf/any.pb.h> +#include <gtest/gtest.h> + +#include <utility> + +namespace pb = arrow::flight::protocol; +using ::testing::_; +using ::testing::Ref; + +namespace arrow { +namespace flight { +namespace sql { + +namespace internal { +class FlightClientImpl { + public: + ~FlightClientImpl() = default; + + MOCK_METHOD(Status, GetFlightInfo, + (const FlightCallOptions&, const FlightDescriptor&, + std::unique_ptr<FlightInfo>*)); + MOCK_METHOD(Status, DoGet, + (const FlightCallOptions& options, const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream)); + MOCK_METHOD(Status, DoPut, + (const FlightCallOptions&, const FlightDescriptor&, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>*, + std::unique_ptr<FlightMetadataReader>*)); + MOCK_METHOD(Status, DoAction, + (const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results)); +}; + +Status FlightClientImpl_GetFlightInfo(FlightClientImpl* client, + const FlightCallOptions& options, + const FlightDescriptor& descriptor, + std::unique_ptr<FlightInfo>* info) { + return client->GetFlightInfo(options, descriptor, info); +} + +Status FlightClientImpl_DoPut(FlightClientImpl* client, const FlightCallOptions& options, + const FlightDescriptor& descriptor, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>* stream, + std::unique_ptr<FlightMetadataReader>* reader) { + return client->DoPut(options, descriptor, schema, stream, reader); +} + +Status FlightClientImpl_DoGet(FlightClientImpl* client, const FlightCallOptions& options, + const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream) { + return client->DoGet(options, ticket, stream); +} + +Status FlightClientImpl_DoAction(FlightClientImpl* client, + const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results) { + return client->DoAction(options, action, results); +} + +} // namespace internal + +class FlightMetadataReaderMock : public FlightMetadataReader { + public: + std::shared_ptr<Buffer>* buffer; + + explicit FlightMetadataReaderMock(std::shared_ptr<Buffer>* buffer) { + this->buffer = buffer; + } + + Status ReadMetadata(std::shared_ptr<Buffer>* out) override { + *out = *buffer; + return Status::OK(); + } +}; + +class FlightStreamWriterMock : public FlightStreamWriter { + public: + FlightStreamWriterMock() = default; + + Status DoneWriting() override { return Status::OK(); } + + Status WriteMetadata(std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema, + const ipc::IpcWriteOptions& options) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema) override { + return MetadataRecordBatchWriter::Begin(schema); + } + + ipc::WriteStats stats() const override { return ipc::WriteStats(); } + + Status WriteWithMetadata(const RecordBatch& batch, + std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Close() override { return Status::OK(); } + + Status WriteRecordBatch(const RecordBatch& batch) override { return Status::OK(); } +}; + +FlightDescriptor getDescriptor(google::protobuf::Message& command) { + google::protobuf::Any any; + any.PackFrom(command); + + const std::string& string = any.SerializeAsString(); + return FlightDescriptor::Command(string); +} + +TEST(TestFlightSqlClient, TestGetCatalogs) { + auto client_mock = std::make_shared<internal::FlightClientImpl>(); + FlightSqlClient sql_client(client_mock); Review comment: Er, sorry for the churn @rafael-telles but I think the latest commit is strictly worse. I think the situation is OK as-is so long as it's clearly documented (and I can't claim to be a linker expert or C++ standard expert so maybe this is all OK and I don't understand the reason why). I'm mostly just concerned that it's unclear what the intent is. ########## File path: cpp/src/arrow/flight/flight-sql/client_test.cc ########## @@ -0,0 +1,569 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <arrow/flight/client.h> +#include <arrow/flight/flight-sql/FlightSql.pb.h> +#include <arrow/flight/flight-sql/api.h> +#include <arrow/testing/gtest_util.h> +#include <gmock/gmock.h> +#include <google/protobuf/any.pb.h> +#include <gtest/gtest.h> + +#include <utility> + +namespace pb = arrow::flight::protocol; +using ::testing::_; +using ::testing::Ref; + +namespace arrow { +namespace flight { +namespace sql { + +namespace internal { +class FlightClientImpl { + public: + ~FlightClientImpl() = default; + + MOCK_METHOD(Status, GetFlightInfo, + (const FlightCallOptions&, const FlightDescriptor&, + std::unique_ptr<FlightInfo>*)); + MOCK_METHOD(Status, DoGet, + (const FlightCallOptions& options, const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream)); + MOCK_METHOD(Status, DoPut, + (const FlightCallOptions&, const FlightDescriptor&, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>*, + std::unique_ptr<FlightMetadataReader>*)); + MOCK_METHOD(Status, DoAction, + (const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results)); +}; + +Status FlightClientImpl_GetFlightInfo(FlightClientImpl* client, + const FlightCallOptions& options, + const FlightDescriptor& descriptor, + std::unique_ptr<FlightInfo>* info) { + return client->GetFlightInfo(options, descriptor, info); +} + +Status FlightClientImpl_DoPut(FlightClientImpl* client, const FlightCallOptions& options, + const FlightDescriptor& descriptor, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>* stream, + std::unique_ptr<FlightMetadataReader>* reader) { + return client->DoPut(options, descriptor, schema, stream, reader); +} + +Status FlightClientImpl_DoGet(FlightClientImpl* client, const FlightCallOptions& options, + const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream) { + return client->DoGet(options, ticket, stream); +} + +Status FlightClientImpl_DoAction(FlightClientImpl* client, + const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results) { + return client->DoAction(options, action, results); +} + +} // namespace internal + +class FlightMetadataReaderMock : public FlightMetadataReader { + public: + std::shared_ptr<Buffer>* buffer; + + explicit FlightMetadataReaderMock(std::shared_ptr<Buffer>* buffer) { + this->buffer = buffer; + } + + Status ReadMetadata(std::shared_ptr<Buffer>* out) override { + *out = *buffer; + return Status::OK(); + } +}; + +class FlightStreamWriterMock : public FlightStreamWriter { + public: + FlightStreamWriterMock() = default; + + Status DoneWriting() override { return Status::OK(); } + + Status WriteMetadata(std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema, + const ipc::IpcWriteOptions& options) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema) override { + return MetadataRecordBatchWriter::Begin(schema); + } + + ipc::WriteStats stats() const override { return ipc::WriteStats(); } + + Status WriteWithMetadata(const RecordBatch& batch, + std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Close() override { return Status::OK(); } + + Status WriteRecordBatch(const RecordBatch& batch) override { return Status::OK(); } +}; + +FlightDescriptor getDescriptor(google::protobuf::Message& command) { + google::protobuf::Any any; + any.PackFrom(command); + + const std::string& string = any.SerializeAsString(); + return FlightDescriptor::Command(string); +} + +TEST(TestFlightSqlClient, TestGetCatalogs) { + auto client_mock = std::make_shared<internal::FlightClientImpl>(); + FlightSqlClient sql_client(client_mock); Review comment: Perhaps, but even then, I would say that an implementation that requires very carefully linking objects together to be correct is not one we should go with. ########## File path: cpp/src/arrow/flight/flight-sql/client_test.cc ########## @@ -0,0 +1,569 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <arrow/flight/client.h> +#include <arrow/flight/flight-sql/FlightSql.pb.h> +#include <arrow/flight/flight-sql/api.h> +#include <arrow/testing/gtest_util.h> +#include <gmock/gmock.h> +#include <google/protobuf/any.pb.h> +#include <gtest/gtest.h> + +#include <utility> + +namespace pb = arrow::flight::protocol; +using ::testing::_; +using ::testing::Ref; + +namespace arrow { +namespace flight { +namespace sql { + +namespace internal { +class FlightClientImpl { + public: + ~FlightClientImpl() = default; + + MOCK_METHOD(Status, GetFlightInfo, + (const FlightCallOptions&, const FlightDescriptor&, + std::unique_ptr<FlightInfo>*)); + MOCK_METHOD(Status, DoGet, + (const FlightCallOptions& options, const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream)); + MOCK_METHOD(Status, DoPut, + (const FlightCallOptions&, const FlightDescriptor&, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>*, + std::unique_ptr<FlightMetadataReader>*)); + MOCK_METHOD(Status, DoAction, + (const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results)); +}; + +Status FlightClientImpl_GetFlightInfo(FlightClientImpl* client, + const FlightCallOptions& options, + const FlightDescriptor& descriptor, + std::unique_ptr<FlightInfo>* info) { + return client->GetFlightInfo(options, descriptor, info); +} + +Status FlightClientImpl_DoPut(FlightClientImpl* client, const FlightCallOptions& options, + const FlightDescriptor& descriptor, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>* stream, + std::unique_ptr<FlightMetadataReader>* reader) { + return client->DoPut(options, descriptor, schema, stream, reader); +} + +Status FlightClientImpl_DoGet(FlightClientImpl* client, const FlightCallOptions& options, + const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream) { + return client->DoGet(options, ticket, stream); +} + +Status FlightClientImpl_DoAction(FlightClientImpl* client, + const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results) { + return client->DoAction(options, action, results); +} + +} // namespace internal + +class FlightMetadataReaderMock : public FlightMetadataReader { + public: + std::shared_ptr<Buffer>* buffer; + + explicit FlightMetadataReaderMock(std::shared_ptr<Buffer>* buffer) { + this->buffer = buffer; + } + + Status ReadMetadata(std::shared_ptr<Buffer>* out) override { + *out = *buffer; + return Status::OK(); + } +}; + +class FlightStreamWriterMock : public FlightStreamWriter { + public: + FlightStreamWriterMock() = default; + + Status DoneWriting() override { return Status::OK(); } + + Status WriteMetadata(std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema, + const ipc::IpcWriteOptions& options) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema) override { + return MetadataRecordBatchWriter::Begin(schema); + } + + ipc::WriteStats stats() const override { return ipc::WriteStats(); } + + Status WriteWithMetadata(const RecordBatch& batch, + std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Close() override { return Status::OK(); } + + Status WriteRecordBatch(const RecordBatch& batch) override { return Status::OK(); } +}; + +FlightDescriptor getDescriptor(google::protobuf::Message& command) { + google::protobuf::Any any; + any.PackFrom(command); + + const std::string& string = any.SerializeAsString(); + return FlightDescriptor::Command(string); +} + +TEST(TestFlightSqlClient, TestGetCatalogs) { + auto client_mock = std::make_shared<internal::FlightClientImpl>(); + FlightSqlClient sql_client(client_mock); Review comment: (Sorry - I want to try some alternative approaches but haven't found the time. At this rate it may be Friday before I can get into things…) ########## File path: cpp/src/arrow/flight/flight-sql/client_test.cc ########## @@ -0,0 +1,569 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <arrow/flight/client.h> +#include <arrow/flight/flight-sql/FlightSql.pb.h> +#include <arrow/flight/flight-sql/api.h> +#include <arrow/testing/gtest_util.h> +#include <gmock/gmock.h> +#include <google/protobuf/any.pb.h> +#include <gtest/gtest.h> + +#include <utility> + +namespace pb = arrow::flight::protocol; +using ::testing::_; +using ::testing::Ref; + +namespace arrow { +namespace flight { +namespace sql { + +namespace internal { +class FlightClientImpl { + public: + ~FlightClientImpl() = default; + + MOCK_METHOD(Status, GetFlightInfo, + (const FlightCallOptions&, const FlightDescriptor&, + std::unique_ptr<FlightInfo>*)); + MOCK_METHOD(Status, DoGet, + (const FlightCallOptions& options, const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream)); + MOCK_METHOD(Status, DoPut, + (const FlightCallOptions&, const FlightDescriptor&, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>*, + std::unique_ptr<FlightMetadataReader>*)); + MOCK_METHOD(Status, DoAction, + (const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results)); +}; + +Status FlightClientImpl_GetFlightInfo(FlightClientImpl* client, + const FlightCallOptions& options, + const FlightDescriptor& descriptor, + std::unique_ptr<FlightInfo>* info) { + return client->GetFlightInfo(options, descriptor, info); +} + +Status FlightClientImpl_DoPut(FlightClientImpl* client, const FlightCallOptions& options, + const FlightDescriptor& descriptor, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>* stream, + std::unique_ptr<FlightMetadataReader>* reader) { + return client->DoPut(options, descriptor, schema, stream, reader); +} + +Status FlightClientImpl_DoGet(FlightClientImpl* client, const FlightCallOptions& options, + const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream) { + return client->DoGet(options, ticket, stream); +} + +Status FlightClientImpl_DoAction(FlightClientImpl* client, + const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results) { + return client->DoAction(options, action, results); +} + +} // namespace internal + +class FlightMetadataReaderMock : public FlightMetadataReader { + public: + std::shared_ptr<Buffer>* buffer; + + explicit FlightMetadataReaderMock(std::shared_ptr<Buffer>* buffer) { + this->buffer = buffer; + } + + Status ReadMetadata(std::shared_ptr<Buffer>* out) override { + *out = *buffer; + return Status::OK(); + } +}; + +class FlightStreamWriterMock : public FlightStreamWriter { + public: + FlightStreamWriterMock() = default; + + Status DoneWriting() override { return Status::OK(); } + + Status WriteMetadata(std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema, + const ipc::IpcWriteOptions& options) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema) override { + return MetadataRecordBatchWriter::Begin(schema); + } + + ipc::WriteStats stats() const override { return ipc::WriteStats(); } + + Status WriteWithMetadata(const RecordBatch& batch, + std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Close() override { return Status::OK(); } + + Status WriteRecordBatch(const RecordBatch& batch) override { return Status::OK(); } +}; + +FlightDescriptor getDescriptor(google::protobuf::Message& command) { + google::protobuf::Any any; + any.PackFrom(command); + + const std::string& string = any.SerializeAsString(); + return FlightDescriptor::Command(string); +} + +TEST(TestFlightSqlClient, TestGetCatalogs) { + auto client_mock = std::make_shared<internal::FlightClientImpl>(); + FlightSqlClient sql_client(client_mock); Review comment: We're defining FlightClientImpl twice and just using the fact that only one definition is visible at a time, trusting that the compiler/linker do the right thing. I'm not sure we can rely on this. ########## File path: cpp/src/arrow/flight/flight-sql/client_test.cc ########## @@ -0,0 +1,569 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <arrow/flight/client.h> +#include <arrow/flight/flight-sql/FlightSql.pb.h> +#include <arrow/flight/flight-sql/api.h> +#include <arrow/testing/gtest_util.h> +#include <gmock/gmock.h> +#include <google/protobuf/any.pb.h> +#include <gtest/gtest.h> + +#include <utility> + +namespace pb = arrow::flight::protocol; +using ::testing::_; +using ::testing::Ref; + +namespace arrow { +namespace flight { +namespace sql { + +namespace internal { +class FlightClientImpl { + public: + ~FlightClientImpl() = default; + + MOCK_METHOD(Status, GetFlightInfo, + (const FlightCallOptions&, const FlightDescriptor&, + std::unique_ptr<FlightInfo>*)); + MOCK_METHOD(Status, DoGet, + (const FlightCallOptions& options, const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream)); + MOCK_METHOD(Status, DoPut, + (const FlightCallOptions&, const FlightDescriptor&, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>*, + std::unique_ptr<FlightMetadataReader>*)); + MOCK_METHOD(Status, DoAction, + (const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results)); +}; + +Status FlightClientImpl_GetFlightInfo(FlightClientImpl* client, + const FlightCallOptions& options, + const FlightDescriptor& descriptor, + std::unique_ptr<FlightInfo>* info) { + return client->GetFlightInfo(options, descriptor, info); +} + +Status FlightClientImpl_DoPut(FlightClientImpl* client, const FlightCallOptions& options, + const FlightDescriptor& descriptor, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>* stream, + std::unique_ptr<FlightMetadataReader>* reader) { + return client->DoPut(options, descriptor, schema, stream, reader); +} + +Status FlightClientImpl_DoGet(FlightClientImpl* client, const FlightCallOptions& options, + const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream) { + return client->DoGet(options, ticket, stream); +} + +Status FlightClientImpl_DoAction(FlightClientImpl* client, + const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results) { + return client->DoAction(options, action, results); +} + +} // namespace internal + +class FlightMetadataReaderMock : public FlightMetadataReader { + public: + std::shared_ptr<Buffer>* buffer; + + explicit FlightMetadataReaderMock(std::shared_ptr<Buffer>* buffer) { + this->buffer = buffer; + } + + Status ReadMetadata(std::shared_ptr<Buffer>* out) override { + *out = *buffer; + return Status::OK(); + } +}; + +class FlightStreamWriterMock : public FlightStreamWriter { + public: + FlightStreamWriterMock() = default; + + Status DoneWriting() override { return Status::OK(); } + + Status WriteMetadata(std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema, + const ipc::IpcWriteOptions& options) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema) override { + return MetadataRecordBatchWriter::Begin(schema); + } + + ipc::WriteStats stats() const override { return ipc::WriteStats(); } + + Status WriteWithMetadata(const RecordBatch& batch, + std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Close() override { return Status::OK(); } + + Status WriteRecordBatch(const RecordBatch& batch) override { return Status::OK(); } +}; + +FlightDescriptor getDescriptor(google::protobuf::Message& command) { + google::protobuf::Any any; + any.PackFrom(command); + + const std::string& string = any.SerializeAsString(); + return FlightDescriptor::Command(string); +} + +TEST(TestFlightSqlClient, TestGetCatalogs) { + auto client_mock = std::make_shared<internal::FlightClientImpl>(); + FlightSqlClient sql_client(client_mock); Review comment: I would expect that the object files for client.cc and client_test.cc both contain symbols for FlightClientImpl, and it just so happens that the linker picks the mock when compiling the test binary. ########## File path: cpp/src/arrow/flight/flight-sql/client_test.cc ########## @@ -0,0 +1,569 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <arrow/flight/client.h> +#include <arrow/flight/flight-sql/FlightSql.pb.h> +#include <arrow/flight/flight-sql/api.h> +#include <arrow/testing/gtest_util.h> +#include <gmock/gmock.h> +#include <google/protobuf/any.pb.h> +#include <gtest/gtest.h> + +#include <utility> + +namespace pb = arrow::flight::protocol; +using ::testing::_; +using ::testing::Ref; + +namespace arrow { +namespace flight { +namespace sql { + +namespace internal { +class FlightClientImpl { + public: + ~FlightClientImpl() = default; + + MOCK_METHOD(Status, GetFlightInfo, + (const FlightCallOptions&, const FlightDescriptor&, + std::unique_ptr<FlightInfo>*)); + MOCK_METHOD(Status, DoGet, + (const FlightCallOptions& options, const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream)); + MOCK_METHOD(Status, DoPut, + (const FlightCallOptions&, const FlightDescriptor&, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>*, + std::unique_ptr<FlightMetadataReader>*)); + MOCK_METHOD(Status, DoAction, + (const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results)); +}; + +Status FlightClientImpl_GetFlightInfo(FlightClientImpl* client, + const FlightCallOptions& options, + const FlightDescriptor& descriptor, + std::unique_ptr<FlightInfo>* info) { + return client->GetFlightInfo(options, descriptor, info); +} + +Status FlightClientImpl_DoPut(FlightClientImpl* client, const FlightCallOptions& options, + const FlightDescriptor& descriptor, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>* stream, + std::unique_ptr<FlightMetadataReader>* reader) { + return client->DoPut(options, descriptor, schema, stream, reader); +} + +Status FlightClientImpl_DoGet(FlightClientImpl* client, const FlightCallOptions& options, + const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream) { + return client->DoGet(options, ticket, stream); +} + +Status FlightClientImpl_DoAction(FlightClientImpl* client, + const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results) { + return client->DoAction(options, action, results); +} + +} // namespace internal + +class FlightMetadataReaderMock : public FlightMetadataReader { + public: + std::shared_ptr<Buffer>* buffer; + + explicit FlightMetadataReaderMock(std::shared_ptr<Buffer>* buffer) { + this->buffer = buffer; + } + + Status ReadMetadata(std::shared_ptr<Buffer>* out) override { + *out = *buffer; + return Status::OK(); + } +}; + +class FlightStreamWriterMock : public FlightStreamWriter { + public: + FlightStreamWriterMock() = default; + + Status DoneWriting() override { return Status::OK(); } + + Status WriteMetadata(std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema, + const ipc::IpcWriteOptions& options) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema) override { + return MetadataRecordBatchWriter::Begin(schema); + } + + ipc::WriteStats stats() const override { return ipc::WriteStats(); } + + Status WriteWithMetadata(const RecordBatch& batch, + std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Close() override { return Status::OK(); } + + Status WriteRecordBatch(const RecordBatch& batch) override { return Status::OK(); } +}; + +FlightDescriptor getDescriptor(google::protobuf::Message& command) { + google::protobuf::Any any; + any.PackFrom(command); + + const std::string& string = any.SerializeAsString(); + return FlightDescriptor::Command(string); +} + +TEST(TestFlightSqlClient, TestGetCatalogs) { + auto client_mock = std::make_shared<internal::FlightClientImpl>(); + FlightSqlClient sql_client(client_mock); Review comment: That said, my understanding of this is shaky and maybe there is a good justification for why this works. ########## File path: cpp/src/arrow/flight/flight-sql/client_test.cc ########## @@ -0,0 +1,569 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <arrow/flight/client.h> +#include <arrow/flight/flight-sql/FlightSql.pb.h> +#include <arrow/flight/flight-sql/api.h> +#include <arrow/testing/gtest_util.h> +#include <gmock/gmock.h> +#include <google/protobuf/any.pb.h> +#include <gtest/gtest.h> + +#include <utility> + +namespace pb = arrow::flight::protocol; +using ::testing::_; +using ::testing::Ref; + +namespace arrow { +namespace flight { +namespace sql { + +namespace internal { +class FlightClientImpl { + public: + ~FlightClientImpl() = default; + + MOCK_METHOD(Status, GetFlightInfo, + (const FlightCallOptions&, const FlightDescriptor&, + std::unique_ptr<FlightInfo>*)); + MOCK_METHOD(Status, DoGet, + (const FlightCallOptions& options, const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream)); + MOCK_METHOD(Status, DoPut, + (const FlightCallOptions&, const FlightDescriptor&, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>*, + std::unique_ptr<FlightMetadataReader>*)); + MOCK_METHOD(Status, DoAction, + (const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results)); +}; + +Status FlightClientImpl_GetFlightInfo(FlightClientImpl* client, + const FlightCallOptions& options, + const FlightDescriptor& descriptor, + std::unique_ptr<FlightInfo>* info) { + return client->GetFlightInfo(options, descriptor, info); +} + +Status FlightClientImpl_DoPut(FlightClientImpl* client, const FlightCallOptions& options, + const FlightDescriptor& descriptor, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>* stream, + std::unique_ptr<FlightMetadataReader>* reader) { + return client->DoPut(options, descriptor, schema, stream, reader); +} + +Status FlightClientImpl_DoGet(FlightClientImpl* client, const FlightCallOptions& options, + const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream) { + return client->DoGet(options, ticket, stream); +} + +Status FlightClientImpl_DoAction(FlightClientImpl* client, + const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results) { + return client->DoAction(options, action, results); +} + +} // namespace internal + +class FlightMetadataReaderMock : public FlightMetadataReader { + public: + std::shared_ptr<Buffer>* buffer; + + explicit FlightMetadataReaderMock(std::shared_ptr<Buffer>* buffer) { + this->buffer = buffer; + } + + Status ReadMetadata(std::shared_ptr<Buffer>* out) override { + *out = *buffer; + return Status::OK(); + } +}; + +class FlightStreamWriterMock : public FlightStreamWriter { + public: + FlightStreamWriterMock() = default; + + Status DoneWriting() override { return Status::OK(); } + + Status WriteMetadata(std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema, + const ipc::IpcWriteOptions& options) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema) override { + return MetadataRecordBatchWriter::Begin(schema); + } + + ipc::WriteStats stats() const override { return ipc::WriteStats(); } + + Status WriteWithMetadata(const RecordBatch& batch, + std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Close() override { return Status::OK(); } + + Status WriteRecordBatch(const RecordBatch& batch) override { return Status::OK(); } +}; + +FlightDescriptor getDescriptor(google::protobuf::Message& command) { + google::protobuf::Any any; + any.PackFrom(command); + + const std::string& string = any.SerializeAsString(); + return FlightDescriptor::Command(string); +} + +TEST(TestFlightSqlClient, TestGetCatalogs) { + auto client_mock = std::make_shared<internal::FlightClientImpl>(); + FlightSqlClient sql_client(client_mock); Review comment: Both libarrow_flight_sql.so and client_test.cc.o have symbols for FlightClientImpl for me. That said, they're marked as weak symbols, and presumably the one in the program takes precedence. (Confirmed with LD_DEBUG.) This works and I'm not going to hold this up, however, it is rather magical and it's worth at least a comment explaining the intent of FlightClientImpl. ########## File path: cpp/src/arrow/flight/flight-sql/client_test.cc ########## @@ -0,0 +1,569 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <arrow/flight/client.h> +#include <arrow/flight/flight-sql/FlightSql.pb.h> +#include <arrow/flight/flight-sql/api.h> +#include <arrow/testing/gtest_util.h> +#include <gmock/gmock.h> +#include <google/protobuf/any.pb.h> +#include <gtest/gtest.h> + +#include <utility> + +namespace pb = arrow::flight::protocol; +using ::testing::_; +using ::testing::Ref; + +namespace arrow { +namespace flight { +namespace sql { + +namespace internal { +class FlightClientImpl { + public: + ~FlightClientImpl() = default; + + MOCK_METHOD(Status, GetFlightInfo, + (const FlightCallOptions&, const FlightDescriptor&, + std::unique_ptr<FlightInfo>*)); + MOCK_METHOD(Status, DoGet, + (const FlightCallOptions& options, const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream)); + MOCK_METHOD(Status, DoPut, + (const FlightCallOptions&, const FlightDescriptor&, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>*, + std::unique_ptr<FlightMetadataReader>*)); + MOCK_METHOD(Status, DoAction, + (const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results)); +}; + +Status FlightClientImpl_GetFlightInfo(FlightClientImpl* client, + const FlightCallOptions& options, + const FlightDescriptor& descriptor, + std::unique_ptr<FlightInfo>* info) { + return client->GetFlightInfo(options, descriptor, info); +} + +Status FlightClientImpl_DoPut(FlightClientImpl* client, const FlightCallOptions& options, + const FlightDescriptor& descriptor, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>* stream, + std::unique_ptr<FlightMetadataReader>* reader) { + return client->DoPut(options, descriptor, schema, stream, reader); +} + +Status FlightClientImpl_DoGet(FlightClientImpl* client, const FlightCallOptions& options, + const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream) { + return client->DoGet(options, ticket, stream); +} + +Status FlightClientImpl_DoAction(FlightClientImpl* client, + const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results) { + return client->DoAction(options, action, results); +} + +} // namespace internal + +class FlightMetadataReaderMock : public FlightMetadataReader { + public: + std::shared_ptr<Buffer>* buffer; + + explicit FlightMetadataReaderMock(std::shared_ptr<Buffer>* buffer) { + this->buffer = buffer; + } + + Status ReadMetadata(std::shared_ptr<Buffer>* out) override { + *out = *buffer; + return Status::OK(); + } +}; + +class FlightStreamWriterMock : public FlightStreamWriter { + public: + FlightStreamWriterMock() = default; + + Status DoneWriting() override { return Status::OK(); } + + Status WriteMetadata(std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema, + const ipc::IpcWriteOptions& options) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema) override { + return MetadataRecordBatchWriter::Begin(schema); + } + + ipc::WriteStats stats() const override { return ipc::WriteStats(); } + + Status WriteWithMetadata(const RecordBatch& batch, + std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Close() override { return Status::OK(); } + + Status WriteRecordBatch(const RecordBatch& batch) override { return Status::OK(); } +}; + +FlightDescriptor getDescriptor(google::protobuf::Message& command) { + google::protobuf::Any any; + any.PackFrom(command); + + const std::string& string = any.SerializeAsString(); + return FlightDescriptor::Command(string); +} + +TEST(TestFlightSqlClient, TestGetCatalogs) { + auto client_mock = std::make_shared<internal::FlightClientImpl>(); + FlightSqlClient sql_client(client_mock); Review comment: Both libarrow_flight_sql.so and client_test.cc.o have symbols for FlightClientImpl for me. That said, they're marked as weak symbols, and presumably the one in the program takes precedence. (Confirmed with LD_DEBUG.) <details> ``` > nm -C src/arrow/flight/flight_sql/CMakeFiles/arrow-flight-sql-client-test.dir/client_test.cc.o | rg FlightClientImpl::DoPut 0000000000000000 W arrow::flight::sql::internal::FlightClientImpl::DoPut(arrow::flight::FlightCallOptions const&, arrow::flight::FlightDescriptor const&, std::shared_ptr<arrow::Schema> const&, std::unique_ptr<arrow::flight::FlightStreamWriter, std::default_delete<arrow::flight::FlightStreamWriter> >*, std::unique_ptr<arrow::flight::FlightMetadataReader, std::default_delete<arrow::flight::FlightMetadataReader> >*) > nm -C debug/libarrow_flight_sql.so | rg FlightClientImpl::DoPut 00000000000f1dc0 W arrow::flight::sql::internal::FlightClientImpl::DoPut(arrow::flight::FlightCallOptions const&, arrow::flight::FlightDescriptor const&, std::shared_ptr<arrow::Schema> const&, std::unique_ptr<arrow::flight::FlightStreamWriter, std::default_delete<arrow::flight::FlightStreamWriter> >*, std::unique_ptr<arrow::flight::FlightMetadataReader, std::default_delete<arrow::flight::FlightMetadataReader> >*) > nm -C debug/arrow-flight-sql-client-test | rg FlightClientImpl::DoPut 000000000055b320 W arrow::flight::sql::internal::FlightClientImpl::DoPut(arrow::flight::FlightCallOptions const&, arrow::flight::FlightDescriptor const&, std::shared_ptr<arrow::Schema> const&, std::unique_ptr<arrow::flight::FlightStreamWriter, std::default_delete<arrow::flight::FlightStreamWriter> >*, std::unique_ptr<arrow::flight::FlightMetadataReader, std::default_delete<arrow::flight::FlightMetadataReader> >*) ``` </details> This works and I'm not going to hold this up, however, it is rather magical and it's worth at least a comment explaining the intent of FlightClientImpl. ########## File path: cpp/src/arrow/flight/flight-sql/client_test.cc ########## @@ -0,0 +1,569 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <arrow/flight/client.h> +#include <arrow/flight/flight-sql/FlightSql.pb.h> +#include <arrow/flight/flight-sql/api.h> +#include <arrow/testing/gtest_util.h> +#include <gmock/gmock.h> +#include <google/protobuf/any.pb.h> +#include <gtest/gtest.h> + +#include <utility> + +namespace pb = arrow::flight::protocol; +using ::testing::_; +using ::testing::Ref; + +namespace arrow { +namespace flight { +namespace sql { + +namespace internal { +class FlightClientImpl { + public: + ~FlightClientImpl() = default; + + MOCK_METHOD(Status, GetFlightInfo, + (const FlightCallOptions&, const FlightDescriptor&, + std::unique_ptr<FlightInfo>*)); + MOCK_METHOD(Status, DoGet, + (const FlightCallOptions& options, const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream)); + MOCK_METHOD(Status, DoPut, + (const FlightCallOptions&, const FlightDescriptor&, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>*, + std::unique_ptr<FlightMetadataReader>*)); + MOCK_METHOD(Status, DoAction, + (const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results)); +}; + +Status FlightClientImpl_GetFlightInfo(FlightClientImpl* client, + const FlightCallOptions& options, + const FlightDescriptor& descriptor, + std::unique_ptr<FlightInfo>* info) { + return client->GetFlightInfo(options, descriptor, info); +} + +Status FlightClientImpl_DoPut(FlightClientImpl* client, const FlightCallOptions& options, + const FlightDescriptor& descriptor, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>* stream, + std::unique_ptr<FlightMetadataReader>* reader) { + return client->DoPut(options, descriptor, schema, stream, reader); +} + +Status FlightClientImpl_DoGet(FlightClientImpl* client, const FlightCallOptions& options, + const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream) { + return client->DoGet(options, ticket, stream); +} + +Status FlightClientImpl_DoAction(FlightClientImpl* client, + const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results) { + return client->DoAction(options, action, results); +} + +} // namespace internal + +class FlightMetadataReaderMock : public FlightMetadataReader { + public: + std::shared_ptr<Buffer>* buffer; + + explicit FlightMetadataReaderMock(std::shared_ptr<Buffer>* buffer) { + this->buffer = buffer; + } + + Status ReadMetadata(std::shared_ptr<Buffer>* out) override { + *out = *buffer; + return Status::OK(); + } +}; + +class FlightStreamWriterMock : public FlightStreamWriter { + public: + FlightStreamWriterMock() = default; + + Status DoneWriting() override { return Status::OK(); } + + Status WriteMetadata(std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema, + const ipc::IpcWriteOptions& options) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema) override { + return MetadataRecordBatchWriter::Begin(schema); + } + + ipc::WriteStats stats() const override { return ipc::WriteStats(); } + + Status WriteWithMetadata(const RecordBatch& batch, + std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Close() override { return Status::OK(); } + + Status WriteRecordBatch(const RecordBatch& batch) override { return Status::OK(); } +}; + +FlightDescriptor getDescriptor(google::protobuf::Message& command) { + google::protobuf::Any any; + any.PackFrom(command); + + const std::string& string = any.SerializeAsString(); + return FlightDescriptor::Command(string); +} + +TEST(TestFlightSqlClient, TestGetCatalogs) { + auto client_mock = std::make_shared<internal::FlightClientImpl>(); + FlightSqlClient sql_client(client_mock); Review comment: Er, sorry for the churn @rafael-telles but I think the latest commit is strictly worse. I think the situation is OK as-is so long as it's clearly documented (and I can't claim to be a linker expert or C++ standard expert so maybe this is all OK and I don't understand the reason why). I'm mostly just concerned that it's unclear what the intent is. ########## File path: cpp/src/arrow/flight/flight-sql/client_test.cc ########## @@ -0,0 +1,569 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <arrow/flight/client.h> +#include <arrow/flight/flight-sql/FlightSql.pb.h> +#include <arrow/flight/flight-sql/api.h> +#include <arrow/testing/gtest_util.h> +#include <gmock/gmock.h> +#include <google/protobuf/any.pb.h> +#include <gtest/gtest.h> + +#include <utility> + +namespace pb = arrow::flight::protocol; +using ::testing::_; +using ::testing::Ref; + +namespace arrow { +namespace flight { +namespace sql { + +namespace internal { +class FlightClientImpl { + public: + ~FlightClientImpl() = default; + + MOCK_METHOD(Status, GetFlightInfo, + (const FlightCallOptions&, const FlightDescriptor&, + std::unique_ptr<FlightInfo>*)); + MOCK_METHOD(Status, DoGet, + (const FlightCallOptions& options, const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream)); + MOCK_METHOD(Status, DoPut, + (const FlightCallOptions&, const FlightDescriptor&, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>*, + std::unique_ptr<FlightMetadataReader>*)); + MOCK_METHOD(Status, DoAction, + (const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results)); +}; + +Status FlightClientImpl_GetFlightInfo(FlightClientImpl* client, + const FlightCallOptions& options, + const FlightDescriptor& descriptor, + std::unique_ptr<FlightInfo>* info) { + return client->GetFlightInfo(options, descriptor, info); +} + +Status FlightClientImpl_DoPut(FlightClientImpl* client, const FlightCallOptions& options, + const FlightDescriptor& descriptor, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>* stream, + std::unique_ptr<FlightMetadataReader>* reader) { + return client->DoPut(options, descriptor, schema, stream, reader); +} + +Status FlightClientImpl_DoGet(FlightClientImpl* client, const FlightCallOptions& options, + const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream) { + return client->DoGet(options, ticket, stream); +} + +Status FlightClientImpl_DoAction(FlightClientImpl* client, + const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results) { + return client->DoAction(options, action, results); +} + +} // namespace internal + +class FlightMetadataReaderMock : public FlightMetadataReader { + public: + std::shared_ptr<Buffer>* buffer; + + explicit FlightMetadataReaderMock(std::shared_ptr<Buffer>* buffer) { + this->buffer = buffer; + } + + Status ReadMetadata(std::shared_ptr<Buffer>* out) override { + *out = *buffer; + return Status::OK(); + } +}; + +class FlightStreamWriterMock : public FlightStreamWriter { + public: + FlightStreamWriterMock() = default; + + Status DoneWriting() override { return Status::OK(); } + + Status WriteMetadata(std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema, + const ipc::IpcWriteOptions& options) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema) override { + return MetadataRecordBatchWriter::Begin(schema); + } + + ipc::WriteStats stats() const override { return ipc::WriteStats(); } + + Status WriteWithMetadata(const RecordBatch& batch, + std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Close() override { return Status::OK(); } + + Status WriteRecordBatch(const RecordBatch& batch) override { return Status::OK(); } +}; + +FlightDescriptor getDescriptor(google::protobuf::Message& command) { + google::protobuf::Any any; + any.PackFrom(command); + + const std::string& string = any.SerializeAsString(); + return FlightDescriptor::Command(string); +} + +TEST(TestFlightSqlClient, TestGetCatalogs) { + auto client_mock = std::make_shared<internal::FlightClientImpl>(); + FlightSqlClient sql_client(client_mock); Review comment: Perhaps, but even then, I would say that an implementation that requires very carefully linking objects together to be correct is not one we should go with. ########## File path: cpp/src/arrow/flight/flight-sql/client_test.cc ########## @@ -0,0 +1,569 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <arrow/flight/client.h> +#include <arrow/flight/flight-sql/FlightSql.pb.h> +#include <arrow/flight/flight-sql/api.h> +#include <arrow/testing/gtest_util.h> +#include <gmock/gmock.h> +#include <google/protobuf/any.pb.h> +#include <gtest/gtest.h> + +#include <utility> + +namespace pb = arrow::flight::protocol; +using ::testing::_; +using ::testing::Ref; + +namespace arrow { +namespace flight { +namespace sql { + +namespace internal { +class FlightClientImpl { + public: + ~FlightClientImpl() = default; + + MOCK_METHOD(Status, GetFlightInfo, + (const FlightCallOptions&, const FlightDescriptor&, + std::unique_ptr<FlightInfo>*)); + MOCK_METHOD(Status, DoGet, + (const FlightCallOptions& options, const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream)); + MOCK_METHOD(Status, DoPut, + (const FlightCallOptions&, const FlightDescriptor&, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>*, + std::unique_ptr<FlightMetadataReader>*)); + MOCK_METHOD(Status, DoAction, + (const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results)); +}; + +Status FlightClientImpl_GetFlightInfo(FlightClientImpl* client, + const FlightCallOptions& options, + const FlightDescriptor& descriptor, + std::unique_ptr<FlightInfo>* info) { + return client->GetFlightInfo(options, descriptor, info); +} + +Status FlightClientImpl_DoPut(FlightClientImpl* client, const FlightCallOptions& options, + const FlightDescriptor& descriptor, + const std::shared_ptr<Schema>& schema, + std::unique_ptr<FlightStreamWriter>* stream, + std::unique_ptr<FlightMetadataReader>* reader) { + return client->DoPut(options, descriptor, schema, stream, reader); +} + +Status FlightClientImpl_DoGet(FlightClientImpl* client, const FlightCallOptions& options, + const Ticket& ticket, + std::unique_ptr<FlightStreamReader>* stream) { + return client->DoGet(options, ticket, stream); +} + +Status FlightClientImpl_DoAction(FlightClientImpl* client, + const FlightCallOptions& options, const Action& action, + std::unique_ptr<ResultStream>* results) { + return client->DoAction(options, action, results); +} + +} // namespace internal + +class FlightMetadataReaderMock : public FlightMetadataReader { + public: + std::shared_ptr<Buffer>* buffer; + + explicit FlightMetadataReaderMock(std::shared_ptr<Buffer>* buffer) { + this->buffer = buffer; + } + + Status ReadMetadata(std::shared_ptr<Buffer>* out) override { + *out = *buffer; + return Status::OK(); + } +}; + +class FlightStreamWriterMock : public FlightStreamWriter { + public: + FlightStreamWriterMock() = default; + + Status DoneWriting() override { return Status::OK(); } + + Status WriteMetadata(std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema, + const ipc::IpcWriteOptions& options) override { + return Status::OK(); + } + + Status Begin(const std::shared_ptr<Schema>& schema) override { + return MetadataRecordBatchWriter::Begin(schema); + } + + ipc::WriteStats stats() const override { return ipc::WriteStats(); } + + Status WriteWithMetadata(const RecordBatch& batch, + std::shared_ptr<Buffer> app_metadata) override { + return Status::OK(); + } + + Status Close() override { return Status::OK(); } + + Status WriteRecordBatch(const RecordBatch& batch) override { return Status::OK(); } +}; + +FlightDescriptor getDescriptor(google::protobuf::Message& command) { + google::protobuf::Any any; + any.PackFrom(command); + + const std::string& string = any.SerializeAsString(); + return FlightDescriptor::Command(string); +} + +TEST(TestFlightSqlClient, TestGetCatalogs) { + auto client_mock = std::make_shared<internal::FlightClientImpl>(); + FlightSqlClient sql_client(client_mock); Review comment: (Sorry - I want to try some alternative approaches but haven't found the time. At this rate it may be Friday before I can get into things…) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
