Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Add ip_subnet_max/min [5/n] #11515

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: Add ip_subnet_max/min [5/n] #11515

wants to merge 1 commit into from

Conversation

yuandagits
Copy link
Contributor

Summary:
Add ip_subnet and ip_subnetmax.

Split from #11407

Differential Revision: D65833249

@facebook-github-bot 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 Nov 12, 2024
@netlify Netlify
Copy link

netlify bot commented Nov 12, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 2ba025d
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/67808567197bf0000890840c

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65833249

yuandagits added a commit to yuandagits/velox that referenced this pull request Nov 12, 2024
@yuandagits @facebook-github-bot
Add ip_subnet_max/min [4/n] (facebookincubator#11515)
fb57c78 
Summary:

Add ip_subnet and ip_subnetmax.

Split from facebookincubator#11407

Differential Revision: D65833249
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65833249

yuandagits added a commit to yuandagits/velox that referenced this pull request Nov 12, 2024
@yuandagits @facebook-github-bot
Add ip_subnet_max/min [4/n] (facebookincubator#11515)
72f28a7 
Summary:

Add ip_subnet and ip_subnetmax.

Split from facebookincubator#11407

Differential Revision: D65833249
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65833249

yuandagits added a commit to yuandagits/velox that referenced this pull request Nov 12, 2024
@yuandagits @facebook-github-bot
Add ip_subnet_max/min [4/n] (facebookincubator#11515)
51361cd 
Summary:

Add ip_subnet and ip_subnetmax.

Split from facebookincubator#11407

Differential Revision: D65833249
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65833249

yuandagits added a commit to yuandagits/velox that referenced this pull request Nov 12, 2024
@yuandagits @facebook-github-bot
Add ip_subnet_max/min [4/n] (facebookincubator#11515)
8216001 
Summary:

Add ip_subnet and ip_subnetmax.

Split from facebookincubator#11407

Differential Revision: D65833249
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65833249

yuandagits added a commit to yuandagits/velox that referenced this pull request Nov 15, 2024
@yuandagits @facebook-github-bot
Add ip_subnet_max/min [5/n] (facebookincubator#11515)
4cecdb3 
Summary:

Add ip_subnet and ip_subnetmax.

Split from facebookincubator#11407

Differential Revision: D65833249
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65833249

@yuandagits yuandagits changed the title Add ip_subnet_max/min [4/n] feat: Add ip_subnet_max/min [4/n] Nov 20, 2024
yuandagits added a commit to yuandagits/velox that referenced this pull request Dec 6, 2024
@yuandagits @facebook-github-bot
Add ip_subnet_max/min [5/n] (facebookincubator#11515)
ae275bd 
Summary:

Add ip_subnet and ip_subnetmax.

Split from facebookincubator#11407

Differential Revision: D65833249
yuandagits added a commit to yuandagits/velox that referenced this pull request Dec 6, 2024
@yuandagits @facebook-github-bot
Add ip_subnet_max/min [5/n] (facebookincubator#11515)
5eac96f 
Summary:

Add ip_subnet and ip_subnetmax.

Split from facebookincubator#11407

Differential Revision: D65833249
@yuandagits yuandagits changed the title feat: Add ip_subnet_max/min [4/n] feat: Add ip_subnet_max/min [5/n] Dec 12, 2024
yuandagits added a commit to yuandagits/velox that referenced this pull request Dec 18, 2024
@yuandagits @facebook-github-bot
Add ip_subnet_max/min [5/n] (facebookincubator#11515)
6e6fa66 
Summary:

Add ip_subnet and ip_subnetmax.

Split from facebookincubator#11407

Differential Revision: D65833249
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65833249

yuandagits added a commit to yuandagits/velox that referenced this pull request Dec 22, 2024
@yuandagits @facebook-github-bot
Add ip_subnet_max/min [5/n] (facebookincubator#11515)
69aaeb7 
Summary:

Add ip_subnet and ip_subnetmax.

Split from facebookincubator#11407

Differential Revision: D65833249
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65833249

yuandagits added a commit to yuandagits/velox that referenced this pull request Dec 22, 2024
@yuandagits @facebook-github-bot
Add ip_subnet_max/min [5/n] (facebookincubator#11515)
8eacbe0 
Summary:

Add ip_subnet and ip_subnetmax.

Split from facebookincubator#11407

Differential Revision: D65833249
@yuandagits
Copy link
Contributor Author

cc @mohsaka

yuandagits added a commit to yuandagits/velox that referenced this pull request Jan 7, 2025
@yuandagits @facebook-github-bot
Add ip_subnet_max/min [5/n] (facebookincubator#11515)
a1284e9 
Summary:

Add ip_subnet and ip_subnetmax.

Split from facebookincubator#11407

Differential Revision: D65833249
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65833249

@yuandagits yuandagits requested a review from xiaoxmeng January 7, 2025 00:28
mohsaka
mohsaka approved these changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@mohsaka mohsaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

yuandagits added a commit to yuandagits/velox that referenced this pull request Jan 8, 2025
@yuandagits @facebook-github-bot
Add ip_subnet_max/min [5/n] (facebookincubator#11515)
aacbe26 
Summary:

Add ip_subnet and ip_subnetmax.

Split from facebookincubator#11407

Differential Revision: D65833249
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65833249

yuandagits added a commit to yuandagits/velox that referenced this pull request Jan 8, 2025
@yuandagits @facebook-github-bot
Add ip_subnet_max/min [5/n] (facebookincubator#11515)
37281a0 
Summary:

Add ip_subnet and ip_subnetmax.

Split from facebookincubator#11407

Differential Revision: D65833249
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65833249

yuandagits added a commit to yuandagits/velox that referenced this pull request Jan 8, 2025
@yuandagits @facebook-github-bot
Add ip_subnet_max/min [5/n] (facebookincubator#11515)
751d03f 
Summary:

Add ip_subnet and ip_subnetmax.

Split from facebookincubator#11407

Differential Revision: D65833249
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65833249

yuandagits added a commit to yuandagits/velox that referenced this pull request Jan 8, 2025
@yuandagits @facebook-github-bot
Add ip_subnet_max/min [5/n] (facebookincubator#11515)
7b49216 
Summary:

Add ip_subnet and ip_subnetmax.

Split from facebookincubator#11407

Differential Revision: D65833249
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65833249

@yuandagits yuandagits marked this pull request as draft January 8, 2025 23:42
yuandagits added a commit to yuandagits/velox that referenced this pull request Jan 8, 2025
@yuandagits @facebook-github-bot
Add ip_subnet_max/min [5/n] (facebookincubator#11515)
8b8ad76 
Summary:

Add ip_subnet and ip_subnetmax.

Split from facebookincubator#11407

Differential Revision: D65833249
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65833249

yuandagits added a commit to yuandagits/velox that referenced this pull request Jan 9, 2025
@yuandagits @facebook-github-bot
Add ip_subnet_max/min [5/n] (facebookincubator#11515)
4995804 
Summary:

Add ip_subnet and ip_subnetmax.

Split from facebookincubator#11407

Differential Revision: D65833249
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65833249

xiaoxmeng
xiaoxmeng reviewed Jan 9, 2025
View reviewed changes
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuandagits thanks for the change % minors

velox/functions/prestosql/IPAddressFunctions.h Outdated

private:
static int128_t getIPSubnetMax(int128_t ip, uint8_t prefix) {
auto tryIsIpv4 = ipaddress::tryIpPrefixLengthFromIPAddressType(ip);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

s/tryIsIpv4/tryIpv4/

velox/functions/prestosql/IPAddressFunctions.h Outdated
auto tryIsIpv4 = ipaddress::tryIpPrefixLengthFromIPAddressType(ip);
// This check should never fail because we're taking a pre-existing
// IPPrefix.
VELOX_DCHECK(tryIsIpv4.hasValue());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add VELOX_CHECK here and it is not on hotpath.

velox/functions/prestosql/IPAddressFunctions.h
}

// Special case: Overflow to all 0 subtracting 1 does not work.
if (prefix == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we check prefix first or ipv4 allows 0 prefix for subnet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ipv4 is okay because ip |= (mask << (ipaddress::kIPV4Bits - prefix)) - 1; won't overflow where as for ipv6 it will

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curious in protocol, does ipv4 allows a subnet with zero prefix? The prefix here is number of bits used for subnet? Thanks!

@yuandagits @facebook-github-bot
Add ip_subnet_max/min [5/n] (facebookincubator#11515)
2ba025d 
Summary:

Add ip_subnet and ip_subnetmax.

Split from facebookincubator#11407

Differential Revision: D65833249
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D65833249

xiaoxmeng
xiaoxmeng approved these changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuandagits LGTM. Thanks for the update!

velox/functions/prestosql/IPAddressFunctions.h
}

// Special case: Overflow to all 0 subtracting 1 does not work.
if (prefix == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curious in protocol, does ipv4 allows a subnet with zero prefix? The prefix here is number of bits used for subnet? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mohsaka mohsaka mohsaka approved these changes

@xiaoxmeng xiaoxmeng xiaoxmeng approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yuandagits @facebook-github-bot @xiaoxmeng @mohsaka