feat(c/driver/postgresql): Implement Foreign Key information for GetObjects #757
Conversation
| @@ -402,6 +402,193 @@ TEST_F(PostgresConnectionTest, GetObjectsGetAllFindsPrimaryKey) { | |||
| ASSERT_TRUE(seen_primary_key) << "could not find primary key for adbc_pkey_test"; | |||
| } | |||
|
|
|||
| TEST_F(PostgresConnectionTest, GetObjectsGetAllFindsForeignKey) { | |||
There was a problem hiding this comment.
I initially tried to combine this with the primary key test, but eventually moved away doing that as this was easier to express with its own set of tables. Still a good deal of shared elements that could be shared between the tests though
There was a problem hiding this comment.
Could possibly factor out a helper that just drops the given table + runs the given DDL so you can ASSERT_NO_FATAL_FAILURE(CreateTable("adbc_fkey_test", "CREATE TABLE ..."));
There was a problem hiding this comment.
The nested loop is...messy, I have had some ideas about how to simplify this sort of thing (basically, I think Arrow could use an IDL like Protobuf, where you can take a predefined schema and generate higher-level accessors/iterators/validators)
There was a problem hiding this comment.
I've also thought about whether some complicated tests could belong in Python instead (with the caveats around that development loop)
There was a problem hiding this comment.
Yea I don't like the loops. The IDL would be nice. Also been toying with some util helpers to abstract it in another branch, just hasn't fully materialized yet
| if ((fk_catalog_name_str == "postgres") && | ||
| (fk_schema_name_str == "public") && | ||
| (fk_table_name_str == "adbc_fkey_test_base")) { | ||
| // TODO: the current implementation makes it so the length of |
There was a problem hiding this comment.
I might be overthinking this but we have for instance 5 constraints that added right now in the test suite, of which only 2 are foreign keys. The fact that the adbc specification treats the column names of the source table and the usages as separate lists I think means we would have to ensure they are always the same length to know which source column maps to which foreign key table column. Is that the intent?
There was a problem hiding this comment.
Yes, if it's a foreign key it should be 1:1 so we know what column is being referenced by each column in the foreign key constraint.
|
I don't think the R failure is related to this PR? |
| @@ -402,6 +402,193 @@ TEST_F(PostgresConnectionTest, GetObjectsGetAllFindsPrimaryKey) { | |||
| ASSERT_TRUE(seen_primary_key) << "could not find primary key for adbc_pkey_test"; | |||
| } | |||
|
|
|||
| TEST_F(PostgresConnectionTest, GetObjectsGetAllFindsForeignKey) { | |||
There was a problem hiding this comment.
Could possibly factor out a helper that just drops the given table + runs the given DDL so you can ASSERT_NO_FATAL_FAILURE(CreateTable("adbc_fkey_test", "CREATE TABLE ..."));
| @@ -402,6 +402,193 @@ TEST_F(PostgresConnectionTest, GetObjectsGetAllFindsPrimaryKey) { | |||
| ASSERT_TRUE(seen_primary_key) << "could not find primary key for adbc_pkey_test"; | |||
| } | |||
|
|
|||
| TEST_F(PostgresConnectionTest, GetObjectsGetAllFindsForeignKey) { | |||
There was a problem hiding this comment.
The nested loop is...messy, I have had some ideas about how to simplify this sort of thing (basically, I think Arrow could use an IDL like Protobuf, where you can take a predefined schema and generate higher-level accessors/iterators/validators)
| @@ -402,6 +402,193 @@ TEST_F(PostgresConnectionTest, GetObjectsGetAllFindsPrimaryKey) { | |||
| ASSERT_TRUE(seen_primary_key) << "could not find primary key for adbc_pkey_test"; | |||
| } | |||
|
|
|||
| TEST_F(PostgresConnectionTest, GetObjectsGetAllFindsForeignKey) { | |||
There was a problem hiding this comment.
I've also thought about whether some complicated tests could belong in Python instead (with the caveats around that development loop)
|
I think the R failure is because the upstream nanoarrow package must've gotten out of sync |
|
I kicked the job but if it doesn't pass I don't think we have to consider it a blocker (Dewey has been working on the R stuff anyways) |
|
Alright, it passes. Do you want to add any changes here, or defer those for later PRs? |
|
I think defer any changes to subsequent PRs; not pushing those off forever just think it would be too much to solve alongside implementing this |
|
(...though it would be very unfortunate to have duplicate APIs for this.) |
No description provided.