feat(c/driver/postgresql): Implement GetTableSchema #577
Conversation
|
Just a note that #573 is also related to the postgres type system! (In particular, it factors out the PostgresType -> ArrowSchema step that, as you noted here, is needed in more than one place). |
) Per lidavidm comment #577 (comment) made this act similar to the current nanoarrow setup
|
After #628 (whether accepted or not) will come back to this and clean up the string handling. I think the general structure is in place though, leveraging the great work @paleolimbot did on the type mapping |
| return ADBC_STATUS_INTERNAL; | ||
|
|
||
| if (db_schema != nullptr) { | ||
| char* schema = PQescapeIdentifier(conn_, db_schema, strlen(db_schema)); |
There was a problem hiding this comment.
A bit tangential to this but I don't see the current test suite as validating that we are safe against SQL injection attacks. Probably a reasonable follow up to set those types of test in place
There was a problem hiding this comment.
Agreed, do you want to file something?
| return ADBC_STATUS_INTERNAL; | ||
|
|
||
| if (db_schema != nullptr) { | ||
| char* schema = PQescapeIdentifier(conn_, db_schema, strlen(db_schema)); |
There was a problem hiding this comment.
Agreed, do you want to file something?
c/driver/postgresql/connection.cc
Outdated
| // Disconnect since PostgreSQL connections can be heavy. | ||
| { | ||
| AdbcStatusCode status = database_->Disconnect(&conn_, error); | ||
| if (status != ADBC_STATUS_OK) final_status = status; | ||
| } |
There was a problem hiding this comment.
We don't connect in this method, so we probably shouldn't disconnect if I'm not mistaken?
c/driver/postgresql/connection.cc
Outdated
| ExecStatusType pq_status = PQresultStatus(result); | ||
| if (pq_status == PGRES_TUPLES_OK) { | ||
| int num_rows = PQntuples(result); | ||
| ArrowSchemaInit(schema); |
There was a problem hiding this comment.
This would leave a half-initialized schema in the output argument if we error below which may trip up callers; maybe use a nanoarrow::UniqueSchema and move the schema at the end?
There was a problem hiding this comment.
Cool thanks for the tip. Think I did what you are asking - if so then probably want to do the same thing in statement.cc:InferSchema
There was a problem hiding this comment.
Ah, yeah. Though that got removed by the other refactor anyways.
lidavidm
changed the title
Definitely a few things need to be ironed out and potentially done as pre-cursors, but I think this is far along enough to review