Fix IPv4 byte order handling

clickhouse/columns/ip4.cpp

@@ -32,15 +32,15 @@ void ColumnIPv4::Append(const std::string& str) {
     uint32_t address;
     if (inet_pton(AF_INET, str.c_str(), &address) != 1)
         throw ValidationError("invalid IPv4 format, ip: " + str);
-    data_->Append(htonl(address));
+    data_->Append(address);
 }
 
 void ColumnIPv4::Append(uint32_t ip) {
     data_->Append(htonl(ip));
 }
 
 void ColumnIPv4::Append(in_addr ip) {
-    data_->Append(htonl(ip.s_addr));
+    data_->Append(ip.s_addr);
 }
 
 void ColumnIPv4::Clear() {
@@ -49,13 +49,13 @@ void ColumnIPv4::Clear() {
 
 in_addr ColumnIPv4::At(size_t n) const {
     in_addr addr;
-    addr.s_addr = ntohl(data_->At(n));
+    addr.s_addr = data_->At(n);
     return addr;
 }
 
 in_addr ColumnIPv4::operator [] (size_t n) const {
     in_addr addr;
-    addr.s_addr = ntohl(data_->operator[](n));
+    addr.s_addr = data_->operator[](n);
     return addr;
 }
 

clickhouse/columns/ip4.h

@@ -27,7 +27,7 @@ class ColumnIPv4 : public Column {
     /// @params ip numeric value with host byte order.
     void Append(uint32_t ip);
 
-    ///
+    /// @params ip IPv4 address in network byte order.
     void Append(in_addr ip);
 
     /// Returns element at given row number.

clickhouse/columns/lowcardinality.cpp

@@ -233,8 +233,8 @@ inline void AppendToDictionary(Column& dictionary, const ItemView & item) {
             column_down_cast<ColumnDateTime>(dictionary).AppendRaw(item.get<uint32_t>());
             return;
         case Type::IPv4:
-            // ColumnIPv4::Append applies htonl, and GetItem returns the stored
-            // (already byte-swapped) value, so undo the swap to re-store as-is.
+            // ColumnIPv4::Append(uint32_t) accepts host byte order, while GetItem
+            // returns the stored network-order representation.
             column_down_cast<ColumnIPv4>(dictionary).Append(ntohl(item.get<uint32_t>()));
             return;
         case Type::IPv6: {

ut/Column_ut.cpp

@@ -313,7 +313,7 @@ inline auto convertValueForGetItem(const ColumnType& col, ValueType&& t) {
             || std::is_same_v<T, clickhouse::Int128>) {
         return std::string_view{reinterpret_cast<const char*>(&t), sizeof(T)};
     } else if constexpr (std::is_same_v<T, in_addr>) {
-        return htonl(t.s_addr);
+        return t.s_addr;
     } else if constexpr (std::is_same_v<T, in6_addr>) {
         return std::string_view(reinterpret_cast<const char*>(t.s6_addr), 16);
     } else if constexpr (std::is_same_v<ColumnType, ColumnDate>) {

ut/columns_ut.cpp

@@ -954,15 +954,15 @@ TEST(ColumnsCase, ColumnIPv4)
 
     col.Append("255.255.255.255");
     col.Append("127.0.0.1");
-    col.Append(3585395774);
+    col.Append(0x3eccb4d5);
     col.Append(0);
     const in_addr ip = MakeIPv4(0x12345678);
     col.Append(ip);
 
     ASSERT_EQ(5u, col.Size());
     EXPECT_EQ(MakeIPv4(0xffffffff), col.At(0));
     EXPECT_EQ(MakeIPv4(0x0100007f), col.At(1));
-    EXPECT_EQ(MakeIPv4(3585395774), col.At(2));
+    EXPECT_EQ(MakeIPv4(0xd5b4cc3e), col.At(2));
     EXPECT_EQ(MakeIPv4(0),          col.At(3));
     EXPECT_EQ(ip,                  col.At(4));
 
@@ -976,6 +976,42 @@ TEST(ColumnsCase, ColumnIPv4)
     EXPECT_EQ(0u, col.Size());
 }
 
+TEST(ColumnsCase, ColumnIPv4_append_overloads_use_same_byte_order)
+{
+    auto from_string = ColumnIPv4();
+    auto from_host_order = ColumnIPv4();
+    auto from_in_addr = ColumnIPv4();
+
+    in_addr parsed{};
+    ASSERT_EQ(1, inet_pton(AF_INET, "127.0.0.1", &parsed));
+
+    from_string.Append("127.0.0.1");
+    from_host_order.Append(0x7f000001);
+    from_in_addr.Append(parsed);
+
+    Buffer string_data;
+    Buffer host_order_data;
+    Buffer in_addr_data;
+
+    BufferOutput string_output(&string_data);
+    BufferOutput host_order_output(&host_order_data);
+    BufferOutput in_addr_output(&in_addr_data);
+
+    from_string.SaveBody(&string_output);
+    from_host_order.SaveBody(&host_order_output);
+    from_in_addr.SaveBody(&in_addr_output);
+
+    string_output.Flush();
+    host_order_output.Flush();
+    in_addr_output.Flush();
+
+    EXPECT_EQ(host_order_data, string_data);
+    EXPECT_EQ(host_order_data, in_addr_data);
+    EXPECT_EQ("127.0.0.1", from_string.AsString(0));
+    EXPECT_EQ("127.0.0.1", from_host_order.AsString(0));
+    EXPECT_EQ("127.0.0.1", from_in_addr.AsString(0));
+}
+
 TEST(ColumnsCase, ColumnIPv4_construct_from_data)
 {
     const auto vals = {
@@ -1008,7 +1044,7 @@ TEST(ColumnsCase, ColumnIPv4_construct_from_data)
 
         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;
+            EXPECT_EQ(values[i], col[i]) << " At pos: " << i;
         }
 
         // Make sure that `Append` and `At`/`[]` work properly
@@ -1041,9 +1077,9 @@ TEST(ColumnsCase, ColumnIPv4_construct_from_rvalue_data) {
     };
 
     const auto expected = {
-        MakeIPv4(data[0]),
-        MakeIPv4(data[1]),
-        MakeIPv4(data[2]),
+        MakeIPv4(htonl(data[0])),
+        MakeIPv4(htonl(data[1])),
+        MakeIPv4(htonl(data[2])),
     };
 
     auto col = ColumnIPv4(std::move(data));

ut/utils.cpp

@@ -426,7 +426,7 @@ std::ostream& operator<<(std::ostream& ostr, const ItemView& item_view) {
         }
         case Type::IPv4: {
             in_addr addr;
-            addr.s_addr = ntohl(item_view.get<uint32_t>());
+            addr.s_addr = item_view.get<uint32_t>();
             ostr << addr;
             break;
         }