Copilot commented on code in PR #752:
URL: https://github.com/apache/iceberg-cpp/pull/752#discussion_r3437265383
##########
src/iceberg/type.h:
##########
@@ -46,20 +46,20 @@ class ICEBERG_EXPORT Type : public
iceberg::util::Formattable {
~Type() override = default;
/// \brief Get the type ID.
- [[nodiscard]] virtual TypeId type_id() const = 0;
+ virtual TypeId type_id() const = 0;
/// \brief Is this a primitive type (may not have child fields)?
- [[nodiscard]] virtual bool is_primitive() const = 0;
+ virtual bool is_primitive() const = 0;
/// \brief Is this a nested type (may have child fields)?
- [[nodiscard]] virtual bool is_nested() const = 0;
+ virtual bool is_nested() const = 0;
Review Comment:
`type.h` also drops `[[nodiscard]]` from `Type::is_nested()`, which is part
of the same core query surface as `type_id()` / `is_primitive()` and should
remain `[[nodiscard]]` for consistency and to prevent accidental misuse.
##########
src/iceberg/type.h:
##########
@@ -46,20 +46,20 @@ class ICEBERG_EXPORT Type : public
iceberg::util::Formattable {
~Type() override = default;
/// \brief Get the type ID.
- [[nodiscard]] virtual TypeId type_id() const = 0;
+ virtual TypeId type_id() const = 0;
/// \brief Is this a primitive type (may not have child fields)?
- [[nodiscard]] virtual bool is_primitive() const = 0;
+ virtual bool is_primitive() const = 0;
Review Comment:
`type.h` drops `[[nodiscard]]` from core `Type` query APIs (e.g.,
`type_id()` / `is_primitive()`), which is inconsistent with the rest of the
codebase (many public getters/Status-returning APIs are marked `[[nodiscard]]`)
and makes it easier to accidentally ignore these results.
##########
src/iceberg/type.h:
##########
@@ -46,20 +46,20 @@ class ICEBERG_EXPORT Type : public
iceberg::util::Formattable {
~Type() override = default;
/// \brief Get the type ID.
- [[nodiscard]] virtual TypeId type_id() const = 0;
+ virtual TypeId type_id() const = 0;
/// \brief Is this a primitive type (may not have child fields)?
- [[nodiscard]] virtual bool is_primitive() const = 0;
+ virtual bool is_primitive() const = 0;
/// \brief Is this a nested type (may have child fields)?
- [[nodiscard]] virtual bool is_nested() const = 0;
+ virtual bool is_nested() const = 0;
/// \brief Compare two types for equality.
friend bool operator==(const Type& lhs, const Type& rhs) { return
lhs.Equals(rhs); }
protected:
/// \brief Compare two types for equality.
- [[nodiscard]] virtual bool Equals(const Type& other) const = 0;
+ virtual bool Equals(const Type& other) const = 0;
Review Comment:
`Type::Equals()` is the core equality implementation behind `operator==`;
dropping `[[nodiscard]]` makes it easier to accidentally ignore the result and
is inconsistent with other `Equals` implementations in the repo (e.g.,
`SchemaField::Equals`, `PartitionField::Equals`).
##########
src/iceberg/test/type_test.cc:
##########
@@ -90,7 +94,7 @@ TEST_P(TypeTest, StdFormat) {
ASSERT_EQ(test_case.repr, std::format("{}", *test_case.type));
}
-const static std::array<TypeTestCase, 19> kPrimitiveTypes = {{
+const static std::array<TypeTestCase, 22> kPrimitiveTypes = {{
{
.name = "boolean",
Review Comment:
`kPrimitiveTypes` now includes `variant`, which is explicitly *not* a
primitive type (`Type::is_primitive() == false`). This makes the test data name
misleading and harder to reason about (especially when debugging failures by
test suite name/parameter source).
##########
src/iceberg/type.h:
##########
@@ -475,7 +498,7 @@ class ICEBERG_EXPORT FixedType : public PrimitiveType {
~FixedType() override = default;
/// \brief The length (the number of bytes to store).
- [[nodiscard]] int32_t length() const;
+ int32_t length() const;
Review Comment:
`FixedType::length()` is a public getter; dropping `[[nodiscard]]` is
inconsistent with other getters and makes it easier to accidentally ignore the
return value.
##########
src/iceberg/type.h:
##########
@@ -76,28 +76,27 @@ class ICEBERG_EXPORT NestedType : public Type {
bool is_nested() const override { return true; }
/// \brief Get a view of the child fields.
- [[nodiscard]] virtual std::span<const SchemaField> fields() const = 0;
+ virtual std::span<const SchemaField> fields() const = 0;
using SchemaFieldConstRef = std::reference_wrapper<const SchemaField>;
/// \brief Get a field by field ID.
///
/// \note This is O(1) complexity.
- [[nodiscard]] virtual Result<std::optional<SchemaFieldConstRef>>
GetFieldById(
+ virtual Result<std::optional<SchemaFieldConstRef>> GetFieldById(
int32_t field_id) const = 0;
/// \brief Get a field by index.
///
/// \note This is O(1) complexity.
- [[nodiscard]] virtual Result<std::optional<SchemaFieldConstRef>>
GetFieldByIndex(
+ virtual Result<std::optional<SchemaFieldConstRef>> GetFieldByIndex(
int32_t index) const = 0;
/// \brief Get a field by name. Return an error Status if
/// the field name is not unique; prefer GetFieldById or GetFieldByIndex
/// when possible.
///
/// \note This is O(1) complexity.
- [[nodiscard]] virtual Result<std::optional<SchemaFieldConstRef>>
GetFieldByName(
+ virtual Result<std::optional<SchemaFieldConstRef>> GetFieldByName(
std::string_view name, bool case_sensitive) const = 0;
/// \brief Get a field by name (case-sensitive).
- [[nodiscard]] Result<std::optional<SchemaFieldConstRef>> GetFieldByName(
- std::string_view name) const;
+ Result<std::optional<SchemaFieldConstRef>> GetFieldByName(std::string_view
name) const;
Review Comment:
`NestedType::GetFieldByName(std::string_view)` returns a `Result<...>` and
should be `[[nodiscard]]` (similar to other `Result`-returning APIs), otherwise
callers can accidentally drop errors / results.
##########
src/iceberg/type.h:
##########
@@ -76,28 +76,27 @@ class ICEBERG_EXPORT NestedType : public Type {
bool is_nested() const override { return true; }
/// \brief Get a view of the child fields.
- [[nodiscard]] virtual std::span<const SchemaField> fields() const = 0;
+ virtual std::span<const SchemaField> fields() const = 0;
Review Comment:
`NestedType::fields()` is part of the public type API and previously
benefited from `[[nodiscard]]` (consistent with other getters like
`SchemaField::type()`); dropping it makes it easier to ignore important results.
##########
src/iceberg/type.h:
##########
@@ -300,10 +323,10 @@ class ICEBERG_EXPORT DecimalType : public PrimitiveType {
~DecimalType() override = default;
/// \brief Get the precision (the number of decimal digits).
- [[nodiscard]] int32_t precision() const;
+ int32_t precision() const;
/// \brief Get the scale (essentially, the number of decimal digits after
/// the decimal point; precisely, the value is scaled by $$10^{-s}$$.).
- [[nodiscard]] int32_t scale() const;
+ int32_t scale() const;
Review Comment:
`DecimalType::precision()` / `scale()` are public getters; dropping
`[[nodiscard]]` is inconsistent with other getters in the repo and makes it
easier for callers to accidentally ignore these values.
##########
src/iceberg/type.h:
##########
@@ -353,9 +376,9 @@ class ICEBERG_EXPORT TimeType : public PrimitiveType {
class ICEBERG_EXPORT TimestampBase : public PrimitiveType {
public:
/// \brief Is this type zoned or naive?
- [[nodiscard]] virtual bool is_zoned() const = 0;
+ virtual bool is_zoned() const = 0;
/// \brief The time resolution.
- [[nodiscard]] virtual TimeUnit time_unit() const = 0;
+ virtual TimeUnit time_unit() const = 0;
Review Comment:
`TimestampBase::is_zoned()` / `time_unit()` are fundamental property queries
and should remain `[[nodiscard]]` for consistency with the rest of the public
type API and to prevent accidental ignoring of these results.
##########
src/iceberg/test/visit_type_test.cc:
##########
@@ -46,14 +46,15 @@ struct TypeTestCase {
std::shared_ptr<iceberg::Type> type;
iceberg::TypeId type_id;
bool primitive;
+ bool nested = false;
std::string repr;
};
std::string TypeTestCaseToString(const ::testing::TestParamInfo<TypeTestCase>&
info) {
return info.param.name;
}
-const static std::array<TypeTestCase, 19> kPrimitiveTypes = {{
+const static std::array<TypeTestCase, 22> kPrimitiveTypes = {{
{
.name = "boolean",
.type = iceberg::boolean(),
Review Comment:
`kPrimitiveTypes` now includes `variant`, which is not a primitive type
(`is_primitive() == false`). The parameter set name is therefore misleading and
can confuse future updates/debugging of these tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
