diff --git a/src/iceberg/row/struct_like.cc b/src/iceberg/row/struct_like.cc index 430fd22f0..d49fa2747 100644 --- a/src/iceberg/row/struct_like.cc +++ b/src/iceberg/row/struct_like.cc @@ -76,39 +76,73 @@ Result LiteralToScalar(const Literal& literal) { } } +namespace { + +Result GetCheckedField(const StructLike& row, size_t pos, bool optional) { + ICEBERG_ASSIGN_OR_RAISE(auto field, row.GetField(pos)); + if (!optional && std::holds_alternative(field)) { + return InvalidArgument("Required field at position {} is null", pos); + } + return field; +} + +Result> GetParentStruct(const StructLike& row, size_t pos, + bool optional) { + ICEBERG_ASSIGN_OR_RAISE(auto field, GetCheckedField(row, pos, optional)); + if (std::holds_alternative(field)) return std::shared_ptr{}; + if (!std::holds_alternative>(field)) { + return InvalidSchema("Encountered non-struct at position {}", pos); + } + auto parent = std::get>(std::move(field)); + if (!parent && !optional) { + return InvalidArgument("Required field at position {} is null", pos); + } + return parent; +} + +} // namespace + StructLikeAccessor::StructLikeAccessor(std::shared_ptr type, - std::span position_path) - : type_(std::move(type)), position_path_(position_path.begin(), position_path.end()) { - if (position_path.size() == 1) { - accessor_ = [pos = - position_path[0]](const StructLike& struct_like) -> Result { - return struct_like.GetField(pos); + std::span position_path, + std::vector is_optional) + : type_(std::move(type)), + position_path_(position_path.begin(), position_path.end()), + is_optional_(std::move(is_optional)) { + if (position_path_.size() != is_optional_.size()) { + accessor_ = [](const StructLike&) -> Result { + return InvalidArgument("Optionality count does not match position path"); }; - } else if (position_path.size() == 2) { - accessor_ = [pos0 = position_path[0], pos1 = position_path[1]]( + return; + } + if (position_path_.size() == 1) { + accessor_ = [pos = position_path_[0], optional = static_cast(is_optional_[0])]( const StructLike& struct_like) -> Result { - ICEBERG_ASSIGN_OR_RAISE(auto first_level_field, struct_like.GetField(pos0)); - if (!std::holds_alternative>(first_level_field)) { - return InvalidSchema("Encountered non-struct in the position path [{},{}]", pos0, - pos1); - } - return std::get>(first_level_field)->GetField(pos1); + return GetCheckedField(struct_like, pos, optional); + }; + } else if (position_path_.size() == 2) { + accessor_ = [pos0 = position_path_[0], pos1 = position_path_[1], + optional = static_cast(is_optional_[0]), + leaf_optional = static_cast(is_optional_[1])]( + const StructLike& struct_like) -> Result { + ICEBERG_ASSIGN_OR_RAISE(auto nested, GetParentStruct(struct_like, pos0, optional)); + if (!nested) return Scalar{std::monostate{}}; + return GetCheckedField(*nested, pos1, leaf_optional); }; - } else if (!position_path.empty()) { + } else if (!position_path_.empty()) { accessor_ = [this](const StructLike& struct_like) -> Result { std::vector> backups; + backups.reserve(position_path_.size() - 1); const StructLike* current_struct_like = &struct_like; for (size_t i = 0; i < position_path_.size() - 1; ++i) { - ICEBERG_ASSIGN_OR_RAISE(auto field, - current_struct_like->GetField(position_path_[i])); - if (!std::holds_alternative>(field)) { - return InvalidSchema("Encountered non-struct in the position path [{}]", - position_path_); - } - backups.push_back(std::get>(field)); + ICEBERG_ASSIGN_OR_RAISE( + auto parent, + GetParentStruct(*current_struct_like, position_path_[i], is_optional_[i])); + if (!parent) return Scalar{std::monostate{}}; + backups.push_back(std::move(parent)); current_struct_like = backups.back().get(); } - return current_struct_like->GetField(position_path_.back()); + return GetCheckedField(*current_struct_like, position_path_.back(), + is_optional_.back()); }; } else { accessor_ = [](const StructLike&) -> Result { diff --git a/src/iceberg/row/struct_like.h b/src/iceberg/row/struct_like.h index 7b4bdc3be..77f84e233 100644 --- a/src/iceberg/row/struct_like.h +++ b/src/iceberg/row/struct_like.h @@ -31,6 +31,7 @@ #include #include #include +#include #include "iceberg/expression/literal.h" #include "iceberg/result.h" @@ -105,9 +106,11 @@ class ICEBERG_EXPORT MapLike { class ICEBERG_EXPORT StructLikeAccessor { public: explicit StructLikeAccessor(std::shared_ptr type, - std::span position_path); + std::span position_path, + std::vector is_optional); /// \brief Get the scalar value at the given position. + /// A null optional parent returns monostate; a null required field is an error. Result Get(const StructLike& struct_like) const { return accessor_(struct_like); } @@ -121,13 +124,15 @@ class ICEBERG_EXPORT StructLikeAccessor { /// \brief Get the type of the value that this accessor is bound to. const Type& type() const { return *type_; } - /// \brief Get the position path of the value that this accessor bounded to. + /// \brief Get the position path of the value that this accessor is bound to. const std::vector& position_path() const { return position_path_; } private: std::shared_ptr type_; std::function(const StructLike&)> accessor_; std::vector position_path_; + // One entry per position, including the leaf field. + std::vector is_optional_; }; } // namespace iceberg diff --git a/src/iceberg/schema.cc b/src/iceberg/schema.cc index 24df69103..8f103a9d1 100644 --- a/src/iceberg/schema.cc +++ b/src/iceberg/schema.cc @@ -357,7 +357,18 @@ Result> Schema::GetAccessorById( if (!field.has_value()) { return NotFound("Cannot get accessor for field id: {}", field_id); } - return std::make_unique(field.value().get().type(), it->second); + std::vector is_optional; + is_optional.reserve(it->second.size()); + const StructType* current = this; + for (size_t i = 0; i < it->second.size(); ++i) { + const auto& field_at_position = current->fields()[it->second[i]]; + is_optional.push_back(field_at_position.optional()); + if (i + 1 < it->second.size()) { + current = static_cast(field_at_position.type().get()); + } + } + return std::make_unique(field.value().get().type(), it->second, + std::move(is_optional)); } return NotFound("Cannot get accessor for field id: {}", field_id); } diff --git a/src/iceberg/test/evaluator_test.cc b/src/iceberg/test/evaluator_test.cc index 798f41e1f..e365a7d3e 100644 --- a/src/iceberg/test/evaluator_test.cc +++ b/src/iceberg/test/evaluator_test.cc @@ -330,7 +330,7 @@ TEST_F(EvaluatorTest, NotEqual) { TEST_F(EvaluatorTest, StartsWith) { auto string_schema = std::make_unique( - std::vector{SchemaField::MakeRequired(24, "s", string())}); + std::vector{SchemaField::MakeOptional(24, "s", string())}); ICEBERG_UNWRAP_OR_FAIL( auto evaluator, Evaluator::Make(*string_schema, Expressions::StartsWith("s", "abc"))); diff --git a/src/iceberg/test/struct_like_test.cc b/src/iceberg/test/struct_like_test.cc index ad19d6a09..a57860116 100644 --- a/src/iceberg/test/struct_like_test.cc +++ b/src/iceberg/test/struct_like_test.cc @@ -20,7 +20,10 @@ #include "iceberg/row/struct_like.h" #include +#include +#include #include +#include #include #include @@ -71,6 +74,7 @@ namespace { class SingleFieldStructLike : public StructLike { public: explicit SingleFieldStructLike(Scalar value) : value_(std::move(value)) {} + explicit SingleFieldStructLike(Result value) : value_(std::move(value)) {} Result GetField(size_t pos) const override { if (pos != 0) { @@ -82,9 +86,35 @@ class SingleFieldStructLike : public StructLike { size_t num_fields() const override { return 1; } private: - Scalar value_; + Result value_; }; +std::shared_ptr WrapSingleFieldRow(Result field, size_t depth) { + std::shared_ptr row = + std::make_shared(std::move(field)); + for (size_t i = 1; i < depth; ++i) { + row = std::make_shared(Scalar{row}); + } + return row; +} + +std::unique_ptr MakeNestedSchema( + size_t depth, std::optional optional_parent = std::nullopt, + bool optional_leaf = false) { + auto field = + optional_leaf + ? SchemaField::MakeOptional(static_cast(depth), "value", int32()) + : SchemaField::MakeRequired(static_cast(depth), "value", int32()); + for (size_t level = depth - 1; level > 0; --level) { + auto nested = struct_({field}); + field = + optional_parent == level - 1 + ? SchemaField::MakeOptional(static_cast(level), "nested", nested) + : SchemaField::MakeRequired(static_cast(level), "nested", nested); + } + return std::make_unique(std::vector{std::move(field)}); +} + } // namespace TEST(ManifestFileStructLike, BasicFields) { @@ -152,7 +182,7 @@ TEST(StructLikeAccessorTest, GetLiteralUuid) { std::string_view uuid_data(reinterpret_cast(bytes.data()), bytes.size()); SingleFieldStructLike row(Scalar{uuid_data}); std::array path = {0}; - StructLikeAccessor accessor(iceberg::uuid(), path); + StructLikeAccessor accessor(iceberg::uuid(), path, {false}); ICEBERG_UNWRAP_OR_FAIL(auto literal, accessor.GetLiteral(row)); EXPECT_EQ(literal.type()->type_id(), TypeId::kUuid); @@ -163,13 +193,138 @@ TEST(StructLikeAccessorTest, GetLiteralUuid) { TEST(StructLikeAccessorTest, GetLiteralUuidRejectsWrongLength) { SingleFieldStructLike row(Scalar{std::string_view("not-a-uuid")}); std::array path = {0}; - StructLikeAccessor accessor(iceberg::uuid(), path); + StructLikeAccessor accessor(iceberg::uuid(), path, {false}); auto result = accessor.GetLiteral(row); EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument)); EXPECT_THAT(result, HasErrorMessage("UUID byte array must be exactly 16 bytes")); } +TEST(StructLikeAccessorTest, RequiredAndOptionalLeaf) { + SingleFieldStructLike row(Scalar{std::monostate{}}); + for (bool optional : {false, true}) { + auto schema = MakeNestedSchema(1, std::nullopt, optional); + ICEBERG_UNWRAP_OR_FAIL(auto accessor, schema->GetAccessorById(1)); + if (optional) { + EXPECT_SCALAR_NULL(accessor->Get(row)); + ICEBERG_UNWRAP_OR_FAIL(auto literal, accessor->GetLiteral(row)); + EXPECT_TRUE(literal.IsNull()); + } else { + EXPECT_THAT(accessor->Get(row), IsError(ErrorKind::kInvalidArgument)); + EXPECT_THAT(accessor->GetLiteral(row), IsError(ErrorKind::kInvalidArgument)); + } + } +} + +class NestedStructLikeAccessorTest : public ::testing::TestWithParam {}; + +TEST_P(NestedStructLikeAccessorTest, NullParents) { + const std::vector path(GetParam(), 0); + StructLikeAccessor accessor(int32(), path, std::vector(path.size(), true)); + for (const Scalar null : + {Scalar{std::monostate{}}, Scalar{std::shared_ptr{}}}) { + for (size_t parent = 0; parent + 1 < path.size(); ++parent) { + SCOPED_TRACE(parent); + auto row = WrapSingleFieldRow(null, parent + 1); + ICEBERG_UNWRAP_OR_FAIL(auto scalar, accessor.Get(*row)); + EXPECT_TRUE(std::holds_alternative(scalar)); + ICEBERG_UNWRAP_OR_FAIL(auto literal, accessor.GetLiteral(*row)); + EXPECT_TRUE(literal.IsNull()); + EXPECT_EQ(literal.type()->type_id(), TypeId::kInt); + } + } +} + +TEST_P(NestedStructLikeAccessorTest, SchemaRequiredAndOptionalParents) { + const auto depth = GetParam(); + auto required_schema = MakeNestedSchema(depth); + ICEBERG_UNWRAP_OR_FAIL(auto required_accessor, + required_schema->GetAccessorById(static_cast(depth))); + EXPECT_EQ(required_accessor->position_path(), std::vector(depth, 0)); + + for (const Scalar null : + {Scalar{std::monostate{}}, Scalar{std::shared_ptr{}}}) { + for (size_t parent = 0; parent + 1 < depth; ++parent) { + SCOPED_TRACE(parent); + auto row = WrapSingleFieldRow(null, parent + 1); + auto schema = MakeNestedSchema(depth, parent); + ICEBERG_UNWRAP_OR_FAIL(auto accessor, + schema->GetAccessorById(static_cast(depth))); + EXPECT_SCALAR_NULL(accessor->Get(*row)); + ICEBERG_UNWRAP_OR_FAIL(auto literal, accessor->GetLiteral(*row)); + EXPECT_TRUE(literal.IsNull()); + EXPECT_EQ(literal.type()->type_id(), TypeId::kInt); + + EXPECT_THAT(required_accessor->Get(*row), IsError(ErrorKind::kInvalidArgument)); + EXPECT_THAT(required_accessor->GetLiteral(*row), + IsError(ErrorKind::kInvalidArgument)); + } + } +} + +TEST_P(NestedStructLikeAccessorTest, ValueAndNullLeaf) { + const std::vector path(GetParam(), 0); + StructLikeAccessor accessor(int32(), path, std::vector(path.size(), true)); + auto row = WrapSingleFieldRow(Scalar{int32_t{42}}, path.size()); + ICEBERG_UNWRAP_OR_FAIL(auto value, accessor.GetLiteral(*row)); + EXPECT_EQ(value, Literal::Int(42)); + + auto null_row = WrapSingleFieldRow(Scalar{std::monostate{}}, path.size()); + ICEBERG_UNWRAP_OR_FAIL(auto null, accessor.GetLiteral(*null_row)); + EXPECT_TRUE(null.IsNull()); + EXPECT_EQ(null.type()->type_id(), TypeId::kInt); +} + +TEST_P(NestedStructLikeAccessorTest, RequiredAndOptionalLeaf) { + const auto depth = GetParam(); + auto row = WrapSingleFieldRow(Scalar{std::monostate{}}, depth); + for (bool optional : {false, true}) { + auto schema = MakeNestedSchema(depth, std::nullopt, optional); + ICEBERG_UNWRAP_OR_FAIL(auto accessor, + schema->GetAccessorById(static_cast(depth))); + if (optional) { + EXPECT_SCALAR_NULL(accessor->Get(*row)); + ICEBERG_UNWRAP_OR_FAIL(auto literal, accessor->GetLiteral(*row)); + EXPECT_TRUE(literal.IsNull()); + } else { + EXPECT_THAT(accessor->Get(*row), IsError(ErrorKind::kInvalidArgument)); + EXPECT_THAT(accessor->GetLiteral(*row), IsError(ErrorKind::kInvalidArgument)); + } + } + + auto schema = MakeNestedSchema(depth, 0); + ICEBERG_UNWRAP_OR_FAIL(auto accessor, + schema->GetAccessorById(static_cast(depth))); + EXPECT_THAT(accessor->Get(*row), IsError(ErrorKind::kInvalidArgument)); + auto absent_parent = WrapSingleFieldRow(Scalar{std::monostate{}}, 1); + EXPECT_SCALAR_NULL(accessor->Get(*absent_parent)); +} + +TEST_P(NestedStructLikeAccessorTest, RejectsNonStructParents) { + const std::vector path(GetParam(), 0); + StructLikeAccessor accessor(int32(), path, std::vector(path.size(), true)); + for (size_t parent = 0; parent + 1 < path.size(); ++parent) { + SCOPED_TRACE(parent); + auto row = WrapSingleFieldRow(Scalar{int32_t{42}}, parent + 1); + EXPECT_THAT(accessor.Get(*row), IsError(ErrorKind::kInvalidSchema)); + } +} + +TEST_P(NestedStructLikeAccessorTest, PreservesFieldErrors) { + const std::vector path(GetParam(), 0); + StructLikeAccessor accessor(int32(), path, std::vector(path.size(), true)); + for (size_t level = 0; level < path.size(); ++level) { + SCOPED_TRACE(level); + auto row = WrapSingleFieldRow(IOError("Cannot read field"), level + 1); + auto result = accessor.GetLiteral(*row); + EXPECT_THAT(result, IsError(ErrorKind::kIOError)); + EXPECT_THAT(result, HasErrorMessage("Cannot read field")); + } +} + +INSTANTIATE_TEST_SUITE_P(NestedPaths, NestedStructLikeAccessorTest, + ::testing::Values(size_t{2}, size_t{3}, size_t{4})); + TEST(ManifestFileStructLike, OptionalFields) { ManifestFile manifest_file{.manifest_path = "/path/to/manifest2.avro", .manifest_length = 54321,