Skip to content

Commit

Permalink
[C++][Format] Address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
lidavidm committed Sep 6, 2022
1 parent fd57dab commit 5823b75
Show file tree
Hide file tree
Showing 10 changed files with 83 additions and 35 deletions.
6 changes: 6 additions & 0 deletions cpp/src/arrow/flight/integration_tests/test_integration.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -1064,6 +1066,7 @@ class FlightSqlExtensionScenario : public FlightSqlScenario {
Status ValidateMetadataRetrieval(sql::FlightSqlClient* sql_client) {
std::unique_ptr<FlightInfo> info;
std::vector<int32_t> 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,
Expand Down Expand Up @@ -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"));
Expand Down
3 changes: 1 addition & 2 deletions cpp/src/arrow/flight/sql/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
55 changes: 33 additions & 22 deletions cpp/src/arrow/flight/sql/acero_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -73,6 +76,7 @@ arrow::Result<std::shared_ptr<Buffer>> 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"({
Expand All @@ -93,7 +97,7 @@ arrow::Result<std::shared_ptr<Buffer>> MakeSubstraitPlan() {
"local_files": {
"items": [
{
"uri_file": "file://FILENAME_PLACEHOLDER",
"uri_file": "URI_PLACEHOLDER",
"parquet": {}
}
]
Expand All @@ -103,35 +107,35 @@ arrow::Result<std::shared_ptr<Buffer>> 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<int> 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<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));
const auto& ids = checked_cast<const UInt32Array&>(*batch->column(0));
const auto& values = checked_cast<const DenseUnionArray&>(*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<const DenseUnionScalar&>(*scalar).value,
ASSERT_EQ(*checked_cast<const DenseUnionScalar&>(*scalar).value,
BooleanScalar(true));
} else if (ids.Value(i) == SqlInfoOptions::SqlInfo::FLIGHT_SQL_SERVER_TRANSACTION) {
ASSERT_EQ(
*static_cast<const DenseUnionScalar&>(*scalar).value,
*checked_cast<const DenseUnionScalar&>(*scalar).value,
Int32Scalar(
SqlInfoOptions::SqlSupportedTransaction::SQL_SUPPORTED_TRANSACTION_NONE));
} else {
Expand All @@ -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<FlightInfo> 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
Expand All @@ -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());
Expand All @@ -172,21 +178,24 @@ 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) {
#ifdef _WIN32
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);

Expand All @@ -201,7 +210,8 @@ TEST_F(TestAcero, Prepare) {
ASSERT_OK_AND_ASSIGN(std::unique_ptr<FlightInfo> 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));
Expand All @@ -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
Expand Down
4 changes: 3 additions & 1 deletion cpp/src/arrow/flight/sql/example/sqlite_sql_info.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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::
Expand Down
18 changes: 13 additions & 5 deletions cpp/src/arrow/flight/sql/types.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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.
Expand Down
21 changes: 16 additions & 5 deletions format/FlightSql.proto
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down

0 comments on commit 5823b75

Please sign in to comment.