Add IPPREFIX type #11122
Add IPPREFIX type #11122
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
|
@aditi-pandit @czentgr Can I get a review on this? Thank you! |
|
|
||
| namespace facebook::velox { | ||
|
|
||
| class IPPrefixType : public VarbinaryType { |
There was a problem hiding this comment.
It's a little bit waste of memory to use VARBINARY as physical type here: StringView needs 16 bytes and the string buffer needs 17 bytes per row, totally 33 bytes per row.
@mbasmanova Is this because the usage of this type is rare and we want to trade off some performance for simpler implementation? How about use ROW(HUGEINT, TINYINT)?
There was a problem hiding this comment.
Some more information, I did try to attempt to create the type via an opaque type. However it seems that there is no support for encoding, column reader, and I presume further down. The java type is a fixed width type of 17 bytes.
There was a problem hiding this comment.
Did you try ROW(HUGEINT, TINYINT)? This seems the most efficient way of storing it. Also the conversion between IPPREFIX and IPADDRESS will be a no cost operation.
There is no way to read these types right out of reader anyway, you need to project a cast.
There was a problem hiding this comment.
@Yuhta Thank you for the suggestion. Made change to Row type with overridden arguments method. Please confirm if that is what you were thinking?
Thanks again!
mohsaka
force-pushed
the
ipprefix_type_only
branch
from
55bcd68
to
0f187a3
Compare
There was a problem hiding this comment.
@mohsaka : Some minor comments. But ready for review from Meta engineers.
| @@ -16,11 +16,13 @@ | |||
| #pragma once | |||
|
|
|||
| #include "velox/functions/prestosql/types/IPAddressType.h" | |||
There was a problem hiding this comment.
Maybe add some tests in FunctionRegistryTest or CustomTypeTest to ensure that we can register functions with type IPPREFIX and retrieve them ?
There was a problem hiding this comment.
Added simple function registration and resolution test into FunctionRegistryTest
mohsaka
force-pushed
the
ipprefix_type_only
branch
from
f0161e8
to
2096795
Compare
mohsaka
force-pushed
the
ipprefix_type_only
branch
from
2096795
to
96c3bc4
Compare
|
@Yuhta Could I get a review please. Thanks you! |
This PR only adds the IPPrefix type classes. CAST logic is not implemented. The next PR for IPPrefix type will enhance the fuzzers for IPPrefix type. After that we will add the CAST logic so that it can be tested with fuzzers from the start itself.
The full logic for IPPrefix is available in PRs :
Original PR: #10538
Original Split PR: #10816