From 5823b75a5883bd14f29953ae59e1bbea6dc99bd4 Mon Sep 17 00:00:00 2001 From: David Li Date: Tue, 6 Sep 2022 11:13:46 -0400 Subject: [PATCH] [C++][Format] Address review comments --- .../integration_tests/test_integration.cc | 6 ++ cpp/src/arrow/flight/sql/CMakeLists.txt | 3 +- cpp/src/arrow/flight/sql/acero_test.cc | 55 +++++++++++-------- .../flight/sql/example/sqlite_sql_info.cc | 4 +- cpp/src/arrow/flight/sql/types.h | 18 ++++-- format/FlightSql.proto | 21 +++++-- .../tests/FlightSqlExtensionScenario.java | 2 + .../tests/FlightSqlScenarioProducer.java | 1 + .../arrow/flight/sql/SqlInfoBuilder.java | 5 ++ .../flight/sql/example/FlightSqlExample.java | 3 + 10 files changed, 83 insertions(+), 35 deletions(-) diff --git a/cpp/src/arrow/flight/integration_tests/test_integration.cc b/cpp/src/arrow/flight/integration_tests/test_integration.cc index 7e9fd527ccb93..104f1e5a60f96 100644 --- a/cpp/src/arrow/flight/integration_tests/test_integration.cc +++ b/cpp/src/arrow/flight/integration_tests/test_integration.cc @@ -346,6 +346,8 @@ arrow::Status AssertUnprintableEq(const T& expected, const T& actual, class FlightSqlScenarioServer : public sql::FlightSqlServerBase { public: FlightSqlScenarioServer() : sql::FlightSqlServerBase() { + RegisterSqlInfo(sql::SqlInfoOptions::SqlInfo::FLIGHT_SQL_SERVER_SQL, + sql::SqlInfoResult(false)); RegisterSqlInfo(sql::SqlInfoOptions::SqlInfo::FLIGHT_SQL_SERVER_SUBSTRAIT, sql::SqlInfoResult(true)); RegisterSqlInfo(sql::SqlInfoOptions::SqlInfo::FLIGHT_SQL_SERVER_SUBSTRAIT_MIN_VERSION, @@ -1064,6 +1066,7 @@ class FlightSqlExtensionScenario : public FlightSqlScenario { Status ValidateMetadataRetrieval(sql::FlightSqlClient* sql_client) { std::unique_ptr info; std::vector sql_info = { + sql::SqlInfoOptions::FLIGHT_SQL_SERVER_SQL, sql::SqlInfoOptions::FLIGHT_SQL_SERVER_SUBSTRAIT, sql::SqlInfoOptions::FLIGHT_SQL_SERVER_SUBSTRAIT_MIN_VERSION, sql::SqlInfoOptions::FLIGHT_SQL_SERVER_SUBSTRAIT_MAX_VERSION, @@ -1131,6 +1134,9 @@ class FlightSqlExtensionScenario : public FlightSqlScenario { } } + ARROW_RETURN_NOT_OK(AssertUnprintableEq( + info_values[sql::SqlInfoOptions::FLIGHT_SQL_SERVER_SQL], + sql::SqlInfoResult(false), "FLIGHT_SQL_SERVER_SQL did not match")); ARROW_RETURN_NOT_OK(AssertUnprintableEq( info_values[sql::SqlInfoOptions::FLIGHT_SQL_SERVER_SUBSTRAIT], sql::SqlInfoResult(true), "FLIGHT_SQL_SERVER_SUBSTRAIT did not match")); diff --git a/cpp/src/arrow/flight/sql/CMakeLists.txt b/cpp/src/arrow/flight/sql/CMakeLists.txt index 6d5c5eadc7d0c..14503069dd004 100644 --- a/cpp/src/arrow/flight/sql/CMakeLists.txt +++ b/cpp/src/arrow/flight/sql/CMakeLists.txt @@ -61,8 +61,7 @@ add_arrow_lib(arrow_flight_sql STATIC_LINK_LIBS arrow_flight_static PRIVATE_INCLUDES - "${Protobuf_INCLUDE_DIRS}" - "${CMAKE_CURRENT_BINARY_DIR}/../") + "${Protobuf_INCLUDE_DIRS}") if(MSVC) # Suppress warnings caused by Protobuf (casts) diff --git a/cpp/src/arrow/flight/sql/acero_test.cc b/cpp/src/arrow/flight/sql/acero_test.cc index 0699691e5cf48..f99edfaf9d6c1 100644 --- a/cpp/src/arrow/flight/sql/acero_test.cc +++ b/cpp/src/arrow/flight/sql/acero_test.cc @@ -34,11 +34,14 @@ #include "arrow/table.h" #include "arrow/testing/gtest_util.h" #include "arrow/type_fwd.h" +#include "arrow/util/checked_cast.h" namespace arrow { namespace flight { namespace sql { +using arrow::internal::checked_cast; + class TestAcero : public ::testing::Test { public: void SetUp() override { @@ -73,6 +76,7 @@ arrow::Result> MakeSubstraitPlan() { ARROW_ASSIGN_OR_RAISE(auto dir, arrow::internal::PlatformFilename::FromString(dir_string)); ARROW_ASSIGN_OR_RAISE(auto filename, dir.Join("binary.parquet")); + std::string uri = std::string("file://") + filename.ToString(); // TODO(ARROW-17229): we should use a RootRel here std::string json_plan = R"({ @@ -93,7 +97,7 @@ arrow::Result> MakeSubstraitPlan() { "local_files": { "items": [ { - "uri_file": "file://FILENAME_PLACEHOLDER", + "uri_file": "URI_PLACEHOLDER", "parquet": {} } ] @@ -103,35 +107,35 @@ arrow::Result> MakeSubstraitPlan() { } ] })"; - std::string filename_placeholder = "FILENAME_PLACEHOLDER"; - json_plan.replace(json_plan.find(filename_placeholder), filename_placeholder.size(), - filename.ToString()); + std::string uri_placeholder = "URI_PLACEHOLDER"; + json_plan.replace(json_plan.find(uri_placeholder), uri_placeholder.size(), uri); return engine::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, - })); + FlightCallOptions call_options; + std::vector sql_info_codes = { + SqlInfoOptions::SqlInfo::FLIGHT_SQL_SERVER_SUBSTRAIT, + SqlInfoOptions::SqlInfo::FLIGHT_SQL_SERVER_TRANSACTION, + }; + ASSERT_OK_AND_ASSIGN(auto flight_info, + client_->GetSqlInfo(call_options, sql_info_codes)); ASSERT_OK_AND_ASSIGN(auto reader, - client_->DoGet({}, flight_info->endpoints()[0].ticket)); + client_->DoGet(call_options, 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> info; - const UInt32Array& ids = static_cast(*batch->column(0)); - const DenseUnionArray& values = static_cast(*batch->column(1)); + const auto& ids = checked_cast(*batch->column(0)); + const auto& values = checked_cast(*batch->column(1)); for (int64_t i = 0; i < batch->num_rows(); i++) { ASSERT_OK_AND_ASSIGN(auto scalar, values.GetScalar(i)); if (ids.Value(i) == SqlInfoOptions::SqlInfo::FLIGHT_SQL_SERVER_SUBSTRAIT) { - ASSERT_EQ(*static_cast(*scalar).value, + ASSERT_EQ(*checked_cast(*scalar).value, BooleanScalar(true)); } else if (ids.Value(i) == SqlInfoOptions::SqlInfo::FLIGHT_SQL_SERVER_TRANSACTION) { ASSERT_EQ( - *static_cast(*scalar).value, + *checked_cast(*scalar).value, Int32Scalar( SqlInfoOptions::SqlSupportedTransaction::SQL_SUPPORTED_TRANSACTION_NONE)); } else { @@ -145,11 +149,12 @@ TEST_F(TestAcero, Scan) { GTEST_SKIP() << "ARROW-16392: Substrait File URI not supported for Windows"; #endif + FlightCallOptions call_options; ASSERT_OK_AND_ASSIGN(auto serialized_plan, MakeSubstraitPlan()); SubstraitPlan plan{serialized_plan->ToString(), /*version=*/"0.6.0"}; ASSERT_OK_AND_ASSIGN(std::unique_ptr info, - client_->ExecuteSubstrait({}, plan)); + client_->ExecuteSubstrait(call_options, plan)); ipc::DictionaryMemo memo; ASSERT_OK_AND_ASSIGN(auto schema, info->GetSchema(&memo)); // TODO(ARROW-17229): the scanner "special" fields are still included, strip them @@ -160,7 +165,8 @@ TEST_F(TestAcero, Scan) { ASSERT_EQ(1, info->endpoints().size()); ASSERT_EQ(0, info->endpoints()[0].locations.size()); - ASSERT_OK_AND_ASSIGN(auto reader, client_->DoGet({}, info->endpoints()[0].ticket)); + ASSERT_OK_AND_ASSIGN(auto reader, + client_->DoGet(call_options, info->endpoints()[0].ticket)); ASSERT_OK_AND_ASSIGN(auto reader_schema, reader->GetSchema()); ASSERT_NO_FATAL_FAILURE(AssertSchemaEqual(schema, reader_schema)); ASSERT_OK_AND_ASSIGN(auto table, reader->ToTable()); @@ -172,11 +178,12 @@ TEST_F(TestAcero, Update) { GTEST_SKIP() << "ARROW-16392: Substrait File URI not supported for Windows"; #endif + FlightCallOptions call_options; ASSERT_OK_AND_ASSIGN(auto serialized_plan, MakeSubstraitPlan()); SubstraitPlan plan{serialized_plan->ToString(), /*version=*/"0.6.0"}; EXPECT_RAISES_WITH_MESSAGE_THAT(NotImplemented, ::testing::HasSubstr("Updates are unsupported"), - client_->ExecuteSubstraitUpdate({}, plan)); + client_->ExecuteSubstraitUpdate(call_options, plan)); } TEST_F(TestAcero, Prepare) { @@ -184,9 +191,11 @@ TEST_F(TestAcero, Prepare) { GTEST_SKIP() << "ARROW-16392: Substrait File URI not supported for Windows"; #endif + FlightCallOptions call_options; ASSERT_OK_AND_ASSIGN(auto serialized_plan, MakeSubstraitPlan()); SubstraitPlan plan{serialized_plan->ToString(), /*version=*/"0.6.0"}; - ASSERT_OK_AND_ASSIGN(auto prepared_statement, client_->PrepareSubstrait({}, plan)); + ASSERT_OK_AND_ASSIGN(auto prepared_statement, + client_->PrepareSubstrait(call_options, plan)); ASSERT_NE(prepared_statement->dataset_schema(), nullptr); ASSERT_EQ(prepared_statement->parameter_schema(), nullptr); @@ -201,7 +210,8 @@ TEST_F(TestAcero, Prepare) { ASSERT_OK_AND_ASSIGN(std::unique_ptr info, prepared_statement->Execute()); ASSERT_EQ(1, info->endpoints().size()); ASSERT_EQ(0, info->endpoints()[0].locations.size()); - ASSERT_OK_AND_ASSIGN(auto reader, client_->DoGet({}, info->endpoints()[0].ticket)); + ASSERT_OK_AND_ASSIGN(auto reader, + client_->DoGet(call_options, info->endpoints()[0].ticket)); ASSERT_OK_AND_ASSIGN(auto reader_schema, reader->GetSchema()); ASSERT_NO_FATAL_FAILURE( AssertSchemaEqual(prepared_statement->dataset_schema(), reader_schema)); @@ -216,16 +226,17 @@ TEST_F(TestAcero, Transactions) { GTEST_SKIP() << "ARROW-16392: Substrait File URI not supported for Windows"; #endif + FlightCallOptions call_options; ASSERT_OK_AND_ASSIGN(auto serialized_plan, MakeSubstraitPlan()); Transaction handle("fake-id"); SubstraitPlan plan{serialized_plan->ToString(), /*version=*/"0.6.0"}; EXPECT_RAISES_WITH_MESSAGE_THAT(NotImplemented, ::testing::HasSubstr("Transactions are unsupported"), - client_->ExecuteSubstrait({}, plan, handle)); + client_->ExecuteSubstrait(call_options, plan, handle)); EXPECT_RAISES_WITH_MESSAGE_THAT(NotImplemented, ::testing::HasSubstr("Transactions are unsupported"), - client_->PrepareSubstrait({}, plan, handle)); + client_->PrepareSubstrait(call_options, plan, handle)); } } // namespace sql diff --git a/cpp/src/arrow/flight/sql/example/sqlite_sql_info.cc b/cpp/src/arrow/flight/sql/example/sqlite_sql_info.cc index 1c7d83320127a..9737b5a3090d1 100644 --- a/cpp/src/arrow/flight/sql/example/sqlite_sql_info.cc +++ b/cpp/src/arrow/flight/sql/example/sqlite_sql_info.cc @@ -18,6 +18,7 @@ #include "arrow/flight/sql/example/sqlite_sql_info.h" #include "arrow/flight/sql/types.h" +#include "arrow/util/config.h" namespace arrow { namespace flight { @@ -33,8 +34,9 @@ SqlInfoResultMap GetSqlInfoResultMap() { {SqlInfoOptions::SqlInfo::FLIGHT_SQL_SERVER_VERSION, SqlInfoResult(std::string("sqlite 3"))}, {SqlInfoOptions::SqlInfo::FLIGHT_SQL_SERVER_ARROW_VERSION, - SqlInfoResult(std::string("7.0.0-SNAPSHOT" /* Only an example */))}, + SqlInfoResult(std::string(ARROW_VERSION_STRING))}, {SqlInfoOptions::SqlInfo::FLIGHT_SQL_SERVER_READ_ONLY, SqlInfoResult(false)}, + {SqlInfoOptions::SqlInfo::FLIGHT_SQL_SERVER_SQL, SqlInfoResult(true)}, {SqlInfoOptions::SqlInfo::FLIGHT_SQL_SERVER_SUBSTRAIT, SqlInfoResult(false)}, {SqlInfoOptions::SqlInfo::FLIGHT_SQL_SERVER_TRANSACTION, SqlInfoResult(SqlInfoOptions::SqlSupportedTransaction:: diff --git a/cpp/src/arrow/flight/sql/types.h b/cpp/src/arrow/flight/sql/types.h index 0e043822a19bf..817fbff8b6620 100644 --- a/cpp/src/arrow/flight/sql/types.h +++ b/cpp/src/arrow/flight/sql/types.h @@ -71,17 +71,25 @@ struct ARROW_FLIGHT_SQL_EXPORT SqlInfoOptions { /// - true: if read only FLIGHT_SQL_SERVER_READ_ONLY = 3, + /// Retrieves a boolean value indicating whether the Flight SQL Server + /// supports executing SQL queries. + /// + /// Note that the absence of this info (as opposed to a false + /// value) does not necessarily mean that SQL is not supported, as + /// this property was not originally defined. + FLIGHT_SQL_SERVER_SQL = 4, + /// Retrieves a boolean value indicating whether the Flight SQL Server /// supports executing Substrait plans. - FLIGHT_SQL_SERVER_SUBSTRAIT = 4, + FLIGHT_SQL_SERVER_SUBSTRAIT = 5, /// Retrieves a string value indicating the minimum supported /// Substrait version, or null if Substrait is not supported. - FLIGHT_SQL_SERVER_SUBSTRAIT_MIN_VERSION = 5, + FLIGHT_SQL_SERVER_SUBSTRAIT_MIN_VERSION = 6, /// Retrieves a string value indicating the maximum supported /// Substrait version, or null if Substrait is not supported. - FLIGHT_SQL_SERVER_SUBSTRAIT_MAX_VERSION = 6, + FLIGHT_SQL_SERVER_SUBSTRAIT_MAX_VERSION = 7, /// Retrieves an int32 indicating whether the Flight SQL Server /// supports the BeginTransaction, EndTransaction, BeginSavepoint, @@ -93,11 +101,11 @@ struct ARROW_FLIGHT_SQL_EXPORT SqlInfoOptions { /// whether the server implements the Flight SQL API endpoints. /// /// The possible values are listed in `SqlSupportedTransaction`. - FLIGHT_SQL_SERVER_TRANSACTION = 7, + FLIGHT_SQL_SERVER_TRANSACTION = 8, /// Retrieves a boolean value indicating whether the Flight SQL Server /// supports explicit query cancellation (the CancelQuery action). - FLIGHT_SQL_SERVER_CANCEL = 8, + FLIGHT_SQL_SERVER_CANCEL = 9, /// Retrieves an int32 value indicating the timeout (in milliseconds) for /// prepared statement handles. diff --git a/format/FlightSql.proto b/format/FlightSql.proto index ceefff60d74b5..d8a6cb5bfdb07 100644 --- a/format/FlightSql.proto +++ b/format/FlightSql.proto @@ -90,23 +90,32 @@ enum SqlInfo { */ FLIGHT_SQL_SERVER_READ_ONLY = 3; + /* + * Retrieves a boolean value indicating whether the Flight SQL Server supports executing + * SQL queries. + * + * Note that the absence of this info (as opposed to a false value) does not necessarily + * mean that SQL is not supported, as this property was not originally defined. + */ + FLIGHT_SQL_SERVER_SQL = 4; + /* * Retrieves a boolean value indicating whether the Flight SQL Server supports executing * Substrait plans. */ - FLIGHT_SQL_SERVER_SUBSTRAIT = 4; + FLIGHT_SQL_SERVER_SUBSTRAIT = 5; /* * Retrieves a string value indicating the minimum supported Substrait version, or null * if Substrait is not supported. */ - FLIGHT_SQL_SERVER_SUBSTRAIT_MIN_VERSION = 5; + FLIGHT_SQL_SERVER_SUBSTRAIT_MIN_VERSION = 6; /* * Retrieves a string value indicating the maximum supported Substrait version, or null * if Substrait is not supported. */ - FLIGHT_SQL_SERVER_SUBSTRAIT_MAX_VERSION = 6; + FLIGHT_SQL_SERVER_SUBSTRAIT_MAX_VERSION = 7; /* * Retrieves an int32 indicating whether the Flight SQL Server supports the @@ -118,13 +127,13 @@ enum SqlInfo { * * The possible values are listed in `SqlSupportedTransaction`. */ - FLIGHT_SQL_SERVER_TRANSACTION = 7; + FLIGHT_SQL_SERVER_TRANSACTION = 8; /* * Retrieves a boolean value indicating whether the Flight SQL Server supports explicit * query cancellation (the CancelQuery action). */ - FLIGHT_SQL_SERVER_CANCEL = 8; + FLIGHT_SQL_SERVER_CANCEL = 9; /* * Retrieves an int32 indicating the timeout (in milliseconds) for prepared statement handles. @@ -1486,6 +1495,8 @@ message ActionCreatePreparedStatementRequest { * An embedded message describing a Substrait plan to execute. */ message SubstraitPlan { + option (experimental) = true; + // 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 diff --git a/java/flight/flight-integration-tests/src/main/java/org/apache/arrow/flight/integration/tests/FlightSqlExtensionScenario.java b/java/flight/flight-integration-tests/src/main/java/org/apache/arrow/flight/integration/tests/FlightSqlExtensionScenario.java index 4c11fe5031818..cd20ae4f46f1f 100644 --- a/java/flight/flight-integration-tests/src/main/java/org/apache/arrow/flight/integration/tests/FlightSqlExtensionScenario.java +++ b/java/flight/flight-integration-tests/src/main/java/org/apache/arrow/flight/integration/tests/FlightSqlExtensionScenario.java @@ -97,6 +97,8 @@ private void validateMetadataRetrieval(FlightSqlClient sqlClient) throws Excepti } } + IntegrationAssertions.assertEquals(Boolean.FALSE, + infoValues.get(FlightSql.SqlInfo.FLIGHT_SQL_SERVER_SQL_VALUE)); IntegrationAssertions.assertEquals(Boolean.TRUE, infoValues.get(FlightSql.SqlInfo.FLIGHT_SQL_SERVER_SUBSTRAIT_VALUE)); IntegrationAssertions.assertEquals("min_version", diff --git a/java/flight/flight-integration-tests/src/main/java/org/apache/arrow/flight/integration/tests/FlightSqlScenarioProducer.java b/java/flight/flight-integration-tests/src/main/java/org/apache/arrow/flight/integration/tests/FlightSqlScenarioProducer.java index a9fc0e0c88a30..4ed9a3df0fc62 100644 --- a/java/flight/flight-integration-tests/src/main/java/org/apache/arrow/flight/integration/tests/FlightSqlScenarioProducer.java +++ b/java/flight/flight-integration-tests/src/main/java/org/apache/arrow/flight/integration/tests/FlightSqlScenarioProducer.java @@ -498,6 +498,7 @@ public void getStreamSqlInfo(FlightSql.CommandGetSqlInfo command, CallContext co return; } SqlInfoBuilder sqlInfoBuilder = new SqlInfoBuilder() + .withFlightSqlServerSql(false) .withFlightSqlServerSubstrait(true) .withFlightSqlServerSubstraitMinVersion("min_version") .withFlightSqlServerSubstraitMaxVersion("max_version") diff --git a/java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/SqlInfoBuilder.java b/java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/SqlInfoBuilder.java index 51e260d1d8ca5..18793f9b905fe 100644 --- a/java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/SqlInfoBuilder.java +++ b/java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/SqlInfoBuilder.java @@ -119,6 +119,11 @@ public SqlInfoBuilder withFlightSqlServerArrowVersion(final String value) { return withStringProvider(SqlInfo.FLIGHT_SQL_SERVER_ARROW_VERSION_VALUE, value); } + /** Set a value for SQL support. */ + public SqlInfoBuilder withFlightSqlServerSql(boolean value) { + return withBooleanProvider(SqlInfo.FLIGHT_SQL_SERVER_SQL_VALUE, value); + } + /** Set a value for Substrait support. */ public SqlInfoBuilder withFlightSqlServerSubstrait(boolean value) { return withBooleanProvider(SqlInfo.FLIGHT_SQL_SERVER_SUBSTRAIT_VALUE, value); diff --git a/java/flight/flight-sql/src/test/java/org/apache/arrow/flight/sql/example/FlightSqlExample.java b/java/flight/flight-sql/src/test/java/org/apache/arrow/flight/sql/example/FlightSqlExample.java index baf162cb919fc..bbe9873b7c387 100644 --- a/java/flight/flight-sql/src/test/java/org/apache/arrow/flight/sql/example/FlightSqlExample.java +++ b/java/flight/flight-sql/src/test/java/org/apache/arrow/flight/sql/example/FlightSqlExample.java @@ -217,6 +217,9 @@ public FlightSqlExample(final Location location) { .withFlightSqlServerVersion(metaData.getDatabaseProductVersion()) .withFlightSqlServerArrowVersion(metaData.getDriverVersion()) .withFlightSqlServerReadOnly(metaData.isReadOnly()) + .withFlightSqlServerSql(true) + .withFlightSqlServerSubstrait(false) + .withFlightSqlServerTransaction(SqlSupportedTransaction.SQL_SUPPORTED_TRANSACTION_NONE) .withSqlIdentifierQuoteChar(metaData.getIdentifierQuoteString()) .withSqlDdlCatalog(metaData.supportsCatalogsInDataManipulation()) .withSqlDdlSchema( metaData.supportsSchemasInDataManipulation())