Implement IPv6 support in the inet extension. #10839

troycurtisjr · 2024-02-25T14:27:37Z

I've started to use the duckdb and have several cases where the inet module has been really nice. However, it doesn't currently support IPv6, so this PR aims to add that support. There is also a somewhat related discussion about using uhugeint to store IPv6.

The CI runs for the latest rebase are in progress, but there was already a mostly successful back before my latest rebase. There were a few failures in the NightlyTests action, but nothing seemed related to the inet extension. I'm hoping the new run will have pulled in fixes from main.

Corresponding doc PR: duckdb-web

Mytherin

Thanks for the PR! Looks great, well done!

Some minor comments below. Postgres also supports the family function which returns whether or not a value is IPv4 or IPv6 - perhaps it makes sense to add support for that as well?

Mytherin · 2024-02-26T12:41:48Z

extension/inet/inet_functions.cpp

+	if (addr_type == IPAddressType::IP_ADDRESS_V6) {
+		// The top bit is flipped when storing as the signed hugeint so that sorting
+		// works correctly. Flip it back here to have a proper unsigned value.
+		return retval ^ (uhugeint_t(1) << 127);


Bitwise ops on uhugeint are somewhat expensive - perhaps we can do the bitwise operation directly on the component that is affected instead (e.g. new_addr.upper or new_addr.lower)?

Make sense, you usually do pay a price for magic 😉 . I've started some implementations, and I'll take a look at some of the other bit operations.

I've updated the implementation to perform bit ops directly on the two halves of the hugeint object.

Mytherin · 2024-02-26T12:42:07Z

extension/inet/inet_functions.cpp

 using INET_TYPE = StructTypeTernary<uint8_t, hugeint_t, uint16_t>;

+static uhugeint_t from_compat_addr(hugeint_t compat_addr, IPAddressType addr_type) {


nit: Could we use the naming conventions we use elsewhere in the codebase for functions?

Right, just snake cased it out of habit. Updated.

Mytherin · 2024-02-26T12:45:41Z

extension/inet/inet_functions.cpp

+#pragma GCC diagnostic ignored "-Wunused-function"
+#endif
+
+#include "duckdb/common/operator/numeric_cast.hpp"


Is this include here necessary at all? Can we perhaps include duckdb/common/operator/cast_operators.hpp instead? Then we might also be able to get rid of all of the pragmas.

Turns out it isn't necessary, uhugeint has a built in cast operator from hugeint, and going from always positive hugeint to uhugeint has no risk of overflow, so the lack of checks isn't a concern.

Mytherin · 2024-02-26T12:47:49Z

test/sql/inet/test_inet_functions.test

+# overflow
+statement error
+select INET '255.255.255.255' + 1
+----


Could you add an expected error message here?

Makes sense, I'll work on adding these cases.

Added expected outputs to various error tests.

Mytherin · 2024-02-26T12:57:13Z

test/sql/inet/test_ipv6_inet_type.test

+# quibble with too many digits
+statement error
+SELECT '2001:db8:100:0:c59b:cef3:a0d189:6de4'::INET;
+----


Could we add expected errors here?

* Fix function naming to match the project style * Optimize some uhugeint bit operations * Drop the unnecessary numeric_cast header and associated pragmas

troycurtisjr · 2024-02-27T03:07:32Z

Thanks for the PR! Looks great, well done!

Some minor comments below. Postgres also supports the family function which returns whether or not a value is IPv4 or IPv6 - perhaps it makes sense to add support for that as well?

I have a vague idea of implementing many of the helpers from postgres inet features actually, but wanted to keep this PR focused on the basic support. Still this should be an easy add and provides a programatic way to detect v4 versus v6 now that both are supported, so it seems sensible. I'll get it added in.

troycurtisjr · 2024-03-02T14:56:46Z

I implemented the review comments, including adding the family() function. It is ready for another round of review.

carlopi · 2024-03-07T09:16:45Z

Single failure is on Checks extension entries since family function needs to be declared also there.
I recently made some improvement there, I would say merging (or rebasing) with main and adding this line:

     {"drop_fts_index", "fts", CatalogType::PRAGMA_FUNCTION_ENTRY},
     {"dsdgen", "tpcds", CatalogType::TABLE_FUNCTION_ENTRY},
     {"excel_text", "excel", CatalogType::SCALAR_FUNCTION_ENTRY},
+    {"family", "inet", CatalogType::SCALAR_FUNCTION_ENTRY},
     {"from_json", "json", CatalogType::SCALAR_FUNCTION_ENTRY},
     {"from_json_strict", "json", CatalogType::SCALAR_FUNCTION_ENTRY},
     {"from_substrait", "substrait", CatalogType::TABLE_FUNCTION_ENTRY},

to src/include/duckdb/main/extension_entries.hpp should make this work properly.

I am testing the changes here: https://github.com/carlopi/duckdb/actions/runs/8185666386

Implement IPv6 support in the inet extension.

85b3af5

troycurtisjr mentioned this pull request Feb 25, 2024

Add ipv6 to the inet extension documentation. duckdb/duckdb-web#2314

Merged

szarnyasg added the Ready For Review label Feb 25, 2024

Mytherin reviewed Feb 26, 2024

View reviewed changes

Mytherin added Changes Requested and removed Ready For Review labels Feb 26, 2024

Apply improvements from PR review.

5fa98eb

* Fix function naming to match the project style * Optimize some uhugeint bit operations * Drop the unnecessary numeric_cast header and associated pragmas

github-actions bot marked this pull request as draft February 27, 2024 02:59

Apply format-fix.

915ae82

Mytherin marked this pull request as ready for review February 28, 2024 09:08

troycurtisjr added 4 commits March 1, 2024 21:56

Add expected error messages to the ipv6 inet tests.

2f5479f

Perform bit operation on the address halves to reduce overhead.

4ec5e7e

Implement the inet "family" function.

a4a6ff1

Merge branch 'main' into add-ipv6-inet

c4718a0

github-actions bot marked this pull request as draft March 2, 2024 14:54

troycurtisjr marked this pull request as ready for review March 2, 2024 14:56

Mytherin added Ready For Review and removed Changes Requested labels Mar 6, 2024

carlopi added CI Failure and removed Ready For Review labels Mar 7, 2024

carlopi mentioned this pull request Mar 10, 2024

Add ipv6 inet + minor fixes #11073

Merged

Mytherin merged commit dea2294 into duckdb:main Mar 11, 2024
48 of 49 checks passed

carlopi mentioned this pull request Mar 16, 2024

inet: parsing ipv6 addresses with leading :: doesn't work #11195

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement IPv6 support in the inet extension. #10839

Implement IPv6 support in the inet extension. #10839

troycurtisjr commented Feb 25, 2024 •

edited

Loading

Mytherin left a comment

Mytherin Feb 26, 2024

troycurtisjr Feb 27, 2024

troycurtisjr Mar 2, 2024

Mytherin Feb 26, 2024

troycurtisjr Feb 27, 2024

Mytherin Feb 26, 2024

troycurtisjr Feb 27, 2024

Mytherin Feb 26, 2024

troycurtisjr Feb 27, 2024

troycurtisjr Mar 2, 2024

Mytherin Feb 26, 2024

troycurtisjr commented Feb 27, 2024

troycurtisjr commented Mar 2, 2024

carlopi commented Mar 7, 2024 •

edited

Loading

		using INET_TYPE = StructTypeTernary<uint8_t, hugeint_t, uint16_t>;

		static uhugeint_t from_compat_addr(hugeint_t compat_addr, IPAddressType addr_type) {

Implement IPv6 support in the inet extension. #10839

Implement IPv6 support in the inet extension. #10839

Conversation

troycurtisjr commented Feb 25, 2024 • edited Loading

Mytherin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troycurtisjr commented Feb 27, 2024

troycurtisjr commented Mar 2, 2024

carlopi commented Mar 7, 2024 • edited Loading

troycurtisjr commented Feb 25, 2024 •

edited

Loading

carlopi commented Mar 7, 2024 •

edited

Loading