Add IPPrefix Presto type #10816

mohsaka · 2024-08-22T20:18:30Z

Written with similar comments addressed from #10650
Part of #10538

Some comments.

IPPREFIX uses VARBINARY type, therefore it should not be subject to the same issue as IPADDRESS Comparisons for values of logical types are not handled correctly throughout the library #10338.
The cast to and from functions for IPADDRESS <-> IPPREFIX are both in the IPADDRESS and IPPREFIX. This is not necessary since castTo gets priority. However isSupported* is necessary for the check. I could remove the castFroms and just return in the castFrom function since it will never be called.
Limited fuzzer will be added later.
auto const vec = splitIpSlashCidr(ipAddressString); is not outside the switch statement for efficiency. Don't want to re-parse if not necessary. Folly does the same but in a different way which seemed a little messy.
https://github.com/facebook/folly/blob/556e17ac7b1f6cda68de789d5b06e79b2d3b990f/folly/IPAddress.cpp#L84C24-L84C37

netlify · 2024-08-22T20:18:45Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`b21aafa`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/66f596d0547b0700084b123a

czentgr

Thank you!

velox/docs/develop/types.rst

velox/docs/functions/presto/conversion.rst

velox/functions/prestosql/types/IPAddressType.cpp

velox/functions/prestosql/types/IPPrefixType.cpp

velox/functions/prestosql/tests/IPPrefixCastTest.cpp

czentgr

Thanks!
@mbasmanova when you get a chance can you please take a look?

mbasmanova

@mohsaka Thank you for adding this. Looks good, but I haven't read every line. I assume @czentgr has reviewed carefully. Let me know if that's not the case.

velox/docs/develop/types.rst

velox/docs/functions/presto/conversion.rst

velox/functions/prestosql/TypeOf.cpp

velox/functions/prestosql/tests/IPAddressCastTest.cpp

velox/functions/prestosql/types/IPAddressType.cpp

velox/functions/prestosql/types/UuidType.cpp

velox/functions/prestosql/types/IPPrefixType.cpp

mbasmanova · 2024-08-30T04:57:24Z

velox/functions/prestosql/types/IPPrefixType.cpp

+      exec::StringWriter<false> result(flatResult, row);
+      if (v6Addr.isIPv4Mapped()) {
+        result.append(
+            v6Addr.createIPv4().str() + "/" +


Can we avoid creating and copying intermediate strings?

Changed to fmt::format. Is this what you meant? Please let me know. Thanks!

mbasmanova · 2024-08-30T04:57:48Z

velox/functions/prestosql/types/IPPrefixType.cpp

+    });
+  }
+
+  static auto splitIpSlashCidr(folly::StringPiece ipSlashCidr) {


Can we use std::string_view?

I can switch to string view, but the function will get a little messy in that it will be switched to a find + creation of a vector<string_view> + creation of these string_views. This is used for error reporting and is taken straight from

https://github.com/facebook/folly/blob/556e17ac7b1f6cda68de789d5b06e79b2d3b990f/folly/IPAddress.cpp#L75

Please let me know. Thanks!

Update.
Maybe misinterpreted the comment. Changed function argument to std::string_view. Function itself is still using StringPiece.

Reverted back to StringPiece, however switched to const &. string_view caused string output issue.

Expected error message to contain 'Invalid IP address '2001:db8::1::1'', but received 'Invalid IP address 'ck\0\0\0\0\0\0_��''.

velox/functions/prestosql/types/IPPrefixType.cpp

czentgr

@mbasmanova Thank you for your review. But I was not nearly careful enough given the issues you found that are real. I will have to re-review after all this is fixed.

velox/functions/prestosql/types/IPAddressType.cpp

czentgr

Looks good. Only some minor comments.

velox/docs/functions/presto/conversion.rst

velox/functions/prestosql/types/IPPrefixType.cpp

czentgr · 2024-09-13T01:49:32Z

@mohsaka looks like one of the functions tests is failing. Might be related to your new type. Please take a look.

aditi-pandit

@mohsaka : Did a very high level review. The code looks good. Will read it closely again.

velox/functions/prestosql/tests/IPPrefixCastTest.cpp

velox/functions/prestosql/types/IPAddressType.cpp

velox/functions/prestosql/types/IPPrefixType.cpp

czentgr · 2024-09-19T19:44:21Z

@mohsaka Please squash your commits. Otherwise, looks good.

czentgr

Minor question on the doc external links.

velox/docs/functions/presto/conversion.rst

mohsaka · 2024-09-25T16:17:19Z

@mbasmanova Could I get a review when you have the time? Thank you!

velox/functions/prestosql/types/IPAddressType.cpp

aditi-pandit · 2024-09-26T06:00:28Z

velox/functions/prestosql/types/IPPrefixType.cpp

+    return vec;
+  }
+
+  static void castFromString(


Is this logic consistent with Java ? Would the same invalid values return the same errors ?

Not the same error message, but will return an error for the same input as java. The reason is that we are using folly for the parsing etc. I think a lot of changes will be necessary in the velox code if we want all of the error messages to match java for any library we are using, ex boost, folly.

@mohsaka : I agree that it might not be possible to use the same errors in all places. But we should be as consistent as we can else its very hard during any kind of traffic migration to validate the builds. Meta has big issues with it.

In general, I feel it might be good to split this work as follows:
i) First PR just adds all the type classes for the IPPrefixType. So this PR minus all the casting code.
ii) Add the fuzzer for IPPrefix Casts that compares with Presto.
iii) Add the cast code with the fuzzer in place. That will ensure that we have most consistency. This would also make it simpler to validate the code you have in IPPrefixCastTest.

