Add IPPREFIX type by mohsaka · Pull Request #11122 · facebookincubator/velox

mohsaka · 2024-09-27T23:19:23Z

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

netlify · 2024-09-27T23:19:38Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`96c3bc4`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/670f1be132460a0008c875e6

mohsaka · 2024-09-30T18:31:11Z

@aditi-pandit @czentgr Can I get a review on this? Thank you!

aditi-pandit

Thanks @mohsaka. Looks good overall minus these comments.

velox/functions/prestosql/types/IPPrefixType.cpp

velox/docs/develop/types.rst

Yuhta · 2024-10-01T22:46:33Z

velox/functions/prestosql/types/IPPrefixType.h

+
+namespace facebook::velox {
+
+class IPPrefixType : public VarbinaryType {


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)?

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.

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.

@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!

velox/docs/develop/types.rst

velox/functions/prestosql/types/IPPrefixType.h

czentgr

Thanks!

velox/functions/prestosql/types/IPPrefixType.cpp

aditi-pandit

@mohsaka : Some minor comments. But ready for review from Meta engineers.

velox/functions/prestosql/types/IPPrefixType.cpp

aditi-pandit · 2024-10-14T05:21:42Z

velox/functions/prestosql/IPAddressFunctions.h

Maybe add some tests in FunctionRegistryTest or CustomTypeTest to ensure that we can register functions with type IPPREFIX and retrieve them ?

Added simple function registration and resolution test into FunctionRegistryTest

mohsaka · 2024-10-16T18:28:18Z

@Yuhta Could I get a review please. Thanks you!

facebook-github-bot · 2024-10-24T18:52:01Z

@pedroerp has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-10-28T18:10:03Z

@pedroerp merged this pull request in c8ac4e3.

conbench-facebook · 2024-10-28T18:39:20Z

Conbench analyzed the 1 benchmark run on commit c8ac4e3a.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Summary: Support cast for ipprefix for varchar. Based off of facebookincubator#11122 Will do IPAddress in follow up diff Differential Revision: D65449935

Summary: Pull Request resolved: #11460 Support cast for ipprefix for varchar. Based off of #11122 Will do IPAddress in follow up diff Reviewed By: spershin Differential Revision: D65449935 fbshipit-source-id: 2b87c8942309a783fcf4a48823d618a9144cf151

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 27, 2024

mohsaka mentioned this pull request Sep 27, 2024

Add IPPrefix Presto type #10816

Closed

aditi-pandit reviewed Oct 1, 2024

View reviewed changes

velox/functions/prestosql/types/IPPrefixType.cpp Outdated Show resolved Hide resolved

velox/docs/develop/types.rst Show resolved Hide resolved

aditi-pandit changed the title ~~Add IPPREFIX type only~~ Add IPPREFIX type Oct 1, 2024

Yuhta requested a review from mbasmanova October 1, 2024 19:44

Yuhta reviewed Oct 1, 2024

View reviewed changes

czentgr reviewed Oct 3, 2024

View reviewed changes

velox/docs/develop/types.rst Outdated Show resolved Hide resolved

velox/functions/prestosql/types/IPPrefixType.h Show resolved Hide resolved

mohsaka requested review from Yuhta, aditi-pandit and czentgr October 3, 2024 16:35

czentgr approved these changes Oct 4, 2024

View reviewed changes

mohsaka mentioned this pull request Oct 9, 2024

test: Add IPPREFIX to the velox fuzzer test #11209

Closed

mohsaka requested review from assignUser and majetideepak as code owners October 11, 2024 20:48

mohsaka force-pushed the ipprefix_type_only branch from 55bcd68 to 0f187a3 Compare October 11, 2024 21:01

aditi-pandit reviewed Oct 14, 2024

View reviewed changes

velox/functions/prestosql/types/IPPrefixType.cpp Outdated Show resolved Hide resolved

mohsaka requested a review from aditi-pandit October 14, 2024 05:23

aditi-pandit approved these changes Oct 14, 2024

View reviewed changes

mohsaka force-pushed the ipprefix_type_only branch from f0161e8 to 2096795 Compare October 16, 2024 01:24

mohsaka added 2 commits October 15, 2024 18:31

Merge

6eed5fb

Fix conflict

96c3bc4

mohsaka force-pushed the ipprefix_type_only branch from 2096795 to 96c3bc4 Compare October 16, 2024 01:50

mohsaka mentioned this pull request Oct 21, 2024

Add IPPREFIX to/from VARCHAR casting #11309

Closed

Yuhta approved these changes Oct 24, 2024

View reviewed changes

Yuhta added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Oct 24, 2024

facebook-github-bot closed this in c8ac4e3 Oct 28, 2024

facebook-github-bot added the Merged label Oct 28, 2024

yuandagits mentioned this pull request Nov 6, 2024

Add ipprefix cast operators for varchar [1/n] #11460

Closed

mohsaka deleted the ipprefix_type_only branch March 30, 2026 16:49


		namespace facebook::velox {

		class IPPrefixType : public VarbinaryType {

Conversation

mohsaka commented Sep 27, 2024 • edited by aditi-pandit Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox canceled.

Uh oh!

mohsaka commented Sep 30, 2024

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Yuhta Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohsaka Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

Yuhta Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohsaka Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

czentgr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aditi-pandit Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

mohsaka Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

mohsaka commented Oct 16, 2024

Uh oh!

facebook-github-bot commented Oct 24, 2024

Uh oh!

facebook-github-bot commented Oct 28, 2024

Uh oh!

conbench-facebook bot commented Oct 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mohsaka commented Sep 27, 2024 •

edited by aditi-pandit

Loading

netlify bot commented Sep 27, 2024 •

edited

Loading

Yuhta Oct 1, 2024 •

edited

Loading

Yuhta Oct 2, 2024 •

edited

Loading