-
Notifications
You must be signed in to change notification settings - Fork 85
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
[C] Basic libpq-based driver #65
Conversation
f9ec9ac
to
677ae23
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.
Just musing at ways I could make this easier to do!
c/drivers/postgres/statement.cc
Outdated
} | ||
if (schema_.release) { | ||
schema_.release(&schema_); | ||
schema_.release = nullptr; |
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.
This is redundant, no?
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.
Fixed - thanks for catching that!
c/drivers/postgres/statement.cc
Outdated
|
||
const int num_rows = PQntuples(result_); | ||
|
||
NA_RETURN_NOT_OK(ArrowArrayInit(out, NANOARROW_TYPE_STRUCT)); |
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 should probably add ArrowArrayInitFromSchema()
(to match the recently added ArrowArrayViewInitFromSchema()
)
c/drivers/postgres/statement.cc
Outdated
} | ||
|
||
for (int col = 0; col < schema_.n_children; col++) { | ||
NA_RETURN_NOT_OK(ArrowArrayFinishBuilding(out->children[col], 0)); |
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.
After the last PR, calling ArrowArrayFinishBuilding()
on the parent array will propagate to child arrays
c/drivers/postgres/statement.cc
Outdated
|
||
std::vector<struct ArrowSchemaView> fields(schema_.n_children); | ||
|
||
for (int col = 0; col < schema_.n_children; col++) { |
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 think I have enough general infrastructure now to make a generic ArrowArrayReserve()
that seems like it would cut down a bit of verbosity here
75bebaa
to
c8257c2
Compare
Rebased/updated. This now tries to use the COPY command, taking advantage of the changes in #69. Things that need to happen next:
|
c8257c2
to
c90fc91
Compare
result_ = nullptr; | ||
|
||
struct ArrowError error; | ||
// TODO: consistently release out on error (use another trampoline?) |
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.
This seems like it's going to be a common pattern...should nanoarrow provide a C++ header for this?
class ArrayHolder {
public:
struct ArrowArray array;
ArrayHolder() { array.release = nullptr; }
~ArrayHolder() { if (array.release) array.release(&array); }
}
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.
Possibly, I was also going to file an issue/PR for stringifying type codes, etc. to use in error messages
c/drivers/postgres/statement.cc
Outdated
|
||
std::vector<struct ArrowArrayView> views(fields.size()); | ||
for (size_t i = 0; i < views.size(); i++) { | ||
ArrowArrayViewInit(&views[i], fields[i].data_type); |
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.
You may want to ArrowArrayViewInitFromSchema(&array_view, &schema)
once on the top level and then do ArrowArrayViewSetArray(&array_view, &array)
in the loop. (Then you can do array_view.children[i].stuff
).
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 think you're missing a ArrowArrayViewReset()
, too (although it won't leak memory unless the array has children).
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, I also did a bit of templating and implemented the ArrayHolder above (slightly more generically)
c/drivers/postgres/statement.cc
Outdated
|
||
schema.release(&schema); | ||
bind_.release(&bind_); | ||
bind_.release = nullptr; |
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.
Is this line necessary?
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'm mostly trying to be defensive 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.
I don't really mind either way, although I personally find it a bit confusing since it's a MUST in the spec that the release method has to take care of that.
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.
Ah, right. I'll update it then.
e5c1751
to
9081851
Compare
9081851
to
9873d3e
Compare
The driver supports basic queries (int32 only) and toggling autocommit. It does not yet support bulk ingestion or prepared statements.
It hasn't been optimized for speed and the approach taken here will not be fast (it uses the per-row getters). In future PRs, we should set up some benchmarks and then see if DuckDB's approach makes more sense (use
COPY
). DuckDB also does multithreading (that might be hard for us). We may want to implement #61 first since then we will know whether it is safe to useCOPY
or not.