refactor(c/driver/postgresql): Use AdbcGetInfoData structure #769
Conversation
c/validation/adbc_validation_util.h
Outdated
|
|
||
| /// \brief Read an AdbcGetInfoData struct with RAII safety | ||
| struct GetInfoReader { | ||
| explicit GetInfoReader(struct ArrowArrayView* array_view) : array_view_(array_view) { |
There was a problem hiding this comment.
Probably need to do something here to better tie in the lifetime of the array_view, though I wasn't sure if that was worth doing as a separate class like this or maybe integrating into the existing StreamReader class
c/validation/adbc_validation_util.h
Outdated
| #include <gmock/gmock.h> | ||
| #include <gtest/gtest.h> | ||
| #include <nanoarrow/nanoarrow.h> | ||
| #include <utils.h> |
There was a problem hiding this comment.
Best to use "" includes for our headers
Hmm, we may also want to switch to common/utils.h or something eventually, a bare utils.h is liable to get mixed up
There was a problem hiding this comment.
Makes sense. The common/utils.h change seems to impact quite a few other modules so may do separately
c/driver/common/utils.h
Outdated
|
|
||
| // returns NULL on error | ||
| // for now all arguments are required | ||
| struct AdbcGetInfoCatalog* AdbcGetInfoDataGetCatalogByName( |
There was a problem hiding this comment.
Shouldn't these all be GetObjects not GetInfo?
|
Ah, sorry, I didn't realize that PR just now would conflict with this one |
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
LGTM, just need to fix up the CMake since I poked the same files
|
No worries - I can fix conflicts this afternoon. Other PR looks awesome |
|
Looks like one of the include paths isn't right? |
|
Oh, you got it before me |
|
Hmm, was there a reason to put it in utils.c instead of adbc_validation_util.cc? |
|
As things stand now I would be indifferent to the move. In the future I was thinking this could have more utility than just being for tests, ex: a client tool may want to use these abstractions |
|
Ah, yeah, I just saw the error and figured unique_ptr would clean it right up |
2 participants
No description provided.