lidavidm commented on code in PR #725:
URL: https://github.com/apache/arrow-adbc/pull/725#discussion_r1218485284
##########
c/driver/postgresql/connection.cc:
##########
@@ -459,6 +460,24 @@ class PqGetObjectsHelper {
return ADBC_STATUS_OK;
}
+ // libpq PQexecParams can use either text or binary transfers
+ // For now we are using text transfer internally, so arrays are sent
+ // back like {element1, element2} within a const char*
+ std::vector<std::string> PqTextArrayToVector(std::string text_array) {
+ text_array.erase(0, 1);
+ text_array.erase(text_array.size() - 1);
+
+ std::vector<std::string> elements;
+ std::stringstream ss(text_array);
Review Comment:
nit: move `text_array`?
##########
c/driver/postgresql/connection.cc:
##########
@@ -459,6 +460,24 @@ class PqGetObjectsHelper {
return ADBC_STATUS_OK;
}
+ // libpq PQexecParams can use either text or binary transfers
+ // For now we are using text transfer internally, so arrays are sent
+ // back like {element1, element2} within a const char*
+ std::vector<std::string> PqTextArrayToVector(std::string text_array) {
+ text_array.erase(0, 1);
+ text_array.erase(text_array.size() - 1);
+
+ std::vector<std::string> elements;
+ std::stringstream ss(text_array);
+ std::string tmp;
+
+ while (getline(ss, tmp, ',')) {
+ elements.push_back(tmp);
Review Comment:
And move `tmp`?
##########
c/driver/postgresql/connection.cc:
##########
@@ -513,12 +532,12 @@ class PqGetObjectsHelper {
ArrowArrayAppendString(constraint_type_col_,
ArrowCharView(constraint_type)),
error_);
- for (auto i = 0; i < row[2].len; i++) {
- // TODO: get actual name in here
- const char* data = row[2].data;
+ auto constraint_column_names =
+ PqTextArrayToVector(std::move(std::string(row[2].data)));
+ for (auto constraint_column_name : constraint_column_names) {
Review Comment:
nit: `const auto&`?
##########
c/driver/postgresql/connection.cc:
##########
@@ -513,12 +532,12 @@ class PqGetObjectsHelper {
ArrowArrayAppendString(constraint_type_col_,
ArrowCharView(constraint_type)),
error_);
- for (auto i = 0; i < row[2].len; i++) {
- // TODO: get actual name in here
- const char* data = row[2].data;
+ auto constraint_column_names =
+ PqTextArrayToVector(std::move(std::string(row[2].data)));
Review Comment:
move here is redundant, though
##########
c/driver/postgresql/postgresql_test.cc:
##########
@@ -258,6 +258,131 @@ TEST_F(PostgresConnectionTest, GetObjectsGetDbSchemas) {
ASSERT_TRUE(seen_public) << "public schema does not exist";
}
+TEST_F(PostgresConnectionTest, GetObjectsGetAllFindsPrimaryKey) {
+ ASSERT_THAT(AdbcConnectionNew(&connection, &error), IsOkStatus(&error));
+ ASSERT_THAT(AdbcConnectionInit(&connection, &database, &error),
IsOkStatus(&error));
+
+ if (!quirks()->supports_get_objects()) {
+ GTEST_SKIP();
+ }
+
+ adbc_validation::StreamReader reader;
+ ASSERT_THAT(
+ AdbcConnectionGetObjects(&connection, ADBC_OBJECT_DEPTH_ALL, nullptr,
nullptr,
+ nullptr, nullptr, nullptr,
&reader.stream.value, &error),
+ IsOkStatus(&error));
+ ASSERT_NO_FATAL_FAILURE(reader.GetSchema());
+ ASSERT_NO_FATAL_FAILURE(reader.Next());
+ ASSERT_NE(nullptr, reader.array->release);
+ ASSERT_GT(reader.array->length, 0);
+
+ // should exist on public.adbc_test id column
+ bool seen_id_column = false;
+ bool seen_primary_key = false;
+
+ // TODO: these are in the PqObjectsHelper.GetObjects method; move to shared
location
Review Comment:
I think eventually we want a better way of adapting iterators/rows to Arrow,
yeah. I had been toying with the idea of having an IDL so you could generate
type-safe row-based iterators/builders from a schema definition.
--
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]
