wgtmac commented on PR #780: URL: https://github.com/apache/iceberg-cpp/pull/780#issuecomment-4801093210
I think this is a good point to factor out a small internal nanoarrow builder/helper layer instead of adding another local copy of the same lifecycle code. We already have the same pattern in a few production paths: initialize from schema, start appending, append child values, finish each row/list/map element, then finish building. For example, `position_delete_writer.cc` and `arrow_c_data_util.cc` both guard the initialized `ArrowArray` immediately after `ArrowArrayInitFromSchema`, while this new builder currently misses that and can leak if `ArrowArrayStartAppending` fails. `manifest_adapter.cc` also has local primitive/list/map append helpers, including map-shape validation, and this PR adds another set of primitive/string-map helpers. A concrete way to keep this scoped would be: 1. Add an internal move-only nanoarrow array builder/owner that handles init/start/finish/release/ownership transfer. 2. Move the simple append helpers (`null`, signed/unsigned int, double, string, bytes, and maybe map entry validation) behind that internal helper. 3. Keep higher-level code like `ManifestAdapter`, batch projection, and metadata-table row materialization separate, since those have different domain semantics. That should fix the leak path here and avoid repeating this fragile nanoarrow lifecycle in the next metadata-table PRs without turning this into a broad refactor. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