Based on prior experience I feel the fuzzer should not be an afterthought.

@amitkdutta @kagamiori @kgpai

velox/functions/prestosql/types/IPPrefixType.cpp

velox/functions/prestosql/types/UuidType.cpp

Summary: Requested here to split off from the PR. #10816 (comment) Pull Request resolved: #11105 Reviewed By: Yuhta, amitkdutta Differential Revision: D63475266 Pulled By: kagamiori fbshipit-source-id: 11c7830b1cd4b665bc7915fd59b52673a7d39537

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

Current split PR for IPPREFIX type only here:
#11122

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 Aug 22, 2024

mohsaka changed the title ~~Add IPPREFIX type~~ Add IPPrefix Presto type Aug 26, 2024

czentgr reviewed Aug 26, 2024

View reviewed changes

mohsaka requested a review from czentgr August 27, 2024 00:14

mohsaka force-pushed the ipprefix_final branch 2 times, most recently from 7f4a345 to 325df43 Compare August 28, 2024 16:23

czentgr reviewed Aug 28, 2024

View reviewed changes

mohsaka force-pushed the ipprefix_final branch 4 times, most recently from 9cbaa6e to a0e4692 Compare August 29, 2024 08:28

czentgr reviewed Aug 29, 2024

View reviewed changes

mbasmanova reviewed Aug 30, 2024

View reviewed changes

czentgr reviewed Aug 30, 2024

View reviewed changes

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

mohsaka requested a review from czentgr September 9, 2024 22:12

czentgr reviewed Sep 12, 2024

View reviewed changes

mohsaka force-pushed the ipprefix_final branch 2 times, most recently from c0ced7d to f8bc3d7 Compare September 13, 2024 16:47

aditi-pandit reviewed Sep 16, 2024

View reviewed changes

velox/functions/prestosql/tests/IPPrefixCastTest.cpp Show resolved Hide resolved

velox/functions/prestosql/types/IPAddressType.cpp Show resolved Hide resolved

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

mohsaka requested review from czentgr and aditi-pandit September 16, 2024 17:00

mohsaka force-pushed the ipprefix_final branch from 0ec917a to 946ff03 Compare September 20, 2024 16:09

czentgr reviewed Sep 23, 2024

View reviewed changes

