diff --git a/src/iceberg/schema_internal.cc b/src/iceberg/schema_internal.cc index 5dac0d3bf..087d90a90 100644 --- a/src/iceberg/schema_internal.cc +++ b/src/iceberg/schema_internal.cc @@ -19,8 +19,6 @@ #include "iceberg/schema_internal.h" -#include -#include #include #include #include @@ -29,6 +27,7 @@ #include "iceberg/schema.h" #include "iceberg/type.h" #include "iceberg/util/macros.h" +#include "iceberg/util/string_util.h" namespace iceberg { @@ -227,7 +226,7 @@ Status ToArrowSchema(const Schema& schema, ArrowSchema* out) { namespace { -int32_t GetFieldId(const ArrowSchema& schema) { +Result GetFieldId(const ArrowSchema& schema) { if (schema.metadata == nullptr) { return kUnknownFieldId; } @@ -240,11 +239,14 @@ int32_t GetFieldId(const ArrowSchema& schema) { return kUnknownFieldId; } - int32_t field_id = kUnknownFieldId; - std::from_chars(field_id_value.data, field_id_value.data + field_id_value.size_bytes, - field_id); + std::string_view field_id(field_id_value.data, field_id_value.size_bytes); + auto field_id_result = StringUtils::ParseNumber(field_id); + if (!field_id_result.has_value()) { + return InvalidSchema( + "Invalid Arrow field ID: '{}'", field_id); + } - return field_id; + return field_id_result.value(); } Result> FromArrowSchema(const ArrowSchema& schema) { @@ -252,7 +254,7 @@ Result> FromArrowSchema(const ArrowSchema& schema) { [](const ArrowSchema& schema) -> Result> { ICEBERG_ASSIGN_OR_RAISE(auto field_type, FromArrowSchema(schema)); - auto field_id = GetFieldId(schema); + ICEBERG_ASSIGN_OR_RAISE(auto field_id, GetFieldId(schema)); bool is_optional = (schema.flags & ARROW_FLAG_NULLABLE) != 0; if (field_type->type_id() == TypeId::kUnknown && !is_optional) { return InvalidSchema("Arrow null field '{}' must be nullable", schema.name); diff --git a/src/iceberg/test/arrow_test.cc b/src/iceberg/test/arrow_test.cc index d18a6eaf9..3450076d1 100644 --- a/src/iceberg/test/arrow_test.cc +++ b/src/iceberg/test/arrow_test.cc @@ -371,6 +371,25 @@ TEST_P(FromArrowSchemaTest, PrimitiveType) { ASSERT_EQ(*field.type(), *param.iceberg_type); } +TEST(FromArrowSchemaTest, RejectMalformedFieldIdMetadata) { + for (const auto& field_id : {"1x", "2147483648", ""}) { + auto metadata = + ::arrow::key_value_metadata(std::unordered_map{ + {std::string(kParquetFieldIdKey), field_id}}); + auto arrow_schema = ::arrow::schema({::arrow::field( + "foo", ::arrow::int32(), /*nullable=*/true, std::move(metadata))}); + ArrowSchema exported_schema; + ASSERT_TRUE(::arrow::ExportSchema(*arrow_schema, &exported_schema).ok()); + + auto result = FromArrowSchema(exported_schema, /*schema_id=*/1); + ArrowSchemaRelease(&exported_schema); + + EXPECT_THAT(result, IsError(ErrorKind::kInvalidSchema)); + EXPECT_THAT(result, + HasErrorMessage(std::format("Invalid Arrow field ID: '{}'", field_id))); + } +} + INSTANTIATE_TEST_SUITE_P( SchemaConversion, FromArrowSchemaTest, ::testing::Values(