Skip to content

Commit 4b9ff79

Browse files
authored
fix(arrow): release schema after conversion failure (#862)
Release partially initialized ArrowSchema output when internal Iceberg-to-Arrow conversion fails. Add coverage using fixed(0), which passes compatibility validation but is rejected by nanoarrow, and verify the output release callback is cleared. Tests: arrow_test.
1 parent bba15b2 commit 4b9ff79

3 files changed

Lines changed: 22 additions & 3 deletions

File tree

src/iceberg/arrow_c_data_guard_internal.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,12 @@ class ICEBERG_EXPORT ArrowSchemaGuard {
4646
explicit ArrowSchemaGuard(ArrowSchema* schema) : schema_(schema) {}
4747
~ArrowSchemaGuard();
4848

49+
/// \brief Release the guard without calling ArrowSchemaRelease.
50+
///
51+
/// Call this when ownership of the underlying ArrowSchema has been
52+
/// transferred elsewhere and the guard should not release it.
53+
void Release() { schema_ = nullptr; }
54+
4955
private:
5056
ArrowSchema* schema_;
5157
};

src/iceberg/schema_internal.cc

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
#include <optional>
2626
#include <string>
2727

28+
#include "iceberg/arrow_c_data_guard_internal.h"
2829
#include "iceberg/constants.h"
2930
#include "iceberg/schema.h"
3031
#include "iceberg/type.h"
@@ -71,6 +72,7 @@ ArrowErrorCode ToArrowSchema(const Type& type, bool optional, std::string_view n
7172
std::optional<int32_t> field_id, ArrowSchema* schema) {
7273
ArrowBuffer metadata_buffer;
7374
NANOARROW_RETURN_NOT_OK(ArrowMetadataBuilderInit(&metadata_buffer, nullptr));
75+
internal::ArrowArrayBufferGuard metadata_buffer_guard(&metadata_buffer);
7476
if (field_id.has_value()) {
7577
NANOARROW_RETURN_NOT_OK(ArrowMetadataBuilderAppend(
7678
&metadata_buffer, ArrowCharView(std::string(kParquetFieldIdKey).c_str()),
@@ -183,7 +185,6 @@ ArrowErrorCode ToArrowSchema(const Type& type, bool optional, std::string_view n
183185
case TypeId::kVariant:
184186
case TypeId::kGeometry:
185187
case TypeId::kGeography:
186-
ArrowBufferReset(&metadata_buffer);
187188
return EINVAL;
188189
}
189190

@@ -193,7 +194,6 @@ ArrowErrorCode ToArrowSchema(const Type& type, bool optional, std::string_view n
193194

194195
NANOARROW_RETURN_NOT_OK(ArrowSchemaSetMetadata(
195196
schema, reinterpret_cast<const char*>(metadata_buffer.data)));
196-
ArrowBufferReset(&metadata_buffer);
197197

198198
if (optional) {
199199
schema->flags |= ARROW_FLAG_NULLABLE;
@@ -214,6 +214,7 @@ Status ToArrowSchema(const Schema& schema, ArrowSchema* out) {
214214
ICEBERG_RETURN_UNEXPECTED(CheckArrowCompatible(schema));
215215

216216
ArrowSchemaInit(out);
217+
internal::ArrowSchemaGuard schema_guard(out);
217218

218219
if (ArrowErrorCode errorCode = ToArrowSchema(schema, /*optional=*/false, /*name=*/"",
219220
/*field_id=*/std::nullopt, out);
@@ -222,6 +223,7 @@ Status ToArrowSchema(const Schema& schema, ArrowSchema* out) {
222223
"Failed to convert Iceberg schema to Arrow schema, error code: {}", errorCode);
223224
}
224225

226+
schema_guard.Release();
225227
return {};
226228
}
227229

src/iceberg/test/arrow_test.cc

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,12 +130,23 @@ TEST(ToArrowSchemaTest, UnsupportedV3Types) {
130130
Schema schema(
131131
{SchemaField::MakeOptional(/*field_id=*/1, "unsupported", unsupported_type)},
132132
/*schema_id=*/0);
133-
ArrowSchema arrow_schema;
133+
ArrowSchema arrow_schema{};
134134
ASSERT_THAT(ToArrowSchema(schema, &arrow_schema),
135135
HasErrorMessage("is not supported by Arrow conversion"));
136+
EXPECT_EQ(arrow_schema.release, nullptr);
136137
}
137138
}
138139

140+
TEST(ToArrowSchemaTest, ReleasesSchemaOnConversionFailure) {
141+
// fixed(0) passes CheckArrowCompatible but nanoarrow rejects its non-positive width.
142+
Schema schema({SchemaField::MakeOptional(/*field_id=*/1, "invalid_fixed", fixed(0))},
143+
/*schema_id=*/0);
144+
ArrowSchema arrow_schema{};
145+
146+
EXPECT_THAT(ToArrowSchema(schema, &arrow_schema), IsError(ErrorKind::kInvalidSchema));
147+
EXPECT_EQ(arrow_schema.release, nullptr);
148+
}
149+
139150
namespace {
140151

141152
void CheckArrowField(const ::arrow::Field& field, ::arrow::Type::type type_id,

0 commit comments

Comments
 (0)