You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "lidavidm (via GitHub)" <gi...@apache.org> on 2023/05/11 13:10:15 UTC
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #676: feat(c/driver/postgresql): Implement GetObjectsSchema
lidavidm commented on code in PR #676:
URL: https://github.com/apache/arrow-adbc/pull/676#discussion_r1191156831
##########
c/driver/postgresql/postgresql_test.cc:
##########
@@ -162,6 +161,53 @@ TEST_F(PostgresConnectionTest, GetInfoMetadata) {
ASSERT_THAT(seen, ::testing::UnorderedElementsAreArray(info));
}
+TEST_F(PostgresConnectionTest, GetObjectsGetCatalogs) {
+ ASSERT_THAT(AdbcConnectionNew(&connection, &error), IsOkStatus(&error));
+ ASSERT_THAT(AdbcConnectionInit(&connection, &database, &error), IsOkStatus(&error));
+
+ if (!quirks()->supports_get_objects()) {
+ GTEST_SKIP();
+ }
+
+ {
Review Comment:
nit: why the extra nesting?
##########
c/driver/postgresql/postgresql_test.cc:
##########
@@ -162,6 +161,53 @@ TEST_F(PostgresConnectionTest, GetInfoMetadata) {
ASSERT_THAT(seen, ::testing::UnorderedElementsAreArray(info));
}
+TEST_F(PostgresConnectionTest, GetObjectsGetCatalogs) {
+ 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_CATALOGS, 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);
+
+ // A default psql install will contain these databases, but users
+ // can drop. Googletest does not support a warning outcome for tests (?)
+ // so these may be too strict
Review Comment:
I think it's fine to assume default PostgreSQL
##########
c/driver/postgresql/connection.cc:
##########
@@ -145,6 +145,76 @@ AdbcStatusCode PostgresConnection::GetInfo(struct AdbcConnection* connection,
return BatchToArrayStream(&array, &schema, out, error);
}
+AdbcStatusCode PostgresConnectionGetObjectsImpl(
+ PGconn* conn, int depth, const char* catalog, const char* db_schema,
+ const char* table_name, const char** table_types, const char* column_name,
+ struct ArrowSchema* schema, struct ArrowArray* array, struct AdbcError* error) {
+ RAISE_ADBC(AdbcInitConnectionObjectsSchema(schema, error));
+
+ struct ArrowError na_error = {0};
+ CHECK_NA_DETAIL(INTERNAL, ArrowArrayInitFromSchema(array, schema, &na_error), &na_error,
+ error);
+ CHECK_NA(INTERNAL, ArrowArrayStartAppending(array), error);
+
+ struct ArrowArray* catalog_name_col = array->children[0];
+ struct ArrowArray* catalog_db_schemas_col = array->children[1];
+
+ // TODO: support proper filters
+ if (!catalog) {
+ struct StringBuilder query = {0};
+ if (StringBuilderInit(&query, /*initial_size=*/256) != 0) return ADBC_STATUS_INTERNAL;
+
+ if (StringBuilderAppend(&query, "%s", "SELECT datname FROM pg_catalog.pg_database"))
+ return ADBC_STATUS_INTERNAL;
Review Comment:
nit: braces? (not sure why clang-format doesn't just do that for you...)
##########
c/driver/postgresql/connection.cc:
##########
@@ -145,6 +145,76 @@ AdbcStatusCode PostgresConnection::GetInfo(struct AdbcConnection* connection,
return BatchToArrayStream(&array, &schema, out, error);
}
+AdbcStatusCode PostgresConnectionGetObjectsImpl(
+ PGconn* conn, int depth, const char* catalog, const char* db_schema,
+ const char* table_name, const char** table_types, const char* column_name,
+ struct ArrowSchema* schema, struct ArrowArray* array, struct AdbcError* error) {
+ RAISE_ADBC(AdbcInitConnectionObjectsSchema(schema, error));
+
+ struct ArrowError na_error = {0};
+ CHECK_NA_DETAIL(INTERNAL, ArrowArrayInitFromSchema(array, schema, &na_error), &na_error,
+ error);
+ CHECK_NA(INTERNAL, ArrowArrayStartAppending(array), error);
+
+ struct ArrowArray* catalog_name_col = array->children[0];
+ struct ArrowArray* catalog_db_schemas_col = array->children[1];
+
+ // TODO: support proper filters
+ if (!catalog) {
+ struct StringBuilder query = {0};
+ if (StringBuilderInit(&query, /*initial_size=*/256) != 0) return ADBC_STATUS_INTERNAL;
+
+ if (StringBuilderAppend(&query, "%s", "SELECT datname FROM pg_catalog.pg_database"))
+ return ADBC_STATUS_INTERNAL;
+
+ PqResultHelper result_helper = PqResultHelper{conn, query.buffer};
+ StringBuilderReset(&query);
+ pg_result* result = result_helper.Execute();
+
+ ExecStatusType pq_status = PQresultStatus(result);
+ if (pq_status == PGRES_TUPLES_OK) {
+ int num_rows = PQntuples(result);
+ array->length = num_rows - 1; // makes ArrowArrayFinishElement happy, but why?
+ for (int row = 0; row < num_rows; row++) {
+ const char* db_name = PQgetvalue(result, row, 0);
+ CHECK_NA(INTERNAL,
+ ArrowArrayAppendString(catalog_name_col, ArrowCharView(db_name)), error);
+ if (depth == ADBC_OBJECT_DEPTH_CATALOGS) {
+ CHECK_NA(INTERNAL, ArrowArrayAppendNull(catalog_db_schemas_col, 1), error);
+ } else {
+ return ADBC_STATUS_NOT_IMPLEMENTED;
+ }
+ }
+ }
+
+ CHECK_NA(INTERNAL, ArrowArrayFinishElement(array), error);
+ }
Review Comment:
maybe add an 'else return NOT_IMPLEMENTED' so it's clear what happened?
##########
c/driver/postgresql/postgresql_test.cc:
##########
@@ -162,6 +161,53 @@ TEST_F(PostgresConnectionTest, GetInfoMetadata) {
ASSERT_THAT(seen, ::testing::UnorderedElementsAreArray(info));
}
+TEST_F(PostgresConnectionTest, GetObjectsGetCatalogs) {
+ 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_CATALOGS, 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);
+
+ // A default psql install will contain these databases, but users
+ // can drop. Googletest does not support a warning outcome for tests (?)
+ // so these may be too strict
+ bool seen_postgres_db = false;
+ bool seen_template0_db = false;
+ bool seen_tempalte1_db = false;
+
+ do {
+ for (int64_t row = 0; row < reader.array->length; row++) {
+ ArrowStringView val =
+ ArrowArrayViewGetStringUnsafe(reader.array_view->children[0], row);
+ auto val_str = std::string(val.data, val.size_bytes);
+ if (val_str == "postgres")
+ seen_postgres_db = true;
+ else if (val_str == "template0")
+ seen_template0_db = true;
+ else if (val_str == "template1")
+ seen_tempalte1_db = true;
Review Comment:
nit: braces?
##########
c/driver/postgresql/connection.cc:
##########
@@ -145,6 +145,76 @@ AdbcStatusCode PostgresConnection::GetInfo(struct AdbcConnection* connection,
return BatchToArrayStream(&array, &schema, out, error);
}
+AdbcStatusCode PostgresConnectionGetObjectsImpl(
+ PGconn* conn, int depth, const char* catalog, const char* db_schema,
+ const char* table_name, const char** table_types, const char* column_name,
+ struct ArrowSchema* schema, struct ArrowArray* array, struct AdbcError* error) {
+ RAISE_ADBC(AdbcInitConnectionObjectsSchema(schema, error));
+
+ struct ArrowError na_error = {0};
+ CHECK_NA_DETAIL(INTERNAL, ArrowArrayInitFromSchema(array, schema, &na_error), &na_error,
+ error);
+ CHECK_NA(INTERNAL, ArrowArrayStartAppending(array), error);
+
+ struct ArrowArray* catalog_name_col = array->children[0];
+ struct ArrowArray* catalog_db_schemas_col = array->children[1];
+
+ // TODO: support proper filters
+ if (!catalog) {
+ struct StringBuilder query = {0};
+ if (StringBuilderInit(&query, /*initial_size=*/256) != 0) return ADBC_STATUS_INTERNAL;
+
+ if (StringBuilderAppend(&query, "%s", "SELECT datname FROM pg_catalog.pg_database"))
+ return ADBC_STATUS_INTERNAL;
+
+ PqResultHelper result_helper = PqResultHelper{conn, query.buffer};
+ StringBuilderReset(&query);
+ pg_result* result = result_helper.Execute();
+
+ ExecStatusType pq_status = PQresultStatus(result);
+ if (pq_status == PGRES_TUPLES_OK) {
+ int num_rows = PQntuples(result);
+ array->length = num_rows - 1; // makes ArrowArrayFinishElement happy, but why?
Review Comment:
Hmm, this seems suspect.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org