velox/docs/functions/presto/conversion.rst Outdated Show resolved Hide resolved

mohsaka force-pushed the ipprefix_final branch from 946ff03 to 17cbb8c Compare September 23, 2024 17:51

mohsaka requested a review from czentgr September 23, 2024 17:53

mohsaka force-pushed the ipprefix_final branch 2 times, most recently from 046954c to d609537 Compare September 23, 2024 19:07

czentgr reviewed Sep 23, 2024

View reviewed changes

velox/docs/functions/presto/conversion.rst Outdated Show resolved Hide resolved

mohsaka force-pushed the ipprefix_final branch from d609537 to 2c1688c Compare September 23, 2024 19:39

mohsaka requested a review from czentgr September 23, 2024 20:07

czentgr approved these changes Sep 23, 2024

View reviewed changes

mohsaka requested a review from mbasmanova September 23, 2024 20:25

aditi-pandit reviewed Sep 26, 2024

View reviewed changes

Add IPPREFIX

b21aafa

mohsaka force-pushed the ipprefix_final branch from 4f4c6e7 to b21aafa Compare September 26, 2024 17:15

mohsaka requested a review from aditi-pandit September 26, 2024 17:16

mohsaka mentioned this pull request Sep 26, 2024

Small PR to fix error message #11105

Closed

mohsaka mentioned this pull request Sep 27, 2024

Add IPPREFIX type #11122

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IPPrefix Presto type #10816

Add IPPrefix Presto type #10816

mohsaka commented Aug 22, 2024 •

edited

Loading

netlify bot commented Aug 22, 2024 •

edited

Loading

czentgr left a comment

czentgr left a comment

mbasmanova left a comment

mbasmanova Aug 30, 2024

mohsaka Sep 9, 2024

mbasmanova Aug 30, 2024

mohsaka Sep 9, 2024

mohsaka Sep 9, 2024

mohsaka Sep 16, 2024

czentgr left a comment

czentgr left a comment

czentgr commented Sep 13, 2024

aditi-pandit left a comment

czentgr commented Sep 19, 2024

czentgr left a comment

mohsaka commented Sep 25, 2024

aditi-pandit Sep 26, 2024

mohsaka Sep 26, 2024 •

edited

Loading

aditi-pandit Sep 26, 2024 •

edited

Loading

mohsaka commented Sep 27, 2024

Add IPPrefix Presto type #10816

Are you sure you want to change the base?

Add IPPrefix Presto type #10816

Conversation

mohsaka commented Aug 22, 2024 • edited Loading

netlify bot commented Aug 22, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

czentgr left a comment

Choose a reason for hiding this comment

czentgr left a comment

Choose a reason for hiding this comment

mbasmanova left a comment

Choose a reason for hiding this comment

mbasmanova Aug 30, 2024

Choose a reason for hiding this comment

mohsaka Sep 9, 2024

Choose a reason for hiding this comment

mbasmanova Aug 30, 2024

Choose a reason for hiding this comment

mohsaka Sep 9, 2024

Choose a reason for hiding this comment

mohsaka Sep 9, 2024

Choose a reason for hiding this comment

mohsaka Sep 16, 2024

Choose a reason for hiding this comment

czentgr left a comment

Choose a reason for hiding this comment

czentgr left a comment

Choose a reason for hiding this comment

czentgr commented Sep 13, 2024

aditi-pandit left a comment

Choose a reason for hiding this comment

czentgr commented Sep 19, 2024

czentgr left a comment

Choose a reason for hiding this comment

mohsaka commented Sep 25, 2024

aditi-pandit Sep 26, 2024

Choose a reason for hiding this comment

mohsaka Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

aditi-pandit Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

mohsaka commented Sep 27, 2024

mohsaka commented Aug 22, 2024 •

edited

Loading

netlify bot commented Aug 22, 2024 •

edited

Loading

mohsaka Sep 26, 2024 •

edited

Loading

aditi-pandit Sep 26, 2024 •

edited

Loading