This is an automated email from the ASF dual-hosted git repository.
wgtmac pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-cpp.git
The following commit(s) were added to refs/heads/main by this push:
new 66d7ef5c fix: respect nullability in nested struct accessors (#967)
66d7ef5c is described below
commit 66d7ef5c75db7fbdb710d115a5ad52fc12eb677c
Author: advancedxy <[email protected]>
AuthorDate: Sat Oct 10 13:51:50 2026 +0800
fix: respect nullability in nested struct accessors (#967)
---
src/iceberg/row/struct_like.cc | 80 ++++++++++++-----
src/iceberg/row/struct_like.h | 9 +-
src/iceberg/schema.cc | 13 ++-
src/iceberg/test/evaluator_test.cc | 2 +-
src/iceberg/test/struct_like_test.cc | 161 ++++++++++++++++++++++++++++++++++-
5 files changed, 235 insertions(+), 30 deletions(-)
diff --git a/src/iceberg/row/struct_like.cc b/src/iceberg/row/struct_like.cc
index 430fd22f..d49fa274 100644
--- a/src/iceberg/row/struct_like.cc
+++ b/src/iceberg/row/struct_like.cc
@@ -76,39 +76,73 @@ Result<Scalar> LiteralToScalar(const Literal& literal) {
}
}
+namespace {
+
+Result<Scalar> GetCheckedField(const StructLike& row, size_t pos, bool
optional) {
+ ICEBERG_ASSIGN_OR_RAISE(auto field, row.GetField(pos));
+ if (!optional && std::holds_alternative<std::monostate>(field)) {
+ return InvalidArgument("Required field at position {} is null", pos);
+ }
+ return field;
+}
+
+Result<std::shared_ptr<StructLike>> GetParentStruct(const StructLike& row,
size_t pos,
+ bool optional) {
+ ICEBERG_ASSIGN_OR_RAISE(auto field, GetCheckedField(row, pos, optional));
+ if (std::holds_alternative<std::monostate>(field)) return
std::shared_ptr<StructLike>{};
+ if (!std::holds_alternative<std::shared_ptr<StructLike>>(field)) {
+ return InvalidSchema("Encountered non-struct at position {}", pos);
+ }
+ auto parent = std::get<std::shared_ptr<StructLike>>(std::move(field));
+ if (!parent && !optional) {
+ return InvalidArgument("Required field at position {} is null", pos);
+ }
+ return parent;
+}
+
+} // namespace
+
StructLikeAccessor::StructLikeAccessor(std::shared_ptr<Type> type,
- std::span<const size_t> 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<Scalar> {
- return struct_like.GetField(pos);
+ std::span<const size_t> position_path,
+ std::vector<bool> 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<Scalar> {
+ 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<bool>(is_optional_[0])](
const StructLike& struct_like) -> Result<Scalar> {
- ICEBERG_ASSIGN_OR_RAISE(auto first_level_field,
struct_like.GetField(pos0));
- if
(!std::holds_alternative<std::shared_ptr<StructLike>>(first_level_field)) {
- return InvalidSchema("Encountered non-struct in the position path
[{},{}]", pos0,
- pos1);
- }
- return
std::get<std::shared_ptr<StructLike>>(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<bool>(is_optional_[0]),
+ leaf_optional = static_cast<bool>(is_optional_[1])](
+ const StructLike& struct_like) -> Result<Scalar> {
+ 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<Scalar> {
std::vector<std::shared_ptr<StructLike>> 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<std::shared_ptr<StructLike>>(field)) {
- return InvalidSchema("Encountered non-struct in the position path
[{}]",
- position_path_);
- }
- backups.push_back(std::get<std::shared_ptr<StructLike>>(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<Scalar> {
diff --git a/src/iceberg/row/struct_like.h b/src/iceberg/row/struct_like.h
index 7b4bdc3b..77f84e23 100644
--- a/src/iceberg/row/struct_like.h
+++ b/src/iceberg/row/struct_like.h
@@ -31,6 +31,7 @@
#include <span>
#include <string_view>
#include <variant>
+#include <vector>
#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> type,
- std::span<const size_t> position_path);
+ std::span<const size_t> position_path,
+ std::vector<bool> 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<Scalar> 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<size_t>& position_path() const { return position_path_; }
private:
std::shared_ptr<Type> type_;
std::function<Result<Scalar>(const StructLike&)> accessor_;
std::vector<size_t> position_path_;
+ // One entry per position, including the leaf field.
+ std::vector<bool> is_optional_;
};
} // namespace iceberg
diff --git a/src/iceberg/schema.cc b/src/iceberg/schema.cc
index 24df6910..8f103a9d 100644
--- a/src/iceberg/schema.cc
+++ b/src/iceberg/schema.cc
@@ -357,7 +357,18 @@ Result<std::unique_ptr<StructLikeAccessor>>
Schema::GetAccessorById(
if (!field.has_value()) {
return NotFound("Cannot get accessor for field id: {}", field_id);
}
- return std::make_unique<StructLikeAccessor>(field.value().get().type(),
it->second);
+ std::vector<bool> 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<const
StructType*>(field_at_position.type().get());
+ }
+ }
+ return std::make_unique<StructLikeAccessor>(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 798f41e1..e365a7d3 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<Schema>(
- std::vector<SchemaField>{SchemaField::MakeRequired(24, "s", string())});
+ std::vector<SchemaField>{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 ad19d6a0..a5786011 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 <array>
+#include <memory>
+#include <optional>
#include <utility>
+#include <vector>
#include <arrow/c/bridge.h>
#include <arrow/json/from_string.h>
@@ -71,6 +74,7 @@ namespace {
class SingleFieldStructLike : public StructLike {
public:
explicit SingleFieldStructLike(Scalar value) : value_(std::move(value)) {}
+ explicit SingleFieldStructLike(Result<Scalar> value) :
value_(std::move(value)) {}
Result<Scalar> 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<Scalar> value_;
};
+std::shared_ptr<StructLike> WrapSingleFieldRow(Result<Scalar> field, size_t
depth) {
+ std::shared_ptr<StructLike> row =
+ std::make_shared<SingleFieldStructLike>(std::move(field));
+ for (size_t i = 1; i < depth; ++i) {
+ row = std::make_shared<SingleFieldStructLike>(Scalar{row});
+ }
+ return row;
+}
+
+std::unique_ptr<Schema> MakeNestedSchema(
+ size_t depth, std::optional<size_t> optional_parent = std::nullopt,
+ bool optional_leaf = false) {
+ auto field =
+ optional_leaf
+ ? SchemaField::MakeOptional(static_cast<int32_t>(depth), "value",
int32())
+ : SchemaField::MakeRequired(static_cast<int32_t>(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<int32_t>(level), "nested",
nested)
+ : SchemaField::MakeRequired(static_cast<int32_t>(level), "nested",
nested);
+ }
+ return std::make_unique<Schema>(std::vector<SchemaField>{std::move(field)});
+}
+
} // namespace
TEST(ManifestFileStructLike, BasicFields) {
@@ -152,7 +182,7 @@ TEST(StructLikeAccessorTest, GetLiteralUuid) {
std::string_view uuid_data(reinterpret_cast<const char*>(bytes.data()),
bytes.size());
SingleFieldStructLike row(Scalar{uuid_data});
std::array<size_t, 1> 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<size_t, 1> 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<size_t>
{};
+
+TEST_P(NestedStructLikeAccessorTest, NullParents) {
+ const std::vector<size_t> path(GetParam(), 0);
+ StructLikeAccessor accessor(int32(), path, std::vector<bool>(path.size(),
true));
+ for (const Scalar null :
+ {Scalar{std::monostate{}}, Scalar{std::shared_ptr<StructLike>{}}}) {
+ 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<std::monostate>(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<int32_t>(depth)));
+ EXPECT_EQ(required_accessor->position_path(), std::vector<size_t>(depth, 0));
+
+ for (const Scalar null :
+ {Scalar{std::monostate{}}, Scalar{std::shared_ptr<StructLike>{}}}) {
+ 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<int32_t>(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<size_t> path(GetParam(), 0);
+ StructLikeAccessor accessor(int32(), path, std::vector<bool>(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<int32_t>(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<int32_t>(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<size_t> path(GetParam(), 0);
+ StructLikeAccessor accessor(int32(), path, std::vector<bool>(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<size_t> path(GetParam(), 0);
+ StructLikeAccessor accessor(int32(), path, std::vector<bool>(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,
