pitrou commented on code in PR #13492: URL: https://github.com/apache/arrow/pull/13492#discussion_r940264932
########## cpp/src/arrow/flight/sql/acero_test.cc: ########## @@ -0,0 +1,220 @@ +// 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. + +/// Integration test using the Acero backend + +#include <memory> +#include <sstream> + +#include <gmock/gmock.h> +#include <gtest/gtest.h> + +#include "arrow/array.h" +#include "arrow/engine/substrait/util.h" +#include "arrow/flight/server.h" +#include "arrow/flight/sql/client.h" +#include "arrow/flight/sql/example/acero_server.h" +#include "arrow/flight/sql/types.h" +#include "arrow/flight/types.h" +#include "arrow/stl_iterator.h" +#include "arrow/table.h" +#include "arrow/testing/gtest_util.h" +#include "arrow/type_fwd.h" + +namespace arrow { +namespace flight { +namespace sql { + +class TestAcero : public ::testing::Test { + public: + void SetUp() override { +#ifdef _WIN32 + GTEST_SKIP() << "ARROW-16392: Substrait File URI not supported for Windows"; +#endif + + ASSERT_OK_AND_ASSIGN(auto location, Location::ForGrpcTcp("0.0.0.0", 0)); + flight::FlightServerOptions options(location); + + ASSERT_OK_AND_ASSIGN(server_, acero_example::MakeServer()); + ASSERT_OK(server_->Init(options)); + + std::stringstream ss; + ss << "grpc://localhost:" << server_->port(); + + ASSERT_OK_AND_ASSIGN(auto client_location, Location::Parse(ss.str())); + ASSERT_OK_AND_ASSIGN(auto client, FlightClient::Connect(client_location)); + + client_.reset(new FlightSqlClient(std::move(client))); + } + + void TearDown() override { + ASSERT_OK(client_->Close()); + ASSERT_OK(server_->Shutdown()); + } + + protected: + std::unique_ptr<FlightSqlClient> client_; + std::unique_ptr<FlightServerBase> server_; +}; + +arrow::Result<std::shared_ptr<Buffer>> MakeSubstraitPlan() { + ARROW_ASSIGN_OR_RAISE(std::string dir_string, + arrow::internal::GetEnvVar("PARQUET_TEST_DATA")); + ARROW_ASSIGN_OR_RAISE(auto dir, + arrow::internal::PlatformFilename::FromString(dir_string)); + ARROW_ASSIGN_OR_RAISE(auto filename, dir.Join("binary.parquet")); + + // TODO(ARROW-17229): we should use a RootRel here + std::string json_plan = R"({ + "relations": [ + { + "rel": { + "read": { + "base_schema": { + "struct": { + "types": [ + {"binary": {}} + ] + }, + "names": [ + "foo" + ] + }, + "local_files": { + "items": [ + { + "uri_file": "file://FILENAME_PLACEHOLDER", Review Comment: Should perhaps use a URI placeholder and produce the proper URI programmatically? ```suggestion "uri_file": "URI_PLACEHOLDER", ``` ########## cpp/src/arrow/flight/sql/acero_test.cc: ########## @@ -0,0 +1,220 @@ +// 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. + +/// Integration test using the Acero backend + +#include <memory> +#include <sstream> + +#include <gmock/gmock.h> +#include <gtest/gtest.h> + +#include "arrow/array.h" +#include "arrow/engine/substrait/util.h" +#include "arrow/flight/server.h" +#include "arrow/flight/sql/client.h" +#include "arrow/flight/sql/example/acero_server.h" +#include "arrow/flight/sql/types.h" +#include "arrow/flight/types.h" +#include "arrow/stl_iterator.h" +#include "arrow/table.h" +#include "arrow/testing/gtest_util.h" +#include "arrow/type_fwd.h" + +namespace arrow { +namespace flight { +namespace sql { + +class TestAcero : public ::testing::Test { + public: + void SetUp() override { +#ifdef _WIN32 + GTEST_SKIP() << "ARROW-16392: Substrait File URI not supported for Windows"; +#endif + + ASSERT_OK_AND_ASSIGN(auto location, Location::ForGrpcTcp("0.0.0.0", 0)); + flight::FlightServerOptions options(location); + + ASSERT_OK_AND_ASSIGN(server_, acero_example::MakeServer()); + ASSERT_OK(server_->Init(options)); + + std::stringstream ss; + ss << "grpc://localhost:" << server_->port(); + + ASSERT_OK_AND_ASSIGN(auto client_location, Location::Parse(ss.str())); + ASSERT_OK_AND_ASSIGN(auto client, FlightClient::Connect(client_location)); + + client_.reset(new FlightSqlClient(std::move(client))); + } + + void TearDown() override { + ASSERT_OK(client_->Close()); + ASSERT_OK(server_->Shutdown()); + } + + protected: + std::unique_ptr<FlightSqlClient> client_; + std::unique_ptr<FlightServerBase> server_; +}; + +arrow::Result<std::shared_ptr<Buffer>> MakeSubstraitPlan() { + ARROW_ASSIGN_OR_RAISE(std::string dir_string, + arrow::internal::GetEnvVar("PARQUET_TEST_DATA")); + ARROW_ASSIGN_OR_RAISE(auto dir, + arrow::internal::PlatformFilename::FromString(dir_string)); + ARROW_ASSIGN_OR_RAISE(auto filename, dir.Join("binary.parquet")); + + // TODO(ARROW-17229): we should use a RootRel here + std::string json_plan = R"({ + "relations": [ + { + "rel": { + "read": { + "base_schema": { + "struct": { + "types": [ + {"binary": {}} + ] + }, + "names": [ + "foo" + ] + }, + "local_files": { + "items": [ + { + "uri_file": "file://FILENAME_PLACEHOLDER", + "parquet": {} + } + ] + } + } + } + } + ] +})"; + std::string filename_placeholder = "FILENAME_PLACEHOLDER"; + json_plan.replace(json_plan.find(filename_placeholder), filename_placeholder.size(), + filename.ToString()); + return engine::substrait::SerializeJsonPlan(json_plan); +} + +TEST_F(TestAcero, GetSqlInfo) { + ASSERT_OK_AND_ASSIGN( + auto flight_info, + client_->GetSqlInfo({}, { + SqlInfoOptions::SqlInfo::FLIGHT_SQL_SERVER_SUBSTRAIT, + SqlInfoOptions::SqlInfo::FLIGHT_SQL_SERVER_TRANSACTION, + })); + ASSERT_OK_AND_ASSIGN(auto reader, + client_->DoGet({}, flight_info->endpoints()[0].ticket)); + ASSERT_OK_AND_ASSIGN(auto results, reader->ToTable()); + ASSERT_OK_AND_ASSIGN(auto batch, results->CombineChunksToBatch()); + ASSERT_EQ(2, results->num_rows()); + std::vector<std::pair<uint32_t, SqlInfoResult>> info; + const UInt32Array& ids = static_cast<const UInt32Array&>(*batch->column(0)); + const DenseUnionArray& values = static_cast<const DenseUnionArray&>(*batch->column(1)); Review Comment: Use `checked_cast`? ########## format/FlightSql.proto: ########## @@ -90,6 +90,55 @@ enum SqlInfo { */ FLIGHT_SQL_SERVER_READ_ONLY = 3; + /* + * Retrieves a boolean value indicating whether the Flight SQL Server supports executing + * Substrait plans. + */ + FLIGHT_SQL_SERVER_SUBSTRAIT = 4; + + /* + * Retrieves a string value indicating the minimum supported Substrait version, or null + * if Substrait is not supported. + */ + FLIGHT_SQL_SERVER_SUBSTRAIT_MIN_VERSION = 5; + + /* + * Retrieves a string value indicating the maximum supported Substrait version, or null + * if Substrait is not supported. + */ + FLIGHT_SQL_SERVER_SUBSTRAIT_MAX_VERSION = 6; + + /* + * Retrieves an int32 indicating whether the Flight SQL Server supports the + * BeginTransaction/EndTransaction/BeginSavepoint/EndSavepoint actions. + * + * Even if this is not supported, the database may still support explicit "BEGIN + * TRANSACTION"/"COMMIT" SQL statements (see SQL_TRANSACTIONS_SUPPORTED); this property + * is only about whether the server implements the Flight SQL API endpoints. + * + * The possible values are listed in `SqlSupportedTransaction`. + */ + FLIGHT_SQL_SERVER_TRANSACTION = 7; + + /* + * Retrieves a boolean value indicating whether the Flight SQL Server supports explicit + * query cancellation (the CancelQuery action). + */ + FLIGHT_SQL_SERVER_CANCEL = 8; + + /* + * Retrieves an int32 indicating the timeout (in milliseconds) for prepared statement handles. + * + * If 0, there is no timeout. Servers should reset the timeout when the handle is used in a command. + */ + FLIGHT_SQL_SERVER_STATEMENT_TIMEOUT = 100; + + /* + * Retrieves an int32 indicating the timeout (in milliseconds) for transactions, since transactions are not tied to a connection. Review Comment: Is it envisioned that servers always have a hard-coded timeout per transaction? Does that match reality? ########## format/FlightSql.proto: ########## @@ -1416,14 +1477,47 @@ message ActionCreatePreparedStatementRequest { Review Comment: Do you plan to remove the "experimental" markers like the above? ########## format/FlightSql.proto: ########## @@ -1416,14 +1477,47 @@ message ActionCreatePreparedStatementRequest { // The valid SQL string to create a prepared statement for. string query = 1; + // Create/execute the prepared statement as part of this transaction (if + // unset, executions of the prepared statement will be auto-committed). + optional bytes transaction_id = 2; +} + +/* + * An embedded message describing a Substrait plan to execute. + */ +message SubstraitPlan { + // The serialized substrait.Plan to create a prepared statement for. + // XXX(ARROW-16902): this is bytes instead of an embedded message + // because Protobuf does not really support one DLL using Protobuf + // definitions from another DLL. + bytes plan = 1; + // The Substrait release, e.g. "0.12.0". This information is not + // tracked in the plan itself, so this is the only way for consumers + // to potentially know if they can handle the plan. + string version = 2; +} + +/* + * Request message for the "CreatePreparedSubstraitPlan" action on a Flight SQL enabled backend. + */ +message ActionCreatePreparedSubstraitPlanRequest { + option (experimental) = true; Review Comment: Is there a particular rationale for adding this option in some message definitions but not all of them? ########## format/FlightSql.proto: ########## @@ -90,6 +90,55 @@ enum SqlInfo { */ FLIGHT_SQL_SERVER_READ_ONLY = 3; + /* + * Retrieves a boolean value indicating whether the Flight SQL Server supports executing + * Substrait plans. + */ + FLIGHT_SQL_SERVER_SUBSTRAIT = 4; + + /* + * Retrieves a string value indicating the minimum supported Substrait version, or null + * if Substrait is not supported. + */ + FLIGHT_SQL_SERVER_SUBSTRAIT_MIN_VERSION = 5; + + /* + * Retrieves a string value indicating the maximum supported Substrait version, or null + * if Substrait is not supported. + */ + FLIGHT_SQL_SERVER_SUBSTRAIT_MAX_VERSION = 6; + + /* + * Retrieves an int32 indicating whether the Flight SQL Server supports the + * BeginTransaction/EndTransaction/BeginSavepoint/EndSavepoint actions. + * + * Even if this is not supported, the database may still support explicit "BEGIN + * TRANSACTION"/"COMMIT" SQL statements (see SQL_TRANSACTIONS_SUPPORTED); this property + * is only about whether the server implements the Flight SQL API endpoints. + * + * The possible values are listed in `SqlSupportedTransaction`. + */ + FLIGHT_SQL_SERVER_TRANSACTION = 7; + + /* + * Retrieves a boolean value indicating whether the Flight SQL Server supports explicit + * query cancellation (the CancelQuery action). + */ + FLIGHT_SQL_SERVER_CANCEL = 8; + + /* + * Retrieves an int32 indicating the timeout (in milliseconds) for prepared statement handles. Review Comment: My 2 cents: milliseconds sounds inflexible and ad hoc. My own preference is to always expose/return seconds represented as a double. ########## cpp/src/arrow/flight/sql/CMakeLists.txt: ########## @@ -61,7 +61,8 @@ add_arrow_lib(arrow_flight_sql STATIC_LINK_LIBS arrow_flight_static PRIVATE_INCLUDES - "${Protobuf_INCLUDE_DIRS}") + "${Protobuf_INCLUDE_DIRS}" + "${CMAKE_CURRENT_BINARY_DIR}/../") Review Comment: Hmm... what is this for? ########## format/FlightSql.proto: ########## @@ -90,6 +90,55 @@ enum SqlInfo { */ FLIGHT_SQL_SERVER_READ_ONLY = 3; + /* + * Retrieves a boolean value indicating whether the Flight SQL Server supports executing + * Substrait plans. + */ + FLIGHT_SQL_SERVER_SUBSTRAIT = 4; + + /* + * Retrieves a string value indicating the minimum supported Substrait version, or null + * if Substrait is not supported. + */ + FLIGHT_SQL_SERVER_SUBSTRAIT_MIN_VERSION = 5; + + /* + * Retrieves a string value indicating the maximum supported Substrait version, or null + * if Substrait is not supported. Review Comment: Hmm... does that mean Substrait N+1 is not planned to be backwards compatible with Substrait N? Implementors must explicitly handle every supported version separately? ########## format/FlightSql.proto: ########## @@ -1550,6 +1780,57 @@ message DoPutUpdateResult { int64 record_count = 1; } +/* + * Request message for the "CancelQuery" action. + * + * Explicitly cancel a running query. + * + * This lets a single client explicitly cancel work, no matter how many clients + * are involved/whether the query is distributed or not, given server support. + * The transaction/statement is not rolled back; it is the application's job to + * commit or rollback as appropriate. This only indicates the client no longer + * wishes to read the remainder of the query results or continue submitting + * data. + * + * This command is idempotent. + */ +message ActionCancelQueryRequest { + option (experimental) = true; + + // The result of the GetFlightInfo RPC that initated the query. + // XXX(ARROW-16902): this must be a serialized FlightInfo, but is + // rendered as bytes because Protobuf does not really support one + // DLL using Protobuf definitions from another DLL. + bytes info = 1; +} + +/* + * The result of cancelling a query. + * + * The result should be wrapped in a google.protobuf.Any message. + */ +message ActionCancelQueryResult { + option (experimental) = true; + + enum CancelResult { + // The cancellation status is unknown. Servers should avoid using + // this value (send a NOT_FOUND error if the requested query is + // not known). Clients can retry the request. + CANCEL_RESULT_UNSPECIFIED = 0; + // The cancellation request is complete. Subsequent requests with + // the same payload may return CANCELLED or a NOT_FOUND error. + CANCEL_RESULT_CANCELLED = 1; + // The cancellation request is in progress. The client may retry + // the request. + CANCEL_RESULT_CANCELLING = 2; + // The query is not cancellable. The client should not retry the + // request. Review Comment: ```suggestion // cancellation request. ``` ########## cpp/src/arrow/flight/sql/acero_test.cc: ########## @@ -0,0 +1,220 @@ +// 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. + +/// Integration test using the Acero backend + +#include <memory> +#include <sstream> + +#include <gmock/gmock.h> +#include <gtest/gtest.h> + +#include "arrow/array.h" +#include "arrow/engine/substrait/util.h" +#include "arrow/flight/server.h" +#include "arrow/flight/sql/client.h" +#include "arrow/flight/sql/example/acero_server.h" +#include "arrow/flight/sql/types.h" +#include "arrow/flight/types.h" +#include "arrow/stl_iterator.h" +#include "arrow/table.h" +#include "arrow/testing/gtest_util.h" +#include "arrow/type_fwd.h" + +namespace arrow { +namespace flight { +namespace sql { + +class TestAcero : public ::testing::Test { + public: + void SetUp() override { +#ifdef _WIN32 + GTEST_SKIP() << "ARROW-16392: Substrait File URI not supported for Windows"; +#endif + + ASSERT_OK_AND_ASSIGN(auto location, Location::ForGrpcTcp("0.0.0.0", 0)); + flight::FlightServerOptions options(location); + + ASSERT_OK_AND_ASSIGN(server_, acero_example::MakeServer()); + ASSERT_OK(server_->Init(options)); + + std::stringstream ss; + ss << "grpc://localhost:" << server_->port(); Review Comment: Perhaps expose a `server_->connect_uri()` instead? ########## format/FlightSql.proto: ########## @@ -1550,6 +1780,57 @@ message DoPutUpdateResult { int64 record_count = 1; } +/* + * Request message for the "CancelQuery" action. + * + * Explicitly cancel a running query. + * + * This lets a single client explicitly cancel work, no matter how many clients + * are involved/whether the query is distributed or not, given server support. + * The transaction/statement is not rolled back; it is the application's job to + * commit or rollback as appropriate. This only indicates the client no longer + * wishes to read the remainder of the query results or continue submitting + * data. + * + * This command is idempotent. + */ +message ActionCancelQueryRequest { + option (experimental) = true; + + // The result of the GetFlightInfo RPC that initated the query. + // XXX(ARROW-16902): this must be a serialized FlightInfo, but is + // rendered as bytes because Protobuf does not really support one + // DLL using Protobuf definitions from another DLL. + bytes info = 1; +} + +/* + * The result of cancelling a query. + * + * The result should be wrapped in a google.protobuf.Any message. + */ +message ActionCancelQueryResult { + option (experimental) = true; + + enum CancelResult { + // The cancellation status is unknown. Servers should avoid using + // this value (send a NOT_FOUND error if the requested query is + // not known). Clients can retry the request. + CANCEL_RESULT_UNSPECIFIED = 0; + // The cancellation request is complete. Subsequent requests with + // the same payload may return CANCELLED or a NOT_FOUND error. + CANCEL_RESULT_CANCELLED = 1; + // The cancellation request is in progress. The client may retry + // the request. Review Comment: ```suggestion // the cancellation request. ``` ########## format/FlightSql.proto: ########## @@ -90,6 +90,55 @@ enum SqlInfo { */ FLIGHT_SQL_SERVER_READ_ONLY = 3; + /* + * Retrieves a boolean value indicating whether the Flight SQL Server supports executing + * Substrait plans. + */ + FLIGHT_SQL_SERVER_SUBSTRAIT = 4; Review Comment: Should there also be a `FLIGHT_SQL_SERVER_SQL` indicating whether the server supports executing SQL requests, or is that mandatory? ########## cpp/src/arrow/flight/sql/acero_test.cc: ########## @@ -0,0 +1,220 @@ +// 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. + +/// Integration test using the Acero backend + +#include <memory> +#include <sstream> + +#include <gmock/gmock.h> +#include <gtest/gtest.h> + +#include "arrow/array.h" +#include "arrow/engine/substrait/util.h" +#include "arrow/flight/server.h" +#include "arrow/flight/sql/client.h" +#include "arrow/flight/sql/example/acero_server.h" +#include "arrow/flight/sql/types.h" +#include "arrow/flight/types.h" +#include "arrow/stl_iterator.h" +#include "arrow/table.h" +#include "arrow/testing/gtest_util.h" +#include "arrow/type_fwd.h" + +namespace arrow { +namespace flight { +namespace sql { + +class TestAcero : public ::testing::Test { + public: + void SetUp() override { +#ifdef _WIN32 + GTEST_SKIP() << "ARROW-16392: Substrait File URI not supported for Windows"; +#endif + + ASSERT_OK_AND_ASSIGN(auto location, Location::ForGrpcTcp("0.0.0.0", 0)); + flight::FlightServerOptions options(location); + + ASSERT_OK_AND_ASSIGN(server_, acero_example::MakeServer()); + ASSERT_OK(server_->Init(options)); + + std::stringstream ss; + ss << "grpc://localhost:" << server_->port(); + + ASSERT_OK_AND_ASSIGN(auto client_location, Location::Parse(ss.str())); + ASSERT_OK_AND_ASSIGN(auto client, FlightClient::Connect(client_location)); + + client_.reset(new FlightSqlClient(std::move(client))); + } + + void TearDown() override { + ASSERT_OK(client_->Close()); + ASSERT_OK(server_->Shutdown()); + } + + protected: + std::unique_ptr<FlightSqlClient> client_; + std::unique_ptr<FlightServerBase> server_; +}; + +arrow::Result<std::shared_ptr<Buffer>> MakeSubstraitPlan() { + ARROW_ASSIGN_OR_RAISE(std::string dir_string, + arrow::internal::GetEnvVar("PARQUET_TEST_DATA")); + ARROW_ASSIGN_OR_RAISE(auto dir, + arrow::internal::PlatformFilename::FromString(dir_string)); + ARROW_ASSIGN_OR_RAISE(auto filename, dir.Join("binary.parquet")); + + // TODO(ARROW-17229): we should use a RootRel here + std::string json_plan = R"({ + "relations": [ + { + "rel": { + "read": { + "base_schema": { + "struct": { + "types": [ + {"binary": {}} + ] + }, + "names": [ + "foo" + ] + }, + "local_files": { + "items": [ + { + "uri_file": "file://FILENAME_PLACEHOLDER", + "parquet": {} + } + ] + } + } + } + } + ] +})"; + std::string filename_placeholder = "FILENAME_PLACEHOLDER"; + json_plan.replace(json_plan.find(filename_placeholder), filename_placeholder.size(), + filename.ToString()); + return engine::substrait::SerializeJsonPlan(json_plan); +} + +TEST_F(TestAcero, GetSqlInfo) { + ASSERT_OK_AND_ASSIGN( + auto flight_info, + client_->GetSqlInfo({}, { + SqlInfoOptions::SqlInfo::FLIGHT_SQL_SERVER_SUBSTRAIT, + SqlInfoOptions::SqlInfo::FLIGHT_SQL_SERVER_TRANSACTION, + })); + ASSERT_OK_AND_ASSIGN(auto reader, + client_->DoGet({}, flight_info->endpoints()[0].ticket)); Review Comment: Same here (at least for the first one :-)). ########## format/FlightSql.proto: ########## @@ -1550,6 +1780,57 @@ message DoPutUpdateResult { int64 record_count = 1; } +/* + * Request message for the "CancelQuery" action. + * + * Explicitly cancel a running query. + * + * This lets a single client explicitly cancel work, no matter how many clients + * are involved/whether the query is distributed or not, given server support. + * The transaction/statement is not rolled back; it is the application's job to + * commit or rollback as appropriate. This only indicates the client no longer + * wishes to read the remainder of the query results or continue submitting + * data. + * + * This command is idempotent. + */ +message ActionCancelQueryRequest { + option (experimental) = true; + + // The result of the GetFlightInfo RPC that initated the query. Review Comment: ```suggestion // The result of the GetFlightInfo RPC that initiated the query. ``` ########## cpp/src/arrow/flight/sql/acero_test.cc: ########## @@ -0,0 +1,220 @@ +// 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. + +/// Integration test using the Acero backend + +#include <memory> +#include <sstream> + +#include <gmock/gmock.h> +#include <gtest/gtest.h> + +#include "arrow/array.h" +#include "arrow/engine/substrait/util.h" +#include "arrow/flight/server.h" +#include "arrow/flight/sql/client.h" +#include "arrow/flight/sql/example/acero_server.h" +#include "arrow/flight/sql/types.h" +#include "arrow/flight/types.h" +#include "arrow/stl_iterator.h" +#include "arrow/table.h" +#include "arrow/testing/gtest_util.h" +#include "arrow/type_fwd.h" + +namespace arrow { +namespace flight { +namespace sql { + +class TestAcero : public ::testing::Test { + public: + void SetUp() override { +#ifdef _WIN32 + GTEST_SKIP() << "ARROW-16392: Substrait File URI not supported for Windows"; +#endif + + ASSERT_OK_AND_ASSIGN(auto location, Location::ForGrpcTcp("0.0.0.0", 0)); + flight::FlightServerOptions options(location); + + ASSERT_OK_AND_ASSIGN(server_, acero_example::MakeServer()); + ASSERT_OK(server_->Init(options)); + + std::stringstream ss; + ss << "grpc://localhost:" << server_->port(); + + ASSERT_OK_AND_ASSIGN(auto client_location, Location::Parse(ss.str())); + ASSERT_OK_AND_ASSIGN(auto client, FlightClient::Connect(client_location)); + + client_.reset(new FlightSqlClient(std::move(client))); + } + + void TearDown() override { + ASSERT_OK(client_->Close()); + ASSERT_OK(server_->Shutdown()); + } + + protected: + std::unique_ptr<FlightSqlClient> client_; + std::unique_ptr<FlightServerBase> server_; +}; + +arrow::Result<std::shared_ptr<Buffer>> MakeSubstraitPlan() { + ARROW_ASSIGN_OR_RAISE(std::string dir_string, + arrow::internal::GetEnvVar("PARQUET_TEST_DATA")); + ARROW_ASSIGN_OR_RAISE(auto dir, + arrow::internal::PlatformFilename::FromString(dir_string)); + ARROW_ASSIGN_OR_RAISE(auto filename, dir.Join("binary.parquet")); + + // TODO(ARROW-17229): we should use a RootRel here + std::string json_plan = R"({ + "relations": [ + { + "rel": { + "read": { + "base_schema": { + "struct": { + "types": [ + {"binary": {}} + ] + }, + "names": [ + "foo" + ] + }, + "local_files": { + "items": [ + { + "uri_file": "file://FILENAME_PLACEHOLDER", + "parquet": {} + } + ] + } + } + } + } + ] +})"; + std::string filename_placeholder = "FILENAME_PLACEHOLDER"; + json_plan.replace(json_plan.find(filename_placeholder), filename_placeholder.size(), + filename.ToString()); + return engine::substrait::SerializeJsonPlan(json_plan); +} + +TEST_F(TestAcero, GetSqlInfo) { + ASSERT_OK_AND_ASSIGN( + auto flight_info, + client_->GetSqlInfo({}, { Review Comment: Can you perhaps make parameters more explicit? ```suggestion client_->GetSqlInfo(/*abc=*/{}, /*def=*/{ ``` -- 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]
