diff --git a/src/iceberg/arrow_c_data_guard_internal.h b/src/iceberg/arrow_c_data_guard_internal.h index f624f74c5..1b1417a95 100644 --- a/src/iceberg/arrow_c_data_guard_internal.h +++ b/src/iceberg/arrow_c_data_guard_internal.h @@ -46,6 +46,12 @@ class ICEBERG_EXPORT ArrowSchemaGuard { explicit ArrowSchemaGuard(ArrowSchema* schema) : schema_(schema) {} ~ArrowSchemaGuard(); + /// \brief Release the guard without calling ArrowSchemaRelease. + /// + /// Call this when ownership of the underlying ArrowSchema has been + /// transferred elsewhere and the guard should not release it. + void Release() { schema_ = nullptr; } + private: ArrowSchema* schema_; }; diff --git a/src/iceberg/schema_internal.cc b/src/iceberg/schema_internal.cc index 5dac0d3bf..810e53695 100644 --- a/src/iceberg/schema_internal.cc +++ b/src/iceberg/schema_internal.cc @@ -25,6 +25,7 @@ #include #include +#include "iceberg/arrow_c_data_guard_internal.h" #include "iceberg/constants.h" #include "iceberg/schema.h" #include "iceberg/type.h" @@ -71,6 +72,7 @@ ArrowErrorCode ToArrowSchema(const Type& type, bool optional, std::string_view n std::optional field_id, ArrowSchema* schema) { ArrowBuffer metadata_buffer; NANOARROW_RETURN_NOT_OK(ArrowMetadataBuilderInit(&metadata_buffer, nullptr)); + internal::ArrowArrayBufferGuard metadata_buffer_guard(&metadata_buffer); if (field_id.has_value()) { NANOARROW_RETURN_NOT_OK(ArrowMetadataBuilderAppend( &metadata_buffer, ArrowCharView(std::string(kParquetFieldIdKey).c_str()), @@ -183,7 +185,6 @@ ArrowErrorCode ToArrowSchema(const Type& type, bool optional, std::string_view n case TypeId::kVariant: case TypeId::kGeometry: case TypeId::kGeography: - ArrowBufferReset(&metadata_buffer); return EINVAL; } @@ -193,7 +194,6 @@ ArrowErrorCode ToArrowSchema(const Type& type, bool optional, std::string_view n NANOARROW_RETURN_NOT_OK(ArrowSchemaSetMetadata( schema, reinterpret_cast(metadata_buffer.data))); - ArrowBufferReset(&metadata_buffer); if (optional) { schema->flags |= ARROW_FLAG_NULLABLE; @@ -214,6 +214,7 @@ Status ToArrowSchema(const Schema& schema, ArrowSchema* out) { ICEBERG_RETURN_UNEXPECTED(CheckArrowCompatible(schema)); ArrowSchemaInit(out); + internal::ArrowSchemaGuard schema_guard(out); if (ArrowErrorCode errorCode = ToArrowSchema(schema, /*optional=*/false, /*name=*/"", /*field_id=*/std::nullopt, out); @@ -222,6 +223,7 @@ Status ToArrowSchema(const Schema& schema, ArrowSchema* out) { "Failed to convert Iceberg schema to Arrow schema, error code: {}", errorCode); } + schema_guard.Release(); return {}; } diff --git a/src/iceberg/test/arrow_test.cc b/src/iceberg/test/arrow_test.cc index d18a6eaf9..1ba95cdd1 100644 --- a/src/iceberg/test/arrow_test.cc +++ b/src/iceberg/test/arrow_test.cc @@ -130,12 +130,23 @@ TEST(ToArrowSchemaTest, UnsupportedV3Types) { Schema schema( {SchemaField::MakeOptional(/*field_id=*/1, "unsupported", unsupported_type)}, /*schema_id=*/0); - ArrowSchema arrow_schema; + ArrowSchema arrow_schema{}; ASSERT_THAT(ToArrowSchema(schema, &arrow_schema), HasErrorMessage("is not supported by Arrow conversion")); + EXPECT_EQ(arrow_schema.release, nullptr); } } +TEST(ToArrowSchemaTest, ReleasesSchemaOnConversionFailure) { + // fixed(0) passes CheckArrowCompatible but nanoarrow rejects its non-positive width. + Schema schema({SchemaField::MakeOptional(/*field_id=*/1, "invalid_fixed", fixed(0))}, + /*schema_id=*/0); + ArrowSchema arrow_schema{}; + + EXPECT_THAT(ToArrowSchema(schema, &arrow_schema), IsError(ErrorKind::kInvalidSchema)); + EXPECT_EQ(arrow_schema.release, nullptr); +} + namespace { void CheckArrowField(const ::arrow::Field& field, ::arrow::Type::type type_id,