fix(c/driver/postgresql): Ensure schema ordering is consisent and res…

…pects case sensitivity of table names (#2028) Fixes #2006.
apache · Jul 24, 2024 · d6255ac · d6255ac
1 parent 7f53b8a
commit d6255ac
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 10 deletions.
diff --git a/c/driver/postgresql/connection.cc b/c/driver/postgresql/connection.cc
@@ -1146,19 +1146,25 @@ AdbcStatusCode PostgresConnection::GetTableSchema(const char* catalog,
                                                   struct AdbcError* error) {
   AdbcStatusCode final_status = ADBC_STATUS_OK;
 
+  char* quoted = PQescapeIdentifier(conn_, table_name, strlen(table_name));
+  std::string table_name_str(quoted);
+  PQfreemem(quoted);
+
+  if (db_schema != nullptr) {
+    quoted = PQescapeIdentifier(conn_, db_schema, strlen(db_schema));
+    table_name_str = std::string(quoted) + "." + table_name_str;
+    PQfreemem(quoted);
+  }
+
   std::string query =
       "SELECT attname, atttypid "
       "FROM pg_catalog.pg_class AS cls "
       "INNER JOIN pg_catalog.pg_attribute AS attr ON cls.oid = attr.attrelid "
       "INNER JOIN pg_catalog.pg_type AS typ ON attr.atttypid = typ.oid "
-      "WHERE attr.attnum >= 0 AND cls.oid = $1::regclass::oid";
+      "WHERE attr.attnum >= 0 AND cls.oid = $1::regclass::oid "
+      "ORDER BY attr.attnum";
 
-  std::vector<std::string> params;
-  if (db_schema != nullptr) {
-    params.push_back(std::string(db_schema) + "." + table_name);
-  } else {
-    params.push_back(table_name);
-  }
+  std::vector<std::string> params = {table_name_str};
 
   PqResultHelper result_helper =
       PqResultHelper{conn_, std::string(query.c_str()), params, error};

diff --git a/c/driver/postgresql/postgresql_test.cc b/c/driver/postgresql/postgresql_test.cc
@@ -673,6 +673,47 @@ TEST_F(PostgresConnectionTest, MetadataSetCurrentDbSchema) {
   ASSERT_THAT(AdbcStatementRelease(&statement.value, &error), IsOkStatus(&error));
 }
 
+TEST_F(PostgresConnectionTest, MetadataGetSchemaCaseSensitiveTable) {
+  ASSERT_THAT(AdbcConnectionNew(&connection, &error), IsOkStatus(&error));
+  ASSERT_THAT(AdbcConnectionInit(&connection, &database, &error), IsOkStatus(&error));
+
+  // Create sample table
+  {
+    adbc_validation::Handle<struct AdbcStatement> statement;
+    ASSERT_THAT(AdbcStatementNew(&connection, &statement.value, &error),
+                IsOkStatus(&error));
+
+    ASSERT_THAT(AdbcStatementSetSqlQuery(&statement.value,
+                                         "DROP TABLE IF EXISTS \"Uppercase\"", &error),
+                IsOkStatus(&error));
+    ASSERT_THAT(AdbcStatementExecuteQuery(&statement.value, nullptr, nullptr, &error),
+                IsOkStatus(&error));
+
+    ASSERT_THAT(
+        AdbcStatementSetSqlQuery(
+            &statement.value, "CREATE TABLE \"Uppercase\" (ints INT, strs TEXT)", &error),
+        IsOkStatus(&error));
+    ASSERT_THAT(AdbcStatementExecuteQuery(&statement.value, nullptr, nullptr, &error),
+                IsOkStatus(&error));
+  }
+
+  // Check its schema
+  nanoarrow::UniqueSchema schema;
+  ASSERT_THAT(AdbcConnectionGetTableSchema(&connection, nullptr, nullptr, "Uppercase",
+                                           schema.get(), &error),
+              IsOkStatus(&error));
+
+  ASSERT_NE(schema->release, nullptr);
+  ASSERT_STREQ(schema->format, "+s");
+  ASSERT_EQ(schema->n_children, 2);
+  ASSERT_STREQ(schema->children[0]->format, "i");
+  ASSERT_STREQ(schema->children[1]->format, "u");
+  ASSERT_STREQ(schema->children[0]->name, "ints");
+  ASSERT_STREQ(schema->children[1]->name, "strs");
+
+  // Do we have to release the connection here?
+}
+
 TEST_F(PostgresConnectionTest, MetadataGetStatistics) {
   if (!quirks()->supports_statistics()) {
     GTEST_SKIP();

diff --git a/c/validation/adbc_validation_statement.cc b/c/validation/adbc_validation_statement.cc
@@ -2065,15 +2065,15 @@ void StatementTest::TestSqlPrepareErrorParamCountMismatch() {
 void StatementTest::TestSqlQueryEmpty() {
   ASSERT_THAT(AdbcStatementNew(&connection, &statement, &error), IsOkStatus(&error));
 
-  ASSERT_THAT(quirks()->DropTable(&connection, "QUERYEMPTY", &error), IsOkStatus(&error));
+  ASSERT_THAT(quirks()->DropTable(&connection, "queryempty", &error), IsOkStatus(&error));
   ASSERT_THAT(
-      AdbcStatementSetSqlQuery(&statement, "CREATE TABLE QUERYEMPTY (FOO INT)", &error),
+      AdbcStatementSetSqlQuery(&statement, "CREATE TABLE queryempty (FOO INT)", &error),
       IsOkStatus(&error));
   ASSERT_THAT(AdbcStatementExecuteQuery(&statement, nullptr, nullptr, &error),
               IsOkStatus(&error));
 
   ASSERT_THAT(
-      AdbcStatementSetSqlQuery(&statement, "SELECT * FROM QUERYEMPTY WHERE 1=0", &error),
+      AdbcStatementSetSqlQuery(&statement, "SELECT * FROM queryempty WHERE 1=0", &error),
       IsOkStatus(&error));
   {
     StreamReader reader;