[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #799: feat(c/driver/postgresql): Implement GetObjects with table_types argument

["lidavidm (via GitHub)" <gi...@apache.org>]

lidavidm commented on code in PR #799:
URL: https://github.com/apache/arrow-adbc/pull/799#discussion_r1230961607


##########
c/driver/postgresql/postgresql_test.cc:
##########
@@ -421,6 +437,80 @@ TEST_F(PostgresConnectionTest, GetObjectsGetAllFindsForeignKey) {
   }
 }
 
+TEST_F(PostgresConnectionTest, GetObjectsTableTypesFilter) {
+  ASSERT_THAT(AdbcConnectionNew(&connection, &error), IsOkStatus(&error));
+  ASSERT_THAT(AdbcConnectionInit(&connection, &database, &error), IsOkStatus(&error));
+
+  if (!quirks()->supports_get_objects()) {
+    GTEST_SKIP();
+  }
+
+  ASSERT_THAT(quirks()->DropView(&connection, "adbc_table_types_view_test", &error),
+              IsOkStatus(&error));
+  ASSERT_THAT(quirks()->DropTable(&connection, "adbc_table_types_table_test", &error),
+              IsOkStatus(&error));
+
+  struct AdbcStatement statement;
+  ASSERT_THAT(AdbcStatementNew(&connection, &statement, &error), IsOkStatus(&error));
+  {
+    ASSERT_THAT(
+        AdbcStatementSetSqlQuery(
+            &statement, "CREATE TABLE adbc_table_types_table_test (id1 INT, id2 INT)",
+            &error),
+        IsOkStatus(&error));
+    adbc_validation::StreamReader reader;
+    ASSERT_THAT(AdbcStatementExecuteQuery(&statement, &reader.stream.value,
+                                          &reader.rows_affected, &error),
+                IsOkStatus(&error));
+    ASSERT_EQ(reader.rows_affected, 0);
+    ASSERT_NO_FATAL_FAILURE(reader.GetSchema());
+    ASSERT_NO_FATAL_FAILURE(reader.Next());
+    ASSERT_EQ(reader.array->release, nullptr);
+  }
+  ASSERT_THAT(AdbcStatementRelease(&statement, &error), IsOkStatus(&error));

Review Comment:
   Same here, `Handle<AdbcStatement>` should simplify things



##########
c/driver/postgresql/postgresql_test.cc:
##########
@@ -63,6 +63,24 @@ class PostgresQuirks : public adbc_validation::DriverQuirks {
     return status;
   }
 
+  AdbcStatusCode DropView(struct AdbcConnection* connection, const std::string& name,
+                          struct AdbcError* error) const override {
+    struct AdbcStatement statement;
+    std::memset(&statement, 0, sizeof(statement));
+    AdbcStatusCode status = AdbcStatementNew(connection, &statement, error);
+    if (status != ADBC_STATUS_OK) return status;
+
+    std::string query = "DROP VIEW IF EXISTS " + name;
+    status = AdbcStatementSetSqlQuery(&statement, query.c_str(), error);
+    if (status != ADBC_STATUS_OK) {
+      std::ignore = AdbcStatementRelease(&statement, error);

Review Comment:
   Use `Handle<AdbcStatement>` to avoid this sort of thing



##########
c/validation/adbc_validation.h:
##########
@@ -47,6 +47,13 @@ class DriverQuirks {
     return ADBC_STATUS_OK;
   }
 
+  /// \brief Drop the given view. Used by tests to reset state.
+  virtual AdbcStatusCode DropView(struct AdbcConnection* connection,
+                                  const std::string& name,
+                                  struct AdbcError* error) const {
+    return ADBC_STATUS_OK;

Review Comment:
   return `NOT_IMPLEMENTED` for the default



##########
c/driver/postgresql/connection.cc:
##########
@@ -365,6 +370,45 @@ class PqGetObjectsHelper {
       params.push_back(std::string(table_name_));
     }
 
+    if (table_types_ != NULL) {

Review Comment:
   Use `nullptr` in C++



##########
c/driver/postgresql/connection.cc:
##########
@@ -185,6 +186,10 @@ class PqGetObjectsHelper {
     na_error_ = {0};
   }
 
+  static const inline std::unordered_map<std::string, std::string> PgTableTypes = {

Review Comment:
   nit: I don't think `inline` does much here, and I think coding convention would make this `kPgTableTypes`?



-- 
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