-
Notifications
You must be signed in to change notification settings - Fork 101
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): Use copy writer in BindStream for parameter binding #2157
refactor(c/driver/postgresql): Use copy writer in BindStream for parameter binding #2157
Conversation
The CI failure here is for Python packaging and I don't think it is related (I can't find a recent Python packaging job on main to compare):
|
c/driver/postgresql/bind_stream.h
Outdated
return ADBC_STATUS_NOT_IMPLEMENTED; | ||
|
||
int64_t param_length = param_buffer->size_bytes - last_offset - sizeof(int32_t); | ||
if (param_length > INT32_MAX) { |
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 can't get std::numeric_limits<int>::max()
to work here even with #define NOMINMAX
. It works in the Windows CI but not in the Python package build 🤷
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.
Weird.
You can use (max)()
to get around it too FWIW
Not sure I'll get to it anytime soon but I guess we need to stop borrowing the Arrow image (or check if there's a newer one to use) |
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.
Neat!
This PR refactors the
BindStream
to use the COPY writer instead of its own serialization logic. This logic is the same between insertion and binding, although the API used to send it to the database is slightly different. This is a helpful consolidation of logic and means that adding type support on the Arrow or Postgresql side (or fixing bugs in the inference or serialization) is slightly easier. It also means we can bind parameters that are lists and is a workaround for inserting into a table with a schema where Postgres knows how to cast the types (but ADBC might not yet).Created on 2024-09-12 with reprex v2.1.1