feat(c/driver/postgresql): Implement LIST/LARGE_LIST Writer #1962
Conversation
| @@ -105,9 +105,7 @@ class PostgresCopyFieldWriter { | |||
| class PostgresCopyFieldTupleWriter : public PostgresCopyFieldWriter { | |||
| public: | |||
| void AppendChild(std::unique_ptr<PostgresCopyFieldWriter> child) { | |||
| int64_t child_i = static_cast<int64_t>(children_.size()); | |||
There was a problem hiding this comment.
The current design makes it so that the array_view is limited to being supplied to the writers through this AppendChild function; however, to support arbitrarily nested writers I think it makes more sense to be moved outside of this
The flip side is now there are a lot of writer.Init() calls placed in the factory function. I also tried to just make the ArrayView part of the constructor for each writer, but that became kind of strange when trying to create the root writer itself.
Probably a better way to structure/refactor this, but decided to live with the code duplication for now to focus on the rest of implementation
c/driver/postgresql/copy/writer.h
Outdated
| ArrowSchemaViewInit(&child_schema_view, schema->children[0], error)); | ||
| switch (child_schema_view.type) { | ||
| case NANOARROW_TYPE_INT32: { | ||
| // TODO: likely need to make type_resolver available here |
There was a problem hiding this comment.
Hinted at in current comment, but I think we need a way to forward the resolver or connection to this part of the code to get this to work
WillAyd
changed the title
c/driver/postgresql/copy/writer.h
Outdated
| @@ -663,15 +766,15 @@ class PostgresCopyStreamWriter { | |||
| return NANOARROW_OK; | |||
| } | |||
|
|
|||
| ArrowErrorCode InitFieldWriters(ArrowError* error) { | |||
| ArrowErrorCode InitFieldWriters(const PostgresConnection* conn, ArrowError* error) { | |||
There was a problem hiding this comment.
nit but should we pass in the whole connection or maybe just const Resolver&?
There was a problem hiding this comment.
That might also simplify the amount of setup you have to do in the unit test?
…on (#1998) This is a follow up to #1962, which required each writer construction to subsequently call `->Init()` to assign to a protected array_view_ member. This takes inspiration from the following cpp guideline to have the class manage its construction more appropriately: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#discussion-use-a-factory-function-if-you-need-virtual-behavior-during-initialization
closes #1882