fix: more review comments

Author: zhjwpku

src/iceberg/type.cc

@@ -367,21 +367,21 @@ GeometryType::GeometryType(std::string crs) {
 }
 
 std::string_view GeometryType::crs() const {
-  return crs_.has_value() ? std::string_view(*crs_) : kDefaultCrs;
+  return crs_.empty() ? kDefaultCrs : std::string_view(crs_);
 }
 TypeId GeometryType::type_id() const { return kTypeId; }
 std::string GeometryType::ToString() const {
-  if (!crs_.has_value()) {
+  if (crs_.empty()) {
     return "geometry";
   }
-  return std::format("geometry({})", *crs_);
+  return std::format("geometry({})", crs_);
 }
 bool GeometryType::Equals(const Type& other) const {
   if (other.type_id() != kTypeId) {
     return false;
   }
   const auto& geometry = static_cast<const GeometryType&>(other);
-  return crs() == geometry.crs();
+  return crs_ == geometry.crs_;
 }
 
 GeographyType::GeographyType(std::string crs) {
@@ -400,18 +400,16 @@ GeographyType::GeographyType(std::string crs, EdgeAlgorithm algorithm)
 }
 
 std::string_view GeographyType::crs() const {
-  return crs_.has_value() ? std::string_view(*crs_) : kDefaultCrs;
-}
-EdgeAlgorithm GeographyType::algorithm() const {
-  return algorithm_.value_or(kDefaultAlgorithm);
+  return crs_.empty() ? kDefaultCrs : std::string_view(crs_);
 }
+EdgeAlgorithm GeographyType::algorithm() const { return algorithm_; }
 TypeId GeographyType::type_id() const { return kTypeId; }
 std::string GeographyType::ToString() const {
-  if (algorithm_.has_value()) {
-    return std::format("geography({}, {})", crs(), iceberg::ToString(*algorithm_));
+  if (algorithm_ != kDefaultAlgorithm) {
+    return std::format("geography({}, {})", crs(), iceberg::ToString(algorithm_));
   }
-  if (crs_.has_value()) {
-    return std::format("geography({})", *crs_);
+  if (!crs_.empty()) {
+    return std::format("geography({})", crs_);
   }
   return "geography";
 }
@@ -420,7 +418,7 @@ bool GeographyType::Equals(const Type& other) const {
     return false;
   }
   const auto& geography = static_cast<const GeographyType&>(other);
-  return crs() == geography.crs() && algorithm() == geography.algorithm();
+  return crs_ == geography.crs_ && algorithm_ == geography.algorithm_;
 }
 
 FixedType::FixedType(int32_t length) : length_(length) {

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;
 };
 
 /// \brief A data type that does not have child fields.
@@ -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;
 };
 
 /// \defgroup type-nested Nested Types
@@ -324,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;
 
   TypeId type_id() const override;
   std::string ToString() const override;
@@ -377,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;
 };
 
 /// \brief A data type representing a timestamp in microseconds without
@@ -499,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;
 
   TypeId type_id() const override;
   std::string ToString() const override;
@@ -552,7 +551,7 @@ class ICEBERG_EXPORT GeometryType : public PrimitiveType {
   explicit GeometryType(std::string crs);
   ~GeometryType() override = default;
 
-  [[nodiscard]] std::string_view crs() const;
+  std::string_view crs() const;
 
   TypeId type_id() const override;
   std::string ToString() const override;
@@ -561,7 +560,7 @@ class ICEBERG_EXPORT GeometryType : public PrimitiveType {
   bool Equals(const Type& other) const override;
 
  private:
-  std::optional<std::string> crs_;
+  std::string crs_;
 };
 
 /// \brief A data type representing OGC geography in WKB format.
@@ -576,8 +575,8 @@ class ICEBERG_EXPORT GeographyType : public PrimitiveType {
   GeographyType(std::string crs, EdgeAlgorithm algorithm);
   ~GeographyType() override = default;
 
-  [[nodiscard]] std::string_view crs() const;
-  [[nodiscard]] EdgeAlgorithm algorithm() const;
+  std::string_view crs() const;
+  EdgeAlgorithm algorithm() const;
 
   TypeId type_id() const override;
   std::string ToString() const override;
@@ -586,8 +585,8 @@ class ICEBERG_EXPORT GeographyType : public PrimitiveType {
   bool Equals(const Type& other) const override;
 
  private:
-  std::optional<std::string> crs_;
-  std::optional<EdgeAlgorithm> algorithm_;
+  std::string crs_;
+  EdgeAlgorithm algorithm_ = kDefaultAlgorithm;
 };
 
 /// @}