Merge pull request #149 from Enmk/ipv4_ipv6_constructor_fix

Fixed ColumnIPv4 and ColumnIPv6 costruct from data

Author: Enmk

clickhouse/columns/ip4.cpp

@@ -13,11 +13,10 @@ ColumnIPv4::ColumnIPv4()
 
 ColumnIPv4::ColumnIPv4(ColumnRef data)
     : Column(Type::CreateIPv4())
-    , data_(data->As<ColumnUInt32>())
+    , data_(data ? data->As<ColumnUInt32>() : nullptr)
 {
-    if (data_->Size() != 0) {
-        throw std::runtime_error("number of entries must be even (32-bit numbers for each IPv4)");
-    }
+    if (!data_)
+        throw std::runtime_error("Expecting ColumnUInt32, got " + (data ? data->GetType().GetName() : "null"));
 }
 
 void ColumnIPv4::Append(const std::string& str) {
@@ -31,8 +30,7 @@ void ColumnIPv4::Append(uint32_t ip) {
     data_->Append(htonl(ip));
 }
 
-void ColumnIPv4::Append(in_addr ip)
-{
+void ColumnIPv4::Append(in_addr ip) {
     data_->Append(htonl(ip.s_addr));
 }
 
@@ -53,7 +51,18 @@ in_addr ColumnIPv4::operator [] (size_t n) const {
 }
 
 std::string ColumnIPv4::AsString(size_t n) const {
-    return inet_ntoa(this->At(n));
+    const auto& addr = this->At(n);
+
+    char buf[INET_ADDRSTRLEN];
+    const char* ip_str = inet_ntop(AF_INET, &addr, buf, INET_ADDRSTRLEN);
+
+    if (ip_str == nullptr) {
+        throw std::system_error(
+                std::error_code(errno, std::generic_category()),
+                "Invalid IPv4 data");
+    }
+
+    return ip_str;
 }
 
 void ColumnIPv4::Append(ColumnRef column) {

clickhouse/columns/ip4.h

@@ -8,7 +8,15 @@ namespace clickhouse {
 
 class ColumnIPv4 : public Column {
 public:
+    using DataType = in_addr;
+    using ValueType = in_addr;
+
     ColumnIPv4();
+    /** Takes ownership of the data, expects ColumnUInt32.
+     * Modifying memory pointed by `data` from outside is UB.
+     *
+     * TODO: deprecate and remove as it is too dangerous and error-prone.
+     */
     explicit ColumnIPv4(ColumnRef data);
 
     /// Appends one element to the column.

clickhouse/columns/ip6.cpp

@@ -16,17 +16,16 @@ ColumnIPv6::ColumnIPv6()
 
 ColumnIPv6::ColumnIPv6(ColumnRef data)
     : Column(Type::CreateIPv6())
-    , data_(data->As<ColumnFixedString>())
+    , data_(data ? data->As<ColumnFixedString>() : nullptr)
 {
-    if (data_->Size() != 0) {
-        throw std::runtime_error("number of entries must be even (two 64-bit numbers for each IPv6)");
-    }
+    if (!data_ || data_->FixedSize() != sizeof(in6_addr))
+        throw std::runtime_error("Expecting ColumnFixedString(16), got " + (data ? data->GetType().GetName() : "null"));
 }
 
-void ColumnIPv6::Append(const std::string& ip) {
+void ColumnIPv6::Append(const std::string_view& str) {
     unsigned char buf[16];
-    if (inet_pton(AF_INET6, ip.c_str(), buf) != 1) {
-        throw std::runtime_error("invalid IPv6 format, ip: " + ip);
+    if (inet_pton(AF_INET6, str.data(), buf) != 1) {
+        throw std::runtime_error("invalid IPv6 format, ip: " + std::string(str));
     }
     data_->Append(std::string_view((const char*)buf, 16));
 }
@@ -43,13 +42,18 @@ void ColumnIPv6::Clear() {
     data_->Clear();
 }
 
-std::string ColumnIPv6::AsString (size_t n) const{
-    const auto& addr = data_->At(n);
+std::string ColumnIPv6::AsString (size_t n) const {
+    const auto& addr = this->At(n);
+
     char buf[INET6_ADDRSTRLEN];
-    const char* ip_str = inet_ntop(AF_INET6, addr.data(), buf, INET6_ADDRSTRLEN);
+    const char* ip_str = inet_ntop(AF_INET6, &addr, buf, INET6_ADDRSTRLEN);
+
     if (ip_str == nullptr) {
-        throw std::runtime_error("invalid IPv6 format: " + std::string(addr));
+        throw std::system_error(
+                std::error_code(errno, std::generic_category()),
+                "Invalid IPv6 data");
     }
+
     return ip_str;
 }
 

clickhouse/columns/ip6.h

@@ -7,13 +7,21 @@ struct in6_addr;
 
 namespace clickhouse {
 
-class ColumnIPv6 : public Column{
+class ColumnIPv6 : public Column {
 public:
+    using DataType = in6_addr;
+    using ValueType = in6_addr;
+
     ColumnIPv6();
+    /** Takes ownership of the data, expects ColumnFixedString.
+     * Modifying memory pointed by `data` from outside is UB.
+     *
+     * TODO: deprecate and remove as it is too dangerous and error-prone.
+     */
     explicit ColumnIPv6(ColumnRef data);
 
     /// Appends one element to the column.
-    void Append(const std::string& str);
+    void Append(const std::string_view& str);
 
     void Append(const in6_addr* addr);
     void Append(const in6_addr& addr);

ut/columns_ut.cpp

@@ -20,40 +20,44 @@
 
 #include <clickhouse/base/socket.h> // for ipv4-ipv6 platform-specific stuff
 
-bool operator==(const in_addr & left, const in_addr & right)
-{
+// only compare PODs of equal size this way
+template <typename L, typename R, typename
+        = std::enable_if_t<sizeof(L) == sizeof(R) && std::conjunction_v<std::is_pod<L>, std::is_pod<R>>>>
+bool operator==(const L & left, const R& right) {
     return memcmp(&left, &right, sizeof(left)) == 0;
 }
 
-bool operator==(const in6_addr & left, const in6_addr & right)
-{
-    return memcmp(&left, &right, sizeof(left)) == 0;
+bool operator==(const in6_addr & left, const std::string_view & right) {
+    return right.size() == sizeof(left) && memcmp(&left, right.data(), sizeof(left)) == 0;
 }
 
-in_addr make_IPv4(uint32_t ip)
-{
+bool operator==(const std::string_view & left, const in6_addr & right) {
+    return left.size() == sizeof(right) && memcmp(left.data(), &right, sizeof(right)) == 0;
+}
+
+namespace {
+
+using namespace clickhouse;
+using namespace std::literals::string_view_literals;
+
+in_addr MakeIPv4(uint32_t ip) {
     static_assert(sizeof(in_addr) == sizeof(ip));
     in_addr result;
     memcpy(&result, &ip, sizeof(ip));
 
     return result;
 }
 
-std::ostream& operator<<(std::ostream& ostr, const in6_addr & addr)
-{
-    char buf[INET6_ADDRSTRLEN];
-    const char* ip_str = inet_ntop(AF_INET6, &addr, buf, INET6_ADDRSTRLEN);
-
-    if (!ip_str)
-        return ostr << "<!INVALID IPv6 VALUE!>";
-
-    return ostr << ip_str;
+in6_addr MakeIPv6(uint8_t v0,  uint8_t v1,  uint8_t v2,  uint8_t v3,
+                   uint8_t v4,  uint8_t v5,  uint8_t v6,  uint8_t v7,
+                   uint8_t v8,  uint8_t v9,  uint8_t v10, uint8_t v11,
+                   uint8_t v12, uint8_t v13, uint8_t v14, uint8_t v15) {
+    return in6_addr{{{v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15}}};
 }
 
-namespace {
-
-using namespace clickhouse;
-using namespace std::literals::string_view_literals;
+in6_addr MakeIPv6(uint8_t v10, uint8_t v11, uint8_t v12, uint8_t v13, uint8_t v14, uint8_t v15) {
+    return in6_addr{{{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, v10, v11, v12, v13, v14, v15}}};
+}
 
 static std::vector<uint32_t> MakeNumbers() {
     return std::vector<uint32_t>
@@ -91,7 +95,7 @@ static const auto LOWCARDINALITY_STRING_FOOBAR_10_ITEMS_BINARY =
 
 template <typename Generator>
 auto GenerateVector(size_t items, Generator && gen) {
-    std::vector<std::result_of_t<Generator(size_t)>> result;
+    std::vector<my_result_of_t<Generator(size_t)>> result;
     result.reserve(items);
     for (size_t i = 0; i < items; ++i) {
         result.push_back(std::move(gen(i)));
@@ -130,8 +134,7 @@ auto AlternateGenerators(Generator1 && gen1, Generator2 && gen2) {
 }
 
 template <typename T>
-std::vector<T> ConcatSequences(std::vector<T> && vec1, std::vector<T> && vec2)
-{
+std::vector<T> ConcatSequences(std::vector<T> && vec1, std::vector<T> && vec2) {
     std::vector<T> result(vec1);
 
     result.reserve(vec1.size() + vec2.size());
@@ -548,14 +551,14 @@ TEST(ColumnsCase, ColumnIPv4)
     col.Append("127.0.0.1");
     col.Append(3585395774);
     col.Append(0);
-    const in_addr ip = make_IPv4(0x12345678);
+    const in_addr ip = MakeIPv4(0x12345678);
     col.Append(ip);
 
     ASSERT_EQ(5u, col.Size());
-    EXPECT_EQ(make_IPv4(0xffffffff), col.At(0));
-    EXPECT_EQ(make_IPv4(0x0100007f), col.At(1));
-    EXPECT_EQ(make_IPv4(3585395774), col.At(2));
-    EXPECT_EQ(make_IPv4(0),          col.At(3));
+    EXPECT_EQ(MakeIPv4(0xffffffff), col.At(0));
+    EXPECT_EQ(MakeIPv4(0x0100007f), col.At(1));
+    EXPECT_EQ(MakeIPv4(3585395774), col.At(2));
+    EXPECT_EQ(MakeIPv4(0),          col.At(3));
     EXPECT_EQ(ip,                  col.At(4));
 
     EXPECT_EQ("255.255.255.255", col.AsString(0));
@@ -568,6 +571,63 @@ TEST(ColumnsCase, ColumnIPv4)
     EXPECT_EQ(0u, col.Size());
 }
 
+TEST(ColumnsCase, ColumnIPv4_construct_from_data)
+{
+    const auto vals = {
+        MakeIPv4(0x12345678),
+        MakeIPv4(0x0),
+        MakeIPv4(0x0100007f)
+    };
+
+    {
+        // Column is usable after being initialized with empty data column
+        auto col = ColumnIPv4(std::make_shared<ColumnUInt32>());
+        EXPECT_EQ(0u, col.Size());
+
+        // Make sure that `Append` and `At`/`[]` work properly
+        size_t i = 0;
+        for (const auto & v : vals) {
+            col.Append(v);
+            EXPECT_EQ(v, col[col.Size() - 1]) << "At pos " << i;
+            EXPECT_EQ(v, col.At(col.Size() - 1)) << "At pos " << i;
+            ++i;
+        }
+
+        EXPECT_EQ(vals.size(), col.Size());
+    }
+
+    {
+        // Column reports values from data column exactly, and also can be modified afterwards.
+        const auto values = std::vector<uint32_t>{std::numeric_limits<uint32_t>::min(), 123, 456, 789101112, std::numeric_limits<uint32_t>::max()};
+        auto col = ColumnIPv4(std::make_shared<ColumnUInt32>(values));
+
+        EXPECT_EQ(values.size(), col.Size());
+        for (size_t i = 0; i < values.size(); ++i) {
+            EXPECT_EQ(ntohl(values[i]), col[i]) << " At pos: " << i;
+        }
+
+        // Make sure that `Append` and `At`/`[]` work properly
+        size_t i = 0;
+        for (const auto & v : vals) {
+            col.Append(v);
+            EXPECT_EQ(v, col[col.Size() - 1]) << "At pos " << i;
+            EXPECT_EQ(v, col.At(col.Size() - 1)) << "At pos " << i;
+            ++i;
+        }
+
+        EXPECT_EQ(values.size() + vals.size(), col.Size());
+    }
+
+    EXPECT_ANY_THROW(ColumnIPv4(nullptr));
+    EXPECT_ANY_THROW(ColumnIPv4(ColumnRef(std::make_shared<ColumnInt8>())));
+    EXPECT_ANY_THROW(ColumnIPv4(ColumnRef(std::make_shared<ColumnInt32>())));
+
+    EXPECT_ANY_THROW(ColumnIPv4(ColumnRef(std::make_shared<ColumnUInt8>())));
+
+    EXPECT_ANY_THROW(ColumnIPv4(ColumnRef(std::make_shared<ColumnInt128>())));
+    EXPECT_ANY_THROW(ColumnIPv4(ColumnRef(std::make_shared<ColumnString>())));
+}
+
 TEST(ColumnsCase, ColumnIPv6)
 {
     // TODO: split into proper method-level unit-tests
@@ -576,22 +636,16 @@ TEST(ColumnsCase, ColumnIPv6)
     col.Append("::");
     col.Append("::FFFF:204.152.189.116");
 
-    const auto ipv6 = in6_addr{{{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15}}};
+    const auto ipv6 = MakeIPv6(0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15);
     col.Append(ipv6);
 
     ASSERT_EQ(4u, col.Size());
-    const auto first_val = in6_addr{{{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}}};
-    EXPECT_EQ(first_val, col.At(0));
-
-    const auto second_val = in6_addr{{{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}}};
-    EXPECT_EQ(second_val, col.At(1));
-
-    const auto third_val = in6_addr{{{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0xff, 0xff, 204, 152, 189, 116}}};
-    EXPECT_EQ(third_val, col.At(2));
+    EXPECT_EQ(MakeIPv6(0, 0, 0, 0, 0, 1),               col.At(0));
+    EXPECT_EQ(MakeIPv6(0, 0, 0, 0, 0, 0),               col.At(1));
+    EXPECT_EQ(MakeIPv6(0xff, 0xff, 204, 152, 189, 116), col.At(2));
 
     EXPECT_EQ(ipv6, col.At(3));
 
-
     EXPECT_EQ("::1",                    col.AsString(0));
     EXPECT_EQ("::",                     col.AsString(1));
     EXPECT_EQ("::ffff:204.152.189.116", col.AsString(2));
@@ -601,6 +655,70 @@ TEST(ColumnsCase, ColumnIPv6)
     EXPECT_EQ(0u, col.Size());
 }
 
+TEST(ColumnsCase, ColumnIPv6_construct_from_data)
+{
+    const auto vals = {
+        MakeIPv6(0xff, 0xff, 204, 152, 189, 116),
+        MakeIPv6(0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15),
+    };
+
+    {
+        // Column is usable after being initialized with empty data column
+        auto col = ColumnIPv6(std::make_shared<ColumnFixedString>(16));
+        EXPECT_EQ(0u, col.Size());
+
+        // Make sure that `Append` and `At`/`[]` work properly
+        size_t i = 0;
+        for (const auto & v : vals) {
+            col.Append(v);
+            EXPECT_EQ(v, col[col.Size() - 1]) << "At pos " << i;
+            EXPECT_EQ(v, col.At(col.Size() - 1)) << "At pos " << i;
+            ++i;
+        }
+
+        EXPECT_EQ(vals.size(), col.Size());
+    }
+
+    {
+        // Column reports values from data column exactly, and also can be modified afterwards.
+        using namespace std::literals;
+        const auto values = std::vector<std::string_view>{
+                "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0A\x0B\x0C\x0D\x0E\x0F"sv,
+                "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1A\x1B\x1C\x1D\x1E\x1F"sv,
+                "\xF0\xF1\xF2\xF3\xF4\xF5\xF6\xF7\xF8\xF9\xFA\xFB\xFC\xFD\xFE\xFF"sv};
+        auto col = ColumnIPv6(std::make_shared<ColumnFixedString>(16, values));
+
+        EXPECT_EQ(values.size(), col.Size());
+        for (size_t i = 0; i < values.size(); ++i) {
+            EXPECT_EQ(values[i], col[i]) << " At pos: " << i;
+        }
+
+        // Make sure that `Append` and `At`/`[]` work properly
+        size_t i = 0;
+        for (const auto & v : vals) {
+            col.Append(v);
+            EXPECT_EQ(v, col[col.Size() - 1]) << "At pos " << i;
+            EXPECT_EQ(v, col.At(col.Size() - 1)) << "At pos " << i;
+            ++i;
+        }
+
+        EXPECT_EQ(values.size() + vals.size(), col.Size());
+    }
+
+    // Make sure that column can't be constructed with wrong data columns (wrong size/wrong type or null)
+    EXPECT_ANY_THROW(ColumnIPv4(nullptr));
+    EXPECT_ANY_THROW(ColumnIPv6(ColumnRef(std::make_shared<ColumnFixedString>(15))));
+    EXPECT_ANY_THROW(ColumnIPv6(ColumnRef(std::make_shared<ColumnFixedString>(17))));
+
+    EXPECT_ANY_THROW(ColumnIPv6(ColumnRef(std::make_shared<ColumnInt8>())));
+    EXPECT_ANY_THROW(ColumnIPv6(ColumnRef(std::make_shared<ColumnInt32>())));
+
+    EXPECT_ANY_THROW(ColumnIPv6(ColumnRef(std::make_shared<ColumnUInt8>())));
+
+    EXPECT_ANY_THROW(ColumnIPv6(ColumnRef(std::make_shared<ColumnInt128>())));
+    EXPECT_ANY_THROW(ColumnIPv6(ColumnRef(std::make_shared<ColumnString>())));
+}
+
 TEST(ColumnsCase, ColumnDecimal128_from_string) {
     auto col = std::make_shared<ColumnDecimal>(38, 0);