Switched to public constructor for from_string for SparseVector [skip ci]

Author: ankane

include/pgvector/pqxx.hpp

@@ -7,6 +7,7 @@
 #pragma once
 
 #include <cstddef>
+#include <limits>
 #include <string_view>
 #include <vector>
 
@@ -182,13 +183,8 @@ template<> struct string_traits<pgvector::SparseVector> {
             throw conversion_error{"Malformed sparsevec literal"};
         }
 
+        std::unordered_map<int, float> map;
         int dimensions = pqxx::from_string<int>(text.substr(n + 2), c);
-        if (dimensions < 0) {
-            throw conversion_error{"Dimensions cannot be negative"};
-        }
-
-        std::vector<int> indices;
-        std::vector<float> values;
 
         if (n > 1) {
             auto add_element = [&](std::string_view substr) {
@@ -200,12 +196,12 @@ template<> struct string_traits<pgvector::SparseVector> {
                 int index = pqxx::from_string<int>(substr.substr(0, ne), c);
                 float value = pqxx::from_string<float>(substr.substr(ne + 1), c);
 
-                if (index < 1 || index > dimensions) {
-                    throw conversion_error{"Index out of bounds"};
+                // check to avoid undefined behavior
+                if (index > std::numeric_limits<int>::min()) {
+                    index -= 1;
                 }
 
-                indices.push_back(index - 1);
-                values.push_back(value);
+                map.insert({index, value});
             };
 
             std::string_view inner = text.substr(1, n - 1);
@@ -219,7 +215,11 @@ template<> struct string_traits<pgvector::SparseVector> {
             add_element(inner.substr(start));
         }
 
-        return pgvector::SparseVector{dimensions, std::move(indices), std::move(values)};
+        try {
+            return pgvector::SparseVector{map, dimensions};
+        } catch (const std::invalid_argument& e) {
+            throw conversion_error{e.what()};
+        }
     }
 
     static std::string_view to_buf(std::span<char> buf, const pgvector::SparseVector& value, ctx c = {}) {

include/pgvector/sparsevec.hpp

@@ -19,16 +19,6 @@ namespace pgvector {
 /// A sparse vector.
 class SparseVector {
   public:
-    /// @private
-    SparseVector(int dimensions, std::vector<int>&& indices, std::vector<float>&& values) {
-        if (values.size() != indices.size()) {
-            throw std::invalid_argument{"indices and values must be the same length"};
-        }
-        dimensions_ = dimensions;
-        indices_ = std::move(indices);
-        values_ = std::move(values);
-    }
-
     /// Creates a sparse vector from a dense vector.
     explicit SparseVector(const std::vector<float>& value) : SparseVector(std::span<const float>{value}) {}
 

test/pqxx_test.cpp

@@ -246,7 +246,7 @@ void test_sparsevec_from_string() {
 
     assert_exception<pqxx::conversion_error>([] {
         auto _ = pqxx::from_string<pgvector::SparseVector>("{}/-1");
-    }, "Dimensions cannot be negative");
+    }, "sparsevec must have at least 1 dimension");
 
     assert_exception<pqxx::conversion_error>([] {
         auto _ = pqxx::from_string<pgvector::SparseVector>("{:}/1");
@@ -258,15 +258,15 @@ void test_sparsevec_from_string() {
 
     assert_exception<pqxx::conversion_error>([] {
         auto _ = pqxx::from_string<pgvector::SparseVector>("{0:1}/1");
-    }, "Index out of bounds");
+    }, "sparsevec index out of bounds");
 
     assert_exception<pqxx::conversion_error>([] {
         auto _ = pqxx::from_string<pgvector::SparseVector>("{-2147483648:1}/1");
-    }, "Index out of bounds");
+    }, "sparsevec index out of bounds");
 
     assert_exception<pqxx::conversion_error>([] {
         auto _ = pqxx::from_string<pgvector::SparseVector>("{2:1}/1");
-    }, "Index out of bounds");
+    }, "sparsevec index out of bounds");
 
     assert_exception<pqxx::conversion_error>([] {
         auto _ = pqxx::from_string<pgvector::SparseVector>("{1:4e38}/1");