Add native IPv4 and IPv6 types support #73
Conversation
| ch_type = "IPv4" | ||
| py_types = compat.string_types + (IPv4Address, int) | ||
|
|
||
| def check_item_type(self, value): |
There was a problem hiding this comment.
It seems than clickhouse_driver.columns.base.Column has the same check_item_type.
There was a problem hiding this comment.
Yes, I have removed it. However IntColumn and UIntColumn override the types_check methods, so I've added a condition in the init to properly handle types_check=True on IP columns (added matching tests)
| from .intcolumn import UInt32Column | ||
|
|
||
|
|
||
| class IPv4(UInt32Column): |
There was a problem hiding this comment.
Let's add "Column" suffix to class name: class IPv4Column
| from ipaddress import IPv6Address, IPv4Address | ||
|
|
||
|
|
||
| class IPv4TestCase(BaseTestCase): |
There was a problem hiding this comment.
Feel free to add new ClickHouse release with IP addresses support to travis matrix: https://github.com/mymarilyn/clickhouse-driver/blob/master/.travis.yml#L2
There was a problem hiding this comment.
To skip tests for versions that do not support IP addresses use require_server_version decorator: https://github.com/mymarilyn/clickhouse-driver/blob/master/tests/columns/test_decimal.py#L12.
There was a problem hiding this comment.
Check travis for build status: https://travis-ci.org/mymarilyn/clickhouse-driver/pull_requests
There was a problem hiding this comment.
I've just pushed branch https://github.com/mymarilyn/clickhouse-driver/tree/feature-version-dependent-settings-in-test where Decimal tests run without errors with latest server version. You can use it as branchpoint for making all tests green.
There was a problem hiding this comment.
I've added the decorator on all tests, this should work now. Note that tests for the Decimal column will fail on 19.3.3 because the --allow_experimental_decimal_type flag was removed in 18.14.9 (it is enabled by default now), so we may want to add an upper version check decorator
|
|
||
| class IPv6(ByteFixedString): | ||
| ch_type = "IPv6" | ||
| py_types = IPv6Address, bytes |
There was a problem hiding this comment.
py_types = compat.string_types + (IPv6Address, bytes)?
There was a problem hiding this comment.
My mistake, fixed (the test now enforce the type check to ensure the type is in py_types)
| ch_type = "IPv6" | ||
| py_types = IPv6Address, bytes | ||
|
|
||
| def check_item_type(self, value): |
There was a problem hiding this comment.
This method is also redundant.
|
Great. Do you want to add IP support to |
|
Yes, I'm already working on it. Hopefully I will be able to open a PR for this by the end of the month |
ClickHouse 19.3.3 added IPv4 and IPv6 data types (not yet documented), see ClickHouse/ClickHouse#3669
This PR adds IPv4 and IPv6 columns to match these new types (implemented as domains in ClickHouse):
The matching error code (generic domain error in ClickHouse, not specific to IP Addresses) is also added.
For convenience, the IPv4 column here supports input as integer, string or IPv4Address; the IPv6 column supports raw bytes, string or IPv6Address.
Both testcases are passing on Python 3 and Python 2.7, although the ipaddress module was added in Python 3.3 so I had to add the backport for Python 2.7 in the dependencies.
I thought about adding checks on the server revision number to ensure it supports IP types, but it will already properly return a type not supported error if needed, so I don't think this is necessary.