Add IPAddress java library as dependency and migrate IPv4 functions to use the new library. #11634

abhishekrb19 · 2021-08-25T17:25:14Z

This change-set does the following:

Include a new IPAddress java library dependency to handle IP addresses.
Migrate the IPv4 address functions -- IPV4_MATCH(), IPV4_PARSE(), IPV4_STRINGIFY() to use the above package inet.ipaddr instead of the org.apache.commons.net, com.google.common.net and java.net packages.

Rationale:

The org.apache.commons.net.util.SubnetUtils class doesn't support IPv6 addresses yet. Please see the open JIRA https://issues.apache.org/jira/browse/NET-405. In order to support IPV6 address functions, going down the path of writing custom regexes, parser and longest string matching functions can be tricky given the various IPv6 address formats. The newly added ipaddress library supports these right out of the box, so we could leverage that.
In the future, if there is a desire to expose the IP address parsing options as run time context parameters or similar, it can be plumbed in relatively easily since the java library provides flexibility there.

Next steps:
If this looks good, I will go ahead and make an attempt at adding the IPv6 address functions. Open to suggestions. Thanks!

Benchmark comparison between the new ipaddress package and the old subnet utils package:

IPv4AddressBenchmark.parseStringUsingIpAddr                        10  avgt    5     6185.716 ±    220.531  ns/op
IPv4AddressBenchmark.parseStringUsingIpAddr                       100  avgt    5    64528.912 ±  10058.256  ns/op
IPv4AddressBenchmark.parseStringUsingIpAddr                      1000  avgt    5   617576.564 ± 129451.314  ns/op

IPv4AddressBenchmark.parseStringUsingSubnetUtils                   10  avgt    5     7972.548 ±   3246.404  ns/op
IPv4AddressBenchmark.parseStringUsingSubnetUtils                  100  avgt    5    51201.575 ±  68695.599  ns/op
IPv4AddressBenchmark.parseStringUsingSubnetUtils                 1000  avgt    5   691529.605 ± 704383.570  ns/op

IPv4AddressBenchmark.parseLongUsingIpAddr                          10  avgt    5      258.954 ±      2.800  ns/op
IPv4AddressBenchmark.parseLongUsingIpAddr                         100  avgt    5     2466.993 ±      7.883  ns/op
IPv4AddressBenchmark.parseLongUsingIpAddr                        1000  avgt    5    24735.502 ±    228.493  ns/op

IPv4AddressBenchmark.parseLongUsingSubnetUtils                     10  avgt    5        8.147 ±      0.549  ns/op
IPv4AddressBenchmark.parseLongUsingSubnetUtils                    100  avgt    5       57.435 ±      2.224  ns/op
IPv4AddressBenchmark.parseLongUsingSubnetUtils                   1000  avgt    5      566.308 ±     31.257  ns/op


IPv4AddressBenchmark.stringContainsUsingIpAddr                     10  avgt    5    10829.294 ±    929.158  ns/op
IPv4AddressBenchmark.stringContainsUsingIpAddr                    100  avgt    5   105955.838 ±   2047.309  ns/op
IPv4AddressBenchmark.stringContainsUsingIpAddr                   1000  avgt    5  1070301.262 ±  95573.228  ns/op

IPv4AddressBenchmark.stringContainsUsingSubnetUtils                10  avgt    5    14650.519 ±    755.609  ns/op
IPv4AddressBenchmark.stringContainsUsingSubnetUtils               100  avgt    5   151821.759 ±  44261.354  ns/op
IPv4AddressBenchmark.stringContainsUsingSubnetUtils              1000  avgt    5  1508744.485 ±  61303.553  ns/op

IPv4AddressBenchmark.toLongUsingIpAddr                             10  avgt    5        9.470 ±      0.043  ns/op
IPv4AddressBenchmark.toLongUsingIpAddr                            100  avgt    5       62.052 ±      3.588  ns/op
IPv4AddressBenchmark.toLongUsingIpAddr                           1000  avgt    5     2042.493 ±     77.128  ns/op

IPv4AddressBenchmark.toLongUsingSubnetUtils                        10  avgt    5     2014.053 ±    151.281  ns/op
IPv4AddressBenchmark.toLongUsingSubnetUtils                       100  avgt    5    19694.955 ±   6740.819  ns/op
IPv4AddressBenchmark.toLongUsingSubnetUtils                      1000  avgt    5   207336.015 ±   9011.625  ns/op

