continue cleanup

Author: mapleFU

cpp/src/parquet/variant.cc

@@ -567,12 +567,21 @@ VariantValue::ObjectInfo VariantValue::getObjectInfo() const {
 
 std::optional<VariantValue> VariantValue::getObjectValueByKey(
     std::string_view key) const {
-  if (getBasicType() != VariantBasicType::Object) {
-    throw ParquetException("Not an object type");
-  }
-
   ObjectInfo info = getObjectInfo();
 
+  return getObjectValueByKey(key, info);
+}
+
+std::optional<VariantValue> VariantValue::getObjectValueByKey(
+    std::string_view key, const VariantValue::ObjectInfo& info) const {
+  // TODO(mwish): Currently we just linear search here. The best way here is:
+  //  1. check the num_elements
+  //  2.1. If the element number is less than 8(or other magic number), we can keep
+  //       current method.
+  //  2.2. If the element number is larger than 8, and metadata.sorted_strings is true,
+  //       we can first apply binary search on the metadata, and then binary search the
+  //       field id.
+
   for (uint32_t i = 0; i < info.num_elements; ++i) {
     std::string_view field_key;
     std::optional<VariantValue> field_value = getObjectFieldByFieldId(i, &field_key);
@@ -692,18 +701,20 @@ VariantValue::ArrayInfo VariantValue::getArrayInfo() const {
     next_offset = arrow::bit_util::FromLittleEndian(next_offset);
 
     if (offset > next_offset) {
-      throw ParquetException("Invalid array value: offsets not monotonically increasing");
+      throw ParquetException(
+          "Invalid array value: offsets not monotonically increasing: " +
+          std::to_string(offset) + " > " + std::to_string(next_offset));
     }
   }
 
   return info;
 }
 
-VariantValue VariantValue::getArrayValueByIndex(uint32_t index) const {
-  ArrayInfo info = getArrayInfo();
-
+VariantValue VariantValue::getArrayValueByIndex(uint32_t index,
+                                                const ArrayInfo& info) const {
   if (index >= info.num_elements) {
-    throw ParquetException("Array index out of range");
+    throw ParquetException("Array index out of range: " + std::to_string(index) +
+                           " >= " + std::to_string(info.num_elements));
   }
 
   // Read the offset and next offset
@@ -725,4 +736,9 @@ VariantValue VariantValue::getArrayValueByIndex(uint32_t index) const {
   return element_value;
 }
 
+VariantValue VariantValue::getArrayValueByIndex(uint32_t index) const {
+  ArrayInfo info = getArrayInfo();
+  return getArrayValueByIndex(index, info);
+}
+
 }  // namespace parquet::variant

cpp/src/parquet/variant.h

@@ -18,6 +18,7 @@
 #pragma once
 
 #include <cstdint>
+#include <optional>
 #include <string_view>
 #include <vector>
 
@@ -189,6 +190,8 @@ struct VariantValue {
   };
   ObjectInfo getObjectInfo() const;
   std::optional<VariantValue> getObjectValueByKey(std::string_view key) const;
+  std::optional<VariantValue> getObjectValueByKey(std::string_view key,
+                                                  const ObjectInfo& info) const;
   VariantValue getObjectFieldByFieldId(uint32_t variantId, std::string_view* key) const;
 
   struct ArrayInfo {
@@ -200,6 +203,7 @@ struct VariantValue {
   ArrayInfo getArrayInfo() const;
   // Would throw ParquetException if index is out of range.
   VariantValue getArrayValueByIndex(uint32_t index) const;
+  VariantValue getArrayValueByIndex(uint32_t index, const ArrayInfo& info) const;
 
  private:
   static constexpr uint8_t BASIC_TYPE_MASK = 0b00000011;
@@ -212,9 +216,11 @@ struct VariantValue {
   template <typename PrimitiveType>
   PrimitiveType getPrimitiveType(VariantPrimitiveType type) const;
 
+  // An extra function because decimal uses 1 byte for scale.
   template <typename DecimalType>
   DecimalValue<DecimalType> getPrimitiveDecimalType(VariantPrimitiveType type) const;
 
+  // An extra function because binary/string uses 4 bytes for length.
   std::string_view getPrimitiveBinaryType(VariantPrimitiveType type) const;
   void checkBasicType(VariantBasicType type) const;
   void checkPrimitiveType(VariantPrimitiveType type, size_t size_required) const;

cpp/src/parquet/variant_test.cc

@@ -424,4 +424,4 @@ TEST(ParquetVariant, ArrayValuesNested) {
   }
 }
 
-}  // namespace parquet::variant
+}  // namespace parquet::variant