-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add IPADDRESS and IPPREFIX with functions #10538
Conversation
✅ Deploy Preview for meta-velox canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mohsaka Thank you for adding these types and functions that use them. This PR is rather large. Would it be possible to break it up into smaller pieces? For example, one PR to add IPADDRESS type + operators (CAST, etc.), another to add IPPREFIX type, and a third PR to add functions.
Let's makes sure that all operators for these types are included. Specifically, make sure that all CASTs supported in Presto are supported in Velox as well.
VELOX_UNSUPPORTED( "Cast from {} to IPAddress not yet supported", resultType->toString());
"not yet supported" in the error message above suggests that there might be CASTs supported in Presto, but not Velox. Let's make sure to include all supported casts.
For CASTs from strings, make sure these are implemented efficiently and do not generate exceptions unnecessarily. See https://velox-lib.io/blog/optimize-try-more
Thank you for the review. Took a look. It shouldn't be affected by this as the binary to HUGEINT is a 1 to 1 mapping. We don't have multiple representatives of the same value. If you were thinking of the case where we have an IPV4 and an IPV6 with the same value, we automatically assume IPV4 converted to IPV6. EX.
We can see that IPV6 ::ffff:ffff:ffff Will be assumed to be IPV4 255.255.255.255 rather than IPV6 of value ::ffff:ffff:ffff. Will also break up the PR. Thanks! |
@mohsaka : Thanks for your PR. Splitting is a good idea. Also, it would be great if we had a limited fuzzer support for validating function results. It is very hard to verify results without it. Christian has a limited fuzzer for CAST expressions https://github.com/facebookincubator/velox/pull/10523/files#diff-324086e31fd4ee807d8a0e8234490cf68550a0501eb5e955ac499e21f71c320c. Would be great if we can do a similar one for IPAddress and IPPrefix as well. |
Summary: Split of #10538 In response to mbasmanova : #10538 (review) Took a look and added the to and from varbinary casting Contains CAST functions for IPAddress <-> Varchar and IPAddress <-> Varbinary utilizing the folly library. In response to: https://velox-lib.io/blog/optimize-try-more I believe the folly library utilizes makeUnexpected already. https://github.com/facebook/folly/blob/main/folly/IPAddress.cpp I added one error for Varbinary conversion utilizing `context.setStatus` If I'm not understanding something correctly please let me know. As for adding a limited fuzzer aditi-pandit #10538 (comment) Is this to test the casting `IPAddress <-> Varchar and IPAddress <-> Varbinary` or something else. We already test the casting using the duckdb type, we also have the prestissimo test in the E2E test. Fuzzer support will be added in after the types and functions have been added. Pull Request resolved: #10596 Reviewed By: bikramSingh91 Differential Revision: D61163377 Pulled By: xiaoxmeng fbshipit-source-id: 2c0fa2e8e6d250a2167a1ff82b2878548798a226
Issue #7591
E2E Tests at
prestodb/presto#23282
IPADDRESS/IPPREFIX
Types
IPADDRESS (HUGEINT)
IPPREFIX ({HUGEINT, TINYINT})
Functions
NOTES:
We cannot apply IPV4 operations on an IPV4 converted IPV6 address.
Ex. We can apply a length 100 prefix on an IPV4 converted address. So we need
to always convert IPV6 to IPV4 if possible.
All error checking is done with the IPPREFIX and IPADDRESS types. We assume if they
are that type then they are valid.
cast(ipaddress as varchar) → varchar
cast(ipaddressString as IPADDRESS) → IPADDRESS
cast(ipprefix as VARCHAR) → VARCHAR
cast(ipprefixString as IPADDRESS) → IPPREFIX
ip_prefix(ipaddress, prefix_bits) → IPPREFIX
ip_subnet_min(ipprefix) → IPADDRESS
ip_subnet_max(ipprefix) → IPADDRESS
ip_subnet_range(ipprefix) → [IPADDRESS, IPADDRESS]
is_subnet_of(ipprefix, ipaddress) → BOOLEAN
is_subnet_of(ipprefix1, ipprefix2) → BOOLEAN