IPv4AddressBenchmark.longContainsUsingIpAddr                       10  avgt    5     3959.837 ±     11.629  ns/op
IPv4AddressBenchmark.longContainsUsingIpAddr                      100  avgt    5    44261.551 ±    191.061  ns/op
IPv4AddressBenchmark.longContainsUsingIpAddr                     1000  avgt    5   441864.580 ±   1245.393  ns/op

IPv4AddressBenchmark.longContainsUsingSubnetUtils                  10  avgt    5     2104.853 ±     41.862  ns/op
IPv4AddressBenchmark.longContainsUsingSubnetUtils                 100  avgt    5    20934.168 ±    382.555  ns/op
IPv4AddressBenchmark.longContainsUsingSubnetUtils                1000  avgt    5   198465.549 ±   2022.392  ns/op

As we can see above, in most cases, the new ipaddress library does better in most cases. There might be more room for optimization, but looks good overall IMO.

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
added documentation for new or modified features or behaviors.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added or updated version, license, or notice information in licenses.yaml
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
added integration tests.
been tested in a test Druid cluster.

lgtm-com · 2021-08-25T19:37:02Z

This pull request introduces 1 alert and fixes 2 when merging 85064d2 into 2a658ac - view on LGTM.com

new alerts:

1 for User-controlled data in numeric cast

fixed alerts:

2 for User-controlled data in numeric cast

lgtm-com · 2021-08-25T22:50:38Z

This pull request introduces 1 alert and fixes 2 when merging bb576d5 into 2a658ac - view on LGTM.com

new alerts:

1 for User-controlled data in numeric cast

fixed alerts:

2 for User-controlled data in numeric cast

lgtm-com · 2021-08-26T01:48:33Z

This pull request introduces 1 alert and fixes 2 when merging 70fb9d8f6a61f66cbaed1e8124f2a14f0da9913f into 2a658ac - view on LGTM.com

new alerts:

1 for User-controlled data in numeric cast

fixed alerts:

2 for User-controlled data in numeric cast

lgtm-com · 2021-08-26T08:35:04Z

This pull request introduces 1 alert and fixes 2 when merging 09cb838 into 2a658ac - view on LGTM.com

new alerts:

1 for User-controlled data in numeric cast

fixed alerts:

2 for User-controlled data in numeric cast

abhishekrb19 · 2021-08-26T17:00:33Z

hi @jihoonson, @asdf2014, the build is failing in the stage 2 starting with these jobs:

32932.42 (Compile=openjdk8, Run=openjdk8) batch index integration test
32932.43 (Compile=openjdk8, Run=openjdk8) batch index integration test with Indexer
...

It appears that mysql is not installed/found.

/bin/sh: 1: /etc/init.d/mysql: not found

I see a few jobs as part of other pull requests fail due to the same reason as well. Is this a known issue? Thanks!

asdf2014 · 2021-08-30T03:39:06Z

Hi @abhishekrb19 , we're on it. Please refer to #11638.

lgtm-com · 2021-08-30T22:30:23Z

This pull request introduces 1 alert and fixes 2 when merging 68acd77 into a096888 - view on LGTM.com

new alerts:

1 for User-controlled data in numeric cast

fixed alerts:

2 for User-controlled data in numeric cast

abhishekrb19 · 2021-09-01T20:50:41Z

@asdf2014, @jihoonson, @clintropolis, this change-set is ready for review. Please take a look whenever you get a chance. Thanks!

clintropolis

very sorry for the delayed review!

this does seem like a nicer and more flexible libary 👍

Have you had a chance to measure if there is any performance difference after the changes in this PR? I did a quick look and didn't spot any obvious pre-existing benchmarks for the ipv4 functions, but there might be something in https://github.com/apache/druid/tree/master/benchmarks/src/test/java/org/apache/druid/benchmark to either modify or model a new benchmark from, to make sure there is no performance regression by making this change.

clintropolis · 2021-10-22T22:25:41Z

