feat(c/driver/postgres): Implement GetObjectsDbSchemas for Postgres #679

WillAyd · 2023-05-11T23:48:56Z

No description provided.

github-actions · 2023-05-11T23:49:08Z

⚠️ Please follow the Conventional Commits format in CONTRIBUTING.md for PR titles.

WillAyd · 2023-05-11T23:50:04Z

c/driver/postgresql/connection.cc

+
+  pg_result* result = result_helper.Execute();
+  ExecStatusType pq_status = PQresultStatus(result);
+  if (pq_status == PGRES_TUPLES_OK) {


This pg looping is getting pretty repetitive. Feels like there is a way to refactor into the existing ResultHelper class and create an iterator over the results

WillAyd · 2023-05-11T23:53:42Z

c/driver/postgresql/connection.cc

        } else {
-          return ADBC_STATUS_NOT_IMPLEMENTED;
+          if (depth >= ADBC_OBJECT_DEPTH_DB_SCHEMAS) {
+            RAISE_ADBC(PostgresConnectionGetSchemasImpl(conn, depth,


Here we are grabbing the associated schemas in a loop, but this brings up a structural question. I assume from the adbc.h header that GetInfo should maintain a hierarchy from catalog -> schema -> table -> constraints, etc...

However, the current implementation (shared with sqlite) creates the overall array structure in a call to AdbcInitConnectionObjectsSchema; at face value I don't see how this preserves the hierarchy between elements. With this expression in a loop, it ends up writing the schema name for as many databases as exist.

I could move it out of the loop to fix that, but figured I'd ask about the hierarchical intent of the result before going too far

Yeah, it's supposed to be a nested data structure, so the catalog name appears once, with the schemas nested inside that row, and ditto for tables being nested into catalogs. The Flight SQL driver has code along those lines, if you want to reference it (in Go/Java).

It is a bit annoying and if I could do it again I would probably stick with the Flight SQL design (GetTables/GetCatalogs/GetSchemas). (Or possibly Calcite's design, which is more flexible; that has come up before too.)

AFACIT those implementations generate maps that relate a table to a schema. I'm guessing elsewhere in the FlightSQL code base is where that mapping gets generated back into an Arrow structure. Any pointers on where I should look for that?

Generally I am unsure I have the right idea of how this is all supposed to be laid out. For example, if we have the following setup in the database:

[{ "catalog_name": "postgres", "catalog_db_schema": [ { "db_schema_name": "public", "db_schema_tables": [...], }, { "db_schema_name": "template0", "db_schema_tables": [...], }, { "db_schema_name": "template1", "db_schema_tables": [...], }, }] }]

Am I supposed to access the value of "public" via reader->array_view->children[1][0]->children[0]? I have a feeling the children[1][0] access is incorrect and there should be a way to extract the list element by index from an array via nanoarrow, but didn't see such a function.

Is that something that needs to be implemented upstream or do I have the wrong mental model?

So to iterate "catalog_db_schema", you would use nanoarrow to get the array offsets of element 0 in the catalog_db_schema array, which would give you a slice of the child array to iterate through. For each of those elements, you would then again get the array offsets of that element in the db_schema_tables array, which would give you a slice of the child child array.

You can see this starting here:

arrow-adbc/c/validation/adbc_validation.cc

Line 564 in 47775a8

void ConnectionTest::TestMetadataGetObjectsTables() {

The code I'm referencing for building up the structure is here: https://github.com/apache/arrow-adbc/blob/main/go/adbc/driver/internal/shared_utils.go

You could also reference the code here: apache/arrow#14082

So to iterate "catalog_db_schema", you would use nanoarrow to get the array offsets of element 0 in the catalog_db_schema array, which would give you a slice of the child array to iterate through...

Great thanks for the guidance. I see nanoarrow has ArrowArrayViewUnionChildOffset but didn't see anything similar for Lists (though I think the member access would be the same as far as lists go). Think its worth trying to upstream that function for lists first?

Ah...I was going to say it exists, but just realized, it's actually part of this project:

arrow-adbc/c/validation/adbc_validation_util.cc

Line 67 in 47775a8

int64_t ArrowArrayViewGetOffsetUnsafe(struct ArrowArrayView* array_view, int64_t i) {

I think it'd be worth upstreaming.

Cool - I'll give that a shot

WillAyd · 2023-05-11T23:54:14Z

c/driver/postgresql/postgresql_test.cc

+          ArrowArrayViewGetStringUnsafe(reader.array_view->children[0], row);
+      auto val_str = std::string(val.data, val.size_bytes);
+      if (val_str == "postgres") {
+        // ASSERT_EQ(reader.array->children[1]->children[0]->length, 1);


This commented assertion and the one on L244 relate to the question about whether or not a hierarchy should exist between the elements inspected by GetInfo

Similar to what exists for Dense/Sparse Union. This would at least be useful in the ADBC library, which currently provides its own implementation apache/arrow-adbc#679 (comment)

Related to #679 (comment) this pulls down the nanoarrow change apache/arrow-nanoarrow#197

WillAyd · 2023-05-22T21:53:02Z

c/driver/postgresql/connection.cc

          CHECK_NA(INTERNAL, ArrowArrayAppendNull(catalog_db_schemas_col, 1), error);
-        } else {
-          return ADBC_STATUS_NOT_IMPLEMENTED;
+        } else if (!db_schema ||


This is messy but there is a test that provides a schema named this schema does not exist and expects a NULL value back. This doesn't work if you provide a schema name that actually does exist, but neither does the sqlite implementation AFAICT

That test expects a non-NULL, but empty, list of schemas back. Obviously, if the schema exists, we should return it.

WillAyd · 2023-05-22T21:54:27Z

c/validation/adbc_validation.cc


-        ASSERT_FALSE(ArrowArrayViewIsNull(catalog_db_schemas_list, row))
-            << "Row " << row << " should have non-null catalog_db_schemas";
+        // ASSERT_FALSE(ArrowArrayViewIsNull(catalog_db_schemas_list, row))


AFAIK postgres only allows you to retrieve the schemas from the database you are currently connected to, but we can see what other databases exist. So either this test is too restrictive being in the base tests or we should make it so that the postgres driver only lists the current database in GetObjects; I think the former option is better

The intent is that if you request the schemas, it should come back as a (possibly empty) list, not null.

Ah OK makes sense. Thanks for the guidance

WillAyd · 2023-05-22T21:54:39Z

c/validation/adbc_validation.cc

            << std::string(catalog_name.data, catalog_name.size_bytes)
            << ") should have nonempty catalog_db_schemas ";
-        ASSERT_FALSE(ArrowArrayViewIsNull(catalog_db_schemas_list, row));
+        // ASSERT_FALSE(ArrowArrayViewIsNull(catalog_db_schemas_list, row));


This looks to be duplicative of the test on L511 above

WillAyd · 2023-05-24T16:57:56Z

I think this is working now with proper tests. Definitely agree on the structure of this being less than ideal, so happy to move towards a more FlightSQL/Go approach here or in a follow up

lidavidm

Thanks! I think we will want to refactor this before tackling columns/constraints, but things at least work.

lidavidm · 2023-05-24T18:27:56Z

c/driver/postgresql/connection.cc

+    PqResultRow db_row = *curr_iter;
+    curr_db = db_row[0].data;
+  } else {
+    return ADBC_STATUS_NOT_IMPLEMENTED;


nit: should probably be like STATUS_IO or something?

I changed to INTERNAL for now - I don't know if there is any valid reason why this call would fail

lidavidm · 2023-05-24T18:28:58Z

c/driver/postgresql/connection.cc

+    assert(curr_db_helper.NumRows() == 1);
+    auto curr_iter = curr_db_helper.begin();
+    PqResultRow db_row = *curr_iter;
+    curr_db = db_row[0].data;


will this pointer stay valid? maybe we should copy to a string?

lidavidm · 2023-05-24T18:29:54Z

c/driver/postgresql/connection.cc

+      }
+
+      int res =
+          StringBuilderAppend(&query, "%s%s%s", " AND nspname ='", schema_name, "'");


Eventually we should do this via query parameters instead of escaping (especially since the spec lets you use LIKE-style patterns for these filters)

lidavidm

Thanks!

Implemented GetSchemas for Postgres

38805ec

WillAyd commented May 11, 2023

View reviewed changes

WillAyd changed the title ~~feat:(c/driver/postgres): Implement GetObjectsDbSchemas for Postgres~~ feat(c/driver/postgres): Implement GetObjectsDbSchemas for Postgres May 11, 2023

Merge branch 'main' into pg-objects-schema

ddd08ee

WillAyd mentioned this pull request May 19, 2023

feat: Add ArrowArrayViewListChildOffset() apache/arrow-nanoarrow#197

Merged

WillAyd mentioned this pull request May 22, 2023

refactor(c): Use ArrowArrayViewListChildOffset from nanoarrow #696

Merged

lidavidm pushed a commit that referenced this pull request May 22, 2023

refactor(c): Use ArrowArrayViewListChildOffset from nanoarrow (#696)

ecf172e

Related to #679 (comment) this pulls down the nanoarrow change apache/arrow-nanoarrow#197

WillAyd added 8 commits May 22, 2023 11:24

Merge branch 'main' into pg-objects-schema

5aad615

Use iterator

8991a3d

Better implementation

9281348

Somewhat working tests

8222ea0

Fixed schema name retrieval test issue

f7bf19d

Recommented code

aad5bac

Comments cleanup

7064b89

Commented maybe bad expectation in base class

853daa4

WillAyd commented May 22, 2023

View reviewed changes

WillAyd added 7 commits May 23, 2023 16:01

Checkpoint

6e803c6

Fixed bad catalog test

df9024b

checkpoint

55980cc

Fixed test expectation for empty child schema list

f3fc512

Fixed test for schema filter

aee8833

Revert test commenting

c8cc864

Move comment

6028888

WillAyd marked this pull request as ready for review May 24, 2023 16:57

lidavidm approved these changes May 24, 2023

View reviewed changes

feedbac

8c8ee97

lidavidm approved these changes May 24, 2023

View reviewed changes

lidavidm merged commit dcc681e into apache:main May 24, 2023

lidavidm added this to the ADBC Libraries 0.5.0 milestone May 24, 2023

WillAyd deleted the pg-objects-schema branch May 24, 2023 22:12

feat(c/driver/postgres): Implement GetObjectsDbSchemas for Postgres #679

feat(c/driver/postgres): Implement GetObjectsDbSchemas for Postgres #679

Uh oh!

Conversation

WillAyd commented May 11, 2023

Uh oh!

github-actions bot commented May 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillAyd May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillAyd May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillAyd May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillAyd commented May 24, 2023

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

WillAyd May 11, 2023 •

edited

Loading

WillAyd May 16, 2023 •

edited

Loading

WillAyd May 11, 2023 •

edited

Loading