fix: address PR #780 review comments

- Rename row_builder_internal.cc → arrow_row_builder.cc (no _internal suffix)
- Move files from inspect/ to core level; rename to arrow_row_builder*
- Merge NanoarrowArrayBuilder into ArrowRowBuilder as a single RAII class
- Add Make(const ArrowSchema*) overload for low-level callers
- Add ArrowArrayGuard::Release() and guard InitFromSchema in Make()
- Move test to arrow_row_builder_test.cc in data_test target

Author: WZhuo

src/iceberg/CMakeLists.txt

@@ -18,8 +18,9 @@
 set(ICEBERG_INCLUDES "$<BUILD_INTERFACE:${PROJECT_BINARY_DIR}/src>"
                      "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/src>")
 set(ICEBERG_SOURCES
-    arrow_c_data_util.cc
     arrow_c_data_guard_internal.cc
+    arrow_c_data_util.cc
+    arrow_row_builder.cc
     catalog/memory/in_memory_catalog.cc
     catalog/session_catalog.cc
     catalog/session_context.cc
@@ -45,7 +46,6 @@ set(ICEBERG_SOURCES
     file_writer.cc
     inspect/history_table.cc
     inspect/metadata_table.cc
-    inspect/row_builder_internal.cc
     inspect/snapshots_table.cc
     inheritable_metadata.cc
     json_serde.cc

src/iceberg/arrow_c_data_guard_internal.h

@@ -31,6 +31,12 @@ class ICEBERG_EXPORT ArrowArrayGuard {
   explicit ArrowArrayGuard(ArrowArray* array) : array_(array) {}
   ~ArrowArrayGuard();
 
+  /// \brief Release the guard without calling ArrowArrayRelease.
+  ///
+  /// Call this when ownership of the underlying ArrowArray has been
+  /// transferred elsewhere and the guard should not release it.
+  void Release() { array_ = nullptr; }
+
  private:
   ArrowArray* array_;
 };

…/iceberg/inspect/row_builder_internal.cc src/iceberg/arrow_row_builder.ccsrc/iceberg/inspect/row_builder_internal.cc renamed to src/iceberg/arrow_row_builder.cc src/iceberg/inspect/row_builder_internal.cc renamed to src/iceberg/arrow_row_builder.cc

@@ -17,14 +17,13 @@
  * under the License.
  */
 
-#include "iceberg/inspect/row_builder_internal.h"
-
 #include <utility>
 
 #include <nanoarrow/nanoarrow.h>
 
 #include "iceberg/arrow/nanoarrow_status_internal.h"
 #include "iceberg/arrow_c_data_guard_internal.h"
+#include "iceberg/arrow_row_builder_internal.h"
 #include "iceberg/schema.h"
 #include "iceberg/schema_internal.h"
 
@@ -34,60 +33,64 @@ Result<ArrowRowBuilder> ArrowRowBuilder::Make(const Schema& schema) {
   ArrowSchema arrow_schema;
   ICEBERG_RETURN_UNEXPECTED(ToArrowSchema(schema, &arrow_schema));
   internal::ArrowSchemaGuard schema_guard(&arrow_schema);
+  return Make(&arrow_schema);
+}
 
-  auto array = std::make_unique<ArrowArray>();
+Result<ArrowRowBuilder> ArrowRowBuilder::Make(const ArrowSchema* schema) {
+  ArrowRowBuilder builder;
   ArrowError error;
   ICEBERG_NANOARROW_RETURN_UNEXPECTED_WITH_ERROR(
-      ArrowArrayInitFromSchema(array.get(), &arrow_schema, &error), error);
-  ICEBERG_NANOARROW_RETURN_UNEXPECTED(ArrowArrayStartAppending(array.get()));
-
-  return ArrowRowBuilder(std::move(array));
+      ArrowArrayInitFromSchema(&builder.array_, schema, &error), error);
+  // Guard the array in case StartAppending fails.
+  internal::ArrowArrayGuard guard(&builder.array_);
+  ICEBERG_NANOARROW_RETURN_UNEXPECTED(ArrowArrayStartAppending(&builder.array_));
+  // Ownership stays with the builder — disarm the guard.
+  guard.Release();
+  return builder;
 }
 
-ArrowRowBuilder::ArrowRowBuilder(std::unique_ptr<ArrowArray>&& array) noexcept
-    : array_(std::move(array)) {}
-
 ArrowRowBuilder::ArrowRowBuilder(ArrowRowBuilder&& other) noexcept
-    : array_(std::move(other.array_)) {}
+    : array_(other.array_) {
+  other.array_.release = nullptr;
+}
 
 ArrowRowBuilder& ArrowRowBuilder::operator=(ArrowRowBuilder&& other) noexcept {
   if (this != &other) {
-    if (array_ != nullptr && array_->release != nullptr) {
-      ArrowArrayRelease(array_.get());
+    if (array_.release != nullptr) {
+      ArrowArrayRelease(&array_);
     }
-    array_ = std::move(other.array_);
+    array_ = other.array_;
+    other.array_.release = nullptr;
   }
   return *this;
 }
 
 ArrowRowBuilder::~ArrowRowBuilder() {
-  if (array_ != nullptr && array_->release != nullptr) {
-    ArrowArrayRelease(array_.get());
+  if (array_.release != nullptr) {
+    ArrowArrayRelease(&array_);
   }
 }
 
-int64_t ArrowRowBuilder::num_columns() const {
-  return array_ == nullptr ? 0 : array_->n_children;
-}
+int64_t ArrowRowBuilder::num_columns() const { return array_.n_children; }
 
 ArrowArray* ArrowRowBuilder::column(int64_t index) {
-  if (array_ == nullptr || index < 0 || index >= array_->n_children) {
+  if (index < 0 || index >= array_.n_children) {
     return nullptr;
   }
-  return array_->children[index];
+  return array_.children[index];
 }
 
 Status ArrowRowBuilder::FinishRow() {
-  ICEBERG_NANOARROW_RETURN_UNEXPECTED(ArrowArrayFinishElement(array_.get()));
+  ICEBERG_NANOARROW_RETURN_UNEXPECTED(ArrowArrayFinishElement(&array_));
   return {};
 }
 
 Result<ArrowArray> ArrowRowBuilder::Finish() && {
   ArrowError error;
   ICEBERG_NANOARROW_RETURN_UNEXPECTED_WITH_ERROR(
-      ArrowArrayFinishBuildingDefault(array_.get(), &error), error);
-  ArrowArray result = *array_;
-  array_->release = nullptr;
+      ArrowArrayFinishBuildingDefault(&array_, &error), error);
+  ArrowArray result = array_;
+  array_.release = nullptr;
   return result;
 }
 

…c/iceberg/inspect/row_builder_internal.h src/iceberg/arrow_row_builder_internal.hsrc/iceberg/inspect/row_builder_internal.h renamed to src/iceberg/arrow_row_builder_internal.h src/iceberg/inspect/row_builder_internal.h renamed to src/iceberg/arrow_row_builder_internal.h

@@ -19,18 +19,17 @@
 
 #pragma once
 
-/// \file iceberg/inspect/row_builder_internal.h
+/// \file iceberg/arrow_row_builder_internal.h
 /// Internal Arrow row-building utilities shared by metadata tables.
 ///
 /// Metadata tables (snapshots, history, manifests, ...) materialize in-memory
 /// structures into Arrow batches that conform to the table's Iceberg schema.
 /// `ArrowRowBuilder` wraps a nanoarrow `ArrowArray` initialized from such a
-/// schema and exposes per-column builders plus typed append helpers so each
+/// schema and exposes per-column access plus typed append helpers so each
 /// metadata table can emit rows without re-implementing the nanoarrow
 /// boilerplate.
 
 #include <cstdint>
-#include <memory>
 #include <string_view>
 #include <unordered_map>
 
@@ -41,7 +40,15 @@
 
 namespace iceberg {
 
-/// \brief Builds an Arrow struct array (a batch) for an arbitrary Iceberg schema.
+/// \brief Movable RAII builder that materializes rows into an Arrow struct array.
+///
+/// Handles the nanoarrow lifecycle: InitFromSchema → StartAppending →
+/// ... append values ... → FinishBuilding → Release.
+///
+/// Two constructors:
+/// - `Make(schema)` accepts an Iceberg Schema (typical for metadata tables).
+/// - `Make(arrow_schema)` accepts a raw ArrowSchema (for lower-level callers
+///   like position_delete_writer or manifest_adapter).
 ///
 /// Typical usage:
 /// \code
@@ -55,9 +62,15 @@ namespace iceberg {
 /// \endcode
 class ICEBERG_EXPORT ArrowRowBuilder {
  public:
-  /// \brief Create a row builder for the given Iceberg schema.
+  /// \brief Create a row builder from an Iceberg schema.
   static Result<ArrowRowBuilder> Make(const Schema& schema);
 
+  /// \brief Create a row builder from an ArrowSchema.
+  ///
+  /// The schema must outlive this call (the caller guards it). On failure the
+  /// partially-initialized array is released automatically.
+  static Result<ArrowRowBuilder> Make(const ArrowSchema* schema);
+
   ArrowRowBuilder(ArrowRowBuilder&& other) noexcept;
   ArrowRowBuilder& operator=(ArrowRowBuilder&& other) noexcept;
 
@@ -85,9 +98,8 @@ class ICEBERG_EXPORT ArrowRowBuilder {
   Result<ArrowArray> Finish() &&;
 
  private:
-  explicit ArrowRowBuilder(std::unique_ptr<ArrowArray>&& array) noexcept;
-
-  std::unique_ptr<ArrowArray> array_;
+  ArrowRowBuilder() = default;
+  ArrowArray array_{};
 };
 
 /// \brief Append a null to a nanoarrow array builder.

src/iceberg/meson.build

@@ -45,6 +45,7 @@ iceberg_include_dir = include_directories('..')
 iceberg_sources = files(
     'arrow_c_data_guard_internal.cc',
     'arrow_c_data_util.cc',
+    'arrow_row_builder.cc',
     'catalog/memory/in_memory_catalog.cc',
     'catalog/session_catalog.cc',
     'catalog/session_context.cc',
@@ -71,7 +72,6 @@ iceberg_sources = files(
     'inheritable_metadata.cc',
     'inspect/history_table.cc',
     'inspect/metadata_table.cc',
-    'inspect/row_builder_internal.cc',
     'inspect/snapshots_table.cc',
     'json_serde.cc',
     'location_provider.cc',

src/iceberg/test/CMakeLists.txt

@@ -184,11 +184,7 @@ if(ICEBERG_BUILD_BUNDLE)
 
   add_iceberg_test(catalog_test USE_BUNDLE SOURCES in_memory_catalog_test.cc)
 
-  add_iceberg_test(metadata_table_test
-                   USE_BUNDLE
-                   SOURCES
-                   metadata_table_test.cc
-                   row_builder_test.cc)
+  add_iceberg_test(metadata_table_test USE_BUNDLE SOURCES metadata_table_test.cc)
 
   add_iceberg_test(eval_expr_test
                    USE_BUNDLE
@@ -249,6 +245,7 @@ if(ICEBERG_BUILD_BUNDLE)
                    USE_BUNDLE
                    SOURCES
                    arrow_c_data_util_test.cc
+                   arrow_row_builder_test.cc
                    data_writer_test.cc
                    delete_filter_test.cc
                    delete_loader_test.cc

src/iceberg/test/row_builder_test.cc …c/iceberg/test/arrow_row_builder_test.ccsrc/iceberg/test/row_builder_test.cc renamed to src/iceberg/test/arrow_row_builder_test.cc src/iceberg/test/row_builder_test.cc renamed to src/iceberg/test/arrow_row_builder_test.cc

@@ -17,7 +17,7 @@
  * under the License.
  */
 
-/// \file row_builder_test.cc
+/// \file arrow_row_builder_test.cc
 /// Unit tests for ArrowRowBuilder and its typed append helpers.
 
 #include <memory>
@@ -28,7 +28,7 @@
 #include <arrow/record_batch.h>
 #include <gtest/gtest.h>
 
-#include "iceberg/inspect/row_builder_internal.h"
+#include "iceberg/arrow_row_builder_internal.h"
 #include "iceberg/schema.h"
 #include "iceberg/schema_field.h"
 #include "iceberg/schema_internal.h"
@@ -70,9 +70,7 @@ std::shared_ptr<::arrow::RecordBatch> FinishAndImport(ArrowRowBuilder builder,
 
 TEST(ArrowRowBuilderTest, BuildsRowsWithTypedValues) {
   auto schema = MakeTestSchema();
-  auto builder_result = ArrowRowBuilder::Make(*schema);
-  ASSERT_THAT(builder_result, IsOk());
-  auto builder = std::move(*builder_result);
+  ICEBERG_UNWRAP_OR_FAIL(auto builder, ArrowRowBuilder::Make(*schema));
 
   ASSERT_EQ(builder.num_columns(), 5);
 
@@ -122,9 +120,7 @@ TEST(ArrowRowBuilderTest, BuildsRowsWithTypedValues) {
 
 TEST(ArrowRowBuilderTest, AppendsNullForOptionalColumns) {
   auto schema = MakeTestSchema();
-  auto builder_result = ArrowRowBuilder::Make(*schema);
-  ASSERT_THAT(builder_result, IsOk());
-  auto builder = std::move(*builder_result);
+  ICEBERG_UNWRAP_OR_FAIL(auto builder, ArrowRowBuilder::Make(*schema));
 
   ASSERT_THAT(AppendInt(builder.column(0), 42), IsOk());
   ASSERT_THAT(AppendNull(builder.column(1)), IsOk());
@@ -147,9 +143,7 @@ TEST(ArrowRowBuilderTest, AppendsNullForOptionalColumns) {
 
 TEST(ArrowRowBuilderTest, AppendsMultiEntryStringMap) {
   auto schema = MakeTestSchema();
-  auto builder_result = ArrowRowBuilder::Make(*schema);
-  ASSERT_THAT(builder_result, IsOk());
-  auto builder = std::move(*builder_result);
+  ICEBERG_UNWRAP_OR_FAIL(auto builder, ArrowRowBuilder::Make(*schema));
 
   ASSERT_THAT(AppendInt(builder.column(0), 1), IsOk());
   ASSERT_THAT(AppendNull(builder.column(1)), IsOk());
@@ -166,19 +160,16 @@ TEST(ArrowRowBuilderTest, AppendsMultiEntryStringMap) {
 
 TEST(ArrowRowBuilderTest, EmptyBuilderProducesZeroRowBatch) {
   auto schema = MakeTestSchema();
-  auto builder_result = ArrowRowBuilder::Make(*schema);
-  ASSERT_THAT(builder_result, IsOk());
+  ICEBERG_UNWRAP_OR_FAIL(auto builder, ArrowRowBuilder::Make(*schema));
 
-  auto batch = FinishAndImport(std::move(*builder_result), *schema);
+  auto batch = FinishAndImport(std::move(builder), *schema);
   EXPECT_EQ(batch->num_rows(), 0);
   EXPECT_EQ(batch->num_columns(), 5);
 }
 
 TEST(ArrowRowBuilderTest, ColumnIndexOutOfRangeReturnsNull) {
   auto schema = MakeTestSchema();
-  auto builder_result = ArrowRowBuilder::Make(*schema);
-  ASSERT_THAT(builder_result, IsOk());
-  auto builder = std::move(*builder_result);
+  ICEBERG_UNWRAP_OR_FAIL(auto builder, ArrowRowBuilder::Make(*schema));
 
   EXPECT_EQ(builder.num_columns(), 5);
   EXPECT_NE(builder.column(0), nullptr);