processing/src/main/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacro.java

    Expr arg = args.get(0);

    class IPv4AddressMatchExpr extends ExprMacroTable.BaseScalarUnivariateMacroFunctionExpr
    {
-      private final SubnetUtils.SubnetInfo subnetInfo;
+      private final IPAddressString subnetString;


since this will be checked every time we check a match, should we maybe store both IPAddressString and IPv4Address so we can do whichever contains check is more efficient? (assuming we split long and string handling to not both convert to IPv4Address).

IPAddressString containment checks are optimized to be efficient (in some cases more efficient than going straight to IPv4Address). So you could use IPv4Address or IPAddressString, but I'd avoid storing both, there's not much benefit to that.

Yes, I agree with @seancfoley, the containment checks are already efficient in most cases. So I'd will leave it as this.

clintropolis · 2021-10-22T22:26:29Z

processing/src/main/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacro.java

@@ -104,19 +105,21 @@ public ExprEval eval(final ObjectBinding bindings)

      private boolean isStringMatch(String stringValue)
      {
-        return IPv4AddressExprUtils.isValidAddress(stringValue) && subnetInfo.isInRange(stringValue);
+        IPv4Address iPv4Address = IPv4AddressExprUtils.parse(stringValue);


should this parse to an IPAddressString instead of an IPv4Address so it can be checked against the subnetString directly?

If you intend to support IPv6 here at some point in time, you'd want to use IPAddressString here (or possibly IPAddress, super class of IPv4Address and IPv6Address). But since this code here is currently IPv4-specific at this time, you could stick with IPv4Address.

Yeah, I think IPv6 would be a separate class on its own. As things stand now, IPv4Address works well. We can perhaps revisit this when we have the IPv6 functionality in place. :)

lgtm-com · 2022-04-12T22:27:15Z

This pull request introduces 1 alert and fixes 2 when merging f2b3069 into 2c79d28 - view on LGTM.com

new alerts:

1 for User-controlled data in numeric cast

fixed alerts:

2 for User-controlled data in numeric cast

abhishekrb19 · 2022-04-13T01:17:11Z

@clintropolis, sorry the delayed response. I've added a benchmark and the results are in the PR summary now. Please let me know when you've a chance to take a look. Thanks!

lgtm-com · 2022-04-13T02:28:22Z

This pull request introduces 1 alert and fixes 2 when merging 34d4769 into 2c79d28 - view on LGTM.com

new alerts:

1 for User-controlled data in numeric cast

fixed alerts:

2 for User-controlled data in numeric cast

lgtm-com · 2022-04-13T17:08:17Z

This pull request introduces 1 alert and fixes 2 when merging 4b97c16 into f24e9c6 - view on LGTM.com

new alerts:

1 for User-controlled data in numeric cast

fixed alerts:

2 for User-controlled data in numeric cast

abhishekrb19 · 2022-04-14T00:31:37Z

CI is failing at the indexing modules test job (https://app.travis-ci.com/github/apache/druid/jobs/567005675). The failure seems unrelated to my changes here in this PR and I see similar failures in other PRs.

clintropolis

👍 very sorry for the delay finishing review

Thanks for collecting the measurements, the numbers mostly look good, with the exception of ipv4_match against LONG typed inputs, but I think the positives of this library outweigh this performance impact, and it doesn't seem worth the trouble to use both libraries.

I imagine this match performance could be improved one way or another (maybe just do bitwise operations directly?) but I think its fine to do this as future work.

abhishekrb19 added 11 commits August 24, 2021 15:13

Add ipaddress library as dependency.

07278d5

IPv4 functions to use the inet.ipaddr package.

3ba48bf

Remove unused imports.

06be3ba

Add new function.

f89b931

Minor rename.

f16f6e7

Add more unit tests.

01fb164

IPv4 address expr utils unit tests and address options.

478ad2f

Adjust the IPv4Util functions.

2d16aa8

Move the UTs a bit around.

d577080

Javadoc comments.

871a516

Add license info for IPAddress.

85064d2

abhishekrb19 added 2 commits August 25, 2021 14:29

Fix groupId, artifact and version in license.yaml.

46e6635

Remove redundant subnet in messages - fixes UT.

bb576d5

Remove unused commons-net dependency for /processing project.

09cb838

abhishekrb19 force-pushed the ipnet_package branch from 70fb9d8 to 09cb838 Compare August 26, 2021 07:27

asdf2014 added the Area - Querying label Aug 26, 2021

Merge branch 'master' into ipnet_package

68acd77

ChihyunChang mentioned this pull request Sep 23, 2021

Support real query cancelling for web console #11738

Merged

1 task

clintropolis reviewed Oct 22, 2021

View reviewed changes

abhishekrb19 added 3 commits January 7, 2022 10:22

Make class and methods public so it can be accessed.

b03830a

Add initial version of benchmark

6bd4461

Add subnetutils package for benchmarks.

f54d275

abhishekrb19 added 4 commits January 7, 2022 14:52

Auto generate ip addresses.

bb0ac45

Add more v4 address representations in setup to avoid bias.

411f74d

Merge branch 'master' into ipnet_package

733cfd0

Use ThreadLocalRandom to avoid forbidden API usage.

f2b3069

abhishekrb19 added 2 commits April 12, 2022 15:32

Adjust IPv4AddressBenchmark to adhere to codestyle rules.

3e8c438

Update ipaddress library to latest 5.3.4

34d4769

Add ipaddress package dependency to benchmarks project.

4b97c16

clintropolis approved these changes May 12, 2022

View reviewed changes

clintropolis merged commit 9177515 into apache:master May 12, 2022

clintropolis mentioned this pull request May 19, 2022

fix regression with ipv4_match and prefixes #12542

Merged

3 tasks

abhishekagarwal87 added this to the 24.0.0 milestone Aug 26, 2022

techdocsmith mentioned this pull request Aug 26, 2022

[Draft] 24.0 Release notes #12825

Closed

abhishekagarwal87 mentioned this pull request Sep 8, 2022

Test issue [Please ignore] #13055

Closed

gl mentioned this pull request Aug 25, 2023

Add IPv6 SQL Functions #10817

Closed

abhishekrb19 deleted the ipnet_package branch October 31, 2023 22:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IPAddress java library as dependency and migrate IPv4 functions to use the new library. #11634

Add IPAddress java library as dependency and migrate IPv4 functions to use the new library. #11634

abhishekrb19 commented Aug 25, 2021 •

edited

Loading

lgtm-com bot commented Aug 25, 2021

lgtm-com bot commented Aug 25, 2021

lgtm-com bot commented Aug 26, 2021

lgtm-com bot commented Aug 26, 2021

abhishekrb19 commented Aug 26, 2021

asdf2014 commented Aug 30, 2021

lgtm-com bot commented Aug 30, 2021

abhishekrb19 commented Sep 1, 2021

clintropolis left a comment

clintropolis Oct 22, 2021 •

edited

Loading

seancfoley Nov 10, 2021 •

edited

Loading

abhishekrb19 Apr 13, 2022

clintropolis Oct 22, 2021

seancfoley Nov 10, 2021 •

edited

Loading

abhishekrb19 Apr 13, 2022

lgtm-com bot commented Apr 12, 2022

abhishekrb19 commented Apr 13, 2022

lgtm-com bot commented Apr 13, 2022

lgtm-com bot commented Apr 13, 2022

abhishekrb19 commented Apr 14, 2022

clintropolis left a comment

Add IPAddress java library as dependency and migrate IPv4 functions to use the new library. #11634

Add IPAddress java library as dependency and migrate IPv4 functions to use the new library. #11634

Conversation

abhishekrb19 commented Aug 25, 2021 • edited Loading

lgtm-com bot commented Aug 25, 2021

lgtm-com bot commented Aug 25, 2021

lgtm-com bot commented Aug 26, 2021

lgtm-com bot commented Aug 26, 2021

abhishekrb19 commented Aug 26, 2021

asdf2014 commented Aug 30, 2021

lgtm-com bot commented Aug 30, 2021

abhishekrb19 commented Sep 1, 2021

clintropolis left a comment

Choose a reason for hiding this comment

clintropolis Oct 22, 2021 • edited Loading

Choose a reason for hiding this comment

seancfoley Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

abhishekrb19 Apr 13, 2022

Choose a reason for hiding this comment

clintropolis Oct 22, 2021

Choose a reason for hiding this comment

seancfoley Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

abhishekrb19 Apr 13, 2022

Choose a reason for hiding this comment

lgtm-com bot commented Apr 12, 2022

abhishekrb19 commented Apr 13, 2022

lgtm-com bot commented Apr 13, 2022

lgtm-com bot commented Apr 13, 2022

abhishekrb19 commented Apr 14, 2022

clintropolis left a comment

Choose a reason for hiding this comment

abhishekrb19 commented Aug 25, 2021 •

edited

Loading

clintropolis Oct 22, 2021 •

edited

Loading

seancfoley Nov 10, 2021 •

edited

Loading

seancfoley Nov 10, 2021 •

edited

Loading