-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(c/driver/postgresql): Factor out Postgres type abstraction and test it independently of the driver #573
Conversation
c07b276
to
9aab307
Compare
d393be1
to
80bcda1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! This looks much better than the existing approach. I had a few questions but LGTM
c/driver/postgresql/postgres_type.h
Outdated
namespace adbcpq { | ||
|
||
// An enum of the types available in most Postgres pg_type tables | ||
enum PostgresTypeId { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit/format: why not enum class
and follow the convention of kUninitialized
, etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call on the enum class
!
I usually see ALL_CAPS_STYLE_VALUES in Arrow C++...is the kXXX
convention more common/the style used here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kFoo
is the recommended style: https://google.github.io/styleguide/cppguide.html#Enumerator_Names
Arrow is inconsistent about it, partly because it often follows this pattern instead:
class Foo {
enum Bar { // note: NOT enum class
CONSTANT,
}
};
Foo::CONSTANT;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! I'll tackle the gnarly rename tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! Enum values are fixed.
0e8cdd7
to
f1ee190
Compare
Looks like there are some MSVC specific errors + you might need to dllexport some of the new things to use them in tests |
Co-authored-by: David Li <li.davidm96@gmail.com>
0c6905b
to
c90a619
Compare
I tried a few ways to get the tests to work on Windows but none of them worked...for now I just put everything back in the header. If you're particularly opposed to that I could try harder but it may take me a bit to learn the requisite linking-on-windows behaviour. |
No worries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
The existing
type.h
/type.cc
provides a limited abstraction of the postgres type system; however, eventually this driver will need to deal with range, domain, array, and record types. This PR factors out existing behaviour and includes aSetSchema()
method to provide the default mapping from the postgres -> arrow type systems. This PR doesn't provide full type support but connects enough wires that future support for these types is more straightforward.Many commits on this branch also implemented a COPY -> Arrow converter abstraction that can handle range, domain, array, and record types. I moved that to a separate branch ( paleolimbot#1 ) because it has an orthogonal scope to this one. If that's of interest I'll continue with that too (which would add support for Array, Domain, and date/time types in results).