Address review comments

Author: pitrou

cpp/src/parquet/schema_test.cc

@@ -2360,18 +2360,18 @@ TEST(TestLogicalTypeSerialization, Roundtrips) {
 TEST(TestLogicalTypeSerialization, VariantSpecificationVersion) {
   // Confirm that Variant logical type sets specification_version to expected value in
   // thrift serialization
-  constexpr int8_t specVersion = 2;
+  constexpr int8_t spec_version = 2;
   auto metadata = PrimitiveNode::Make("metadata", Repetition::REQUIRED, Type::BYTE_ARRAY);
   auto value = PrimitiveNode::Make("value", Repetition::REQUIRED, Type::BYTE_ARRAY);
   NodePtr variant_node =
       GroupNode::Make("variant", Repetition::REQUIRED, {metadata, value},
-                      LogicalType::Variant(specVersion));
+                      LogicalType::Variant(spec_version));
 
-  // Verify varaint logical type
+  // Verify variant logical type
   auto logical_type = variant_node->logical_type();
   ASSERT_TRUE(logical_type->is_variant());
   const auto& variant_type = checked_cast<const VariantLogicalType&>(*logical_type);
-  ASSERT_EQ(variant_type.spec_version(), specVersion);
+  ASSERT_EQ(variant_type.spec_version(), spec_version);
 
   // Verify thrift serialization
   std::vector<format::SchemaElement> elements;
@@ -2384,7 +2384,7 @@ TEST(TestLogicalTypeSerialization, VariantSpecificationVersion) {
 
   // Verify that specification_version is set properly
   ASSERT_TRUE(elements[0].logicalType.VARIANT.__isset.specification_version);
-  ASSERT_EQ(elements[0].logicalType.VARIANT.specification_version, specVersion);
+  ASSERT_EQ(elements[0].logicalType.VARIANT.specification_version, spec_version);
 }
 
 }  // namespace schema

cpp/src/parquet/types.cc

@@ -596,12 +596,12 @@ std::shared_ptr<const LogicalType> LogicalType::FromThrift(
 
     return GeographyLogicalType::Make(std::move(crs), algorithm);
   } else if (type.__isset.VARIANT) {
-    int8_t specVersion = VARIANT_SPEC_VERSION;
+    int8_t spec_version = kVariantSpecVersion;
     if (type.VARIANT.__isset.specification_version) {
-      specVersion = type.VARIANT.specification_version;
+      spec_version = type.VARIANT.specification_version;
     }
 
-    return VariantLogicalType::Make(specVersion);
+    return VariantLogicalType::Make(spec_version);
   } else {
     // Sentinel type for one we do not recognize
     return UndefinedLogicalType::Make();
@@ -669,8 +669,8 @@ std::shared_ptr<const LogicalType> LogicalType::Geography(
   return GeographyLogicalType::Make(std::move(crs), algorithm);
 }
 
-std::shared_ptr<const LogicalType> LogicalType::Variant(int8_t specVersion) {
-  return VariantLogicalType::Make(specVersion);
+std::shared_ptr<const LogicalType> LogicalType::Variant(int8_t spec_version) {
+  return VariantLogicalType::Make(spec_version);
 }
 
 std::shared_ptr<const LogicalType> LogicalType::None() { return NoLogicalType::Make(); }
@@ -1972,16 +1972,16 @@ class LogicalType::Impl::Variant final : public LogicalType::Impl::Incompatible,
   std::string ToJSON() const override;
   format::LogicalType ToThrift() const override;
 
-  int8_t spec_version() const { return specVersion_; }
+  int8_t spec_version() const { return spec_version_; }
 
  private:
-  explicit Variant(const int8_t specVersion)
+  explicit Variant(const int8_t spec_version)
       : LogicalType::Impl(LogicalType::Type::VARIANT, SortOrder::UNKNOWN),
         LogicalType::Impl::Inapplicable() {
-    this->specVersion_ = specVersion;
+    this->spec_version_ = spec_version;
   }
 
-  int8_t specVersion_;
+  int8_t spec_version_;
 };
 
 int8_t VariantLogicalType::spec_version() const {
@@ -1990,13 +1990,13 @@ int8_t VariantLogicalType::spec_version() const {
 
 std::string LogicalType::Impl::Variant::ToString() const {
   std::stringstream type;
-  type << "Variant(" << static_cast<int>(specVersion_) << ")";
+  type << "Variant(" << static_cast<int>(spec_version_) << ")";
   return type.str();
 }
 
 std::string LogicalType::Impl::Variant::ToJSON() const {
   std::stringstream json;
-  json << R"({"Type": "Variant", "SpecVersion": )" << static_cast<int>(specVersion_)
+  json << R"({"Type": "Variant", "SpecVersion": )" << static_cast<int>(spec_version_)
        << "}";
 
   return json.str();
@@ -2005,14 +2005,14 @@ std::string LogicalType::Impl::Variant::ToJSON() const {
 format::LogicalType LogicalType::Impl::Variant::ToThrift() const {
   format::LogicalType type;
   format::VariantType variant_type;
-  variant_type.__set_specification_version(specVersion_);
+  variant_type.__set_specification_version(spec_version_);
   type.__set_VARIANT(variant_type);
   return type;
 }
 
-std::shared_ptr<const LogicalType> VariantLogicalType::Make(const int8_t specVersion) {
+std::shared_ptr<const LogicalType> VariantLogicalType::Make(const int8_t spec_version) {
   auto logical_type = std::shared_ptr<VariantLogicalType>(new VariantLogicalType());
-  logical_type->impl_.reset(new LogicalType::Impl::Variant(specVersion));
+  logical_type->impl_.reset(new LogicalType::Impl::Variant(spec_version));
   return logical_type;
 }
 

cpp/src/parquet/types.h

@@ -179,7 +179,7 @@ class PARQUET_EXPORT LogicalType {
   };
 
   /// \brief The latest supported Variant specification version by this library
-  static constexpr int8_t VARIANT_SPEC_VERSION = 1;
+  static constexpr int8_t kVariantSpecVersion = 1;
 
   /// \brief If possible, return a logical type equivalent to the given legacy
   /// converted type (and decimal metadata if applicable).
@@ -228,7 +228,7 @@ class PARQUET_EXPORT LogicalType {
   static std::shared_ptr<const LogicalType> UUID();
   static std::shared_ptr<const LogicalType> Float16();
   static std::shared_ptr<const LogicalType> Variant(
-      int8_t specVersion = VARIANT_SPEC_VERSION);
+      int8_t specVersion = kVariantSpecVersion);
 
   static std::shared_ptr<const LogicalType> Geometry(std::string crs = "");
 
@@ -500,7 +500,7 @@ class PARQUET_EXPORT GeographyLogicalType : public LogicalType {
 class PARQUET_EXPORT VariantLogicalType : public LogicalType {
  public:
   static std::shared_ptr<const LogicalType> Make(
-      int8_t specVersion = VARIANT_SPEC_VERSION);
+      int8_t specVersion = kVariantSpecVersion);
 
   int8_t spec_version() const;