You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/05/19 06:20:02 UTC
[GitHub] [druid] clintropolis opened a new pull request, #12542: fix regression with ipv4_match and prefixes
clintropolis opened a new pull request, #12542:
URL: https://github.com/apache/druid/pull/12542
### Description
Follow up to #11634, this PR fixes a regression when matching some prefixes, which in cases like`ip_match('192.168.1.250', '192.168.1.251/31')` was now evaluating to false. The solution is to use `prefixContains` instead of `contains`. Added tests to ensure that this behaves as it did previously and this evaluates to true.
This PR has:
- [x] been self-reviewed.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [x] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #12542: fix regression with ipv4_match and prefixes
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #12542:
URL: https://github.com/apache/druid/pull/12542#discussion_r882459179
##########
processing/src/main/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacro.java:
##########
@@ -106,13 +106,13 @@ public ExprEval eval(final ObjectBinding bindings)
private boolean isStringMatch(String stringValue)
{
IPv4Address iPv4Address = IPv4AddressExprUtils.parse(stringValue);
- return iPv4Address != null && subnetString.contains(iPv4Address.toAddressString());
+ return iPv4Address != null && subnetString.prefixContains(iPv4Address.toAddressString());
Review Comment:
1: I think yes, but looking closer I redid some things in the latest commit.
2: oops, missed the benchmark, which is what prompted me to look a bit closer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on pull request #12542: fix regression with ipv4_match and prefixes
Posted by GitBox <gi...@apache.org>.
clintropolis commented on PR #12542:
URL: https://github.com/apache/druid/pull/12542#issuecomment-1138314192
I made some additional adjustments, and updated the benchmark to be more realistic and measured the difference (subnet is created outside of the loop, 100k inputs, etc)
```
Benchmark (numOfAddresses) (prefixRange) Mode Cnt Score Error Units
IPv4AddressBenchmark.longContainsUsingIpAddr 100000 16 avgt 5 9.868 ± 0.639 ms/op
IPv4AddressBenchmark.longContainsUsingIpAddr 100000 31 avgt 5 9.502 ± 0.827 ms/op
IPv4AddressBenchmark.longContainsUsingIpAddrWithStrings 100000 16 avgt 5 40.665 ± 8.771 ms/op
IPv4AddressBenchmark.longContainsUsingIpAddrWithStrings 100000 31 avgt 5 39.876 ± 5.530 ms/op
IPv4AddressBenchmark.longContainsUsingSubnetUtils 100000 16 avgt 5 1.953 ± 0.097 ms/op
IPv4AddressBenchmark.longContainsUsingSubnetUtils 100000 31 avgt 5 2.050 ± 0.149 ms/op
IPv4AddressBenchmark.parseLongUsingIpAddr 100000 16 avgt 5 8.125 ± 0.367 ms/op
IPv4AddressBenchmark.parseLongUsingIpAddr 100000 31 avgt 5 8.143 ± 0.582 ms/op
IPv4AddressBenchmark.parseLongUsingSubnetUtils 100000 16 avgt 5 1.146 ± 0.213 ms/op
IPv4AddressBenchmark.parseLongUsingSubnetUtils 100000 31 avgt 5 1.147 ± 0.152 ms/op
IPv4AddressBenchmark.parseStringUsingIpAddr 100000 16 avgt 5 37.648 ± 2.460 ms/op
IPv4AddressBenchmark.parseStringUsingIpAddr 100000 31 avgt 5 37.580 ± 2.368 ms/op
IPv4AddressBenchmark.parseStringUsingIpAddrString 100000 16 avgt 5 23.114 ± 2.343 ms/op
IPv4AddressBenchmark.parseStringUsingIpAddrString 100000 31 avgt 5 22.552 ± 0.700 ms/op
IPv4AddressBenchmark.parseStringUsingSubnetUtils 100000 16 avgt 5 61.369 ± 5.106 ms/op
IPv4AddressBenchmark.parseStringUsingSubnetUtils 100000 31 avgt 5 62.713 ± 4.682 ms/op
IPv4AddressBenchmark.stringContainsUsingIpAddrPrefixContains 100000 16 avgt 5 38.874 ± 2.810 ms/op
IPv4AddressBenchmark.stringContainsUsingIpAddrPrefixContains 100000 31 avgt 5 39.228 ± 1.715 ms/op
IPv4AddressBenchmark.stringContainsUsingIpAddrPrefixContainsTooManyRoundTrips 100000 16 avgt 5 39.179 ± 1.294 ms/op
IPv4AddressBenchmark.stringContainsUsingIpAddrPrefixContainsTooManyRoundTrips 100000 31 avgt 5 40.146 ± 0.725 ms/op
IPv4AddressBenchmark.stringContainsUsingSubnetUtils 100000 16 avgt 5 74.197 ± 14.850 ms/op
IPv4AddressBenchmark.stringContainsUsingSubnetUtils 100000 31 avgt 5 71.563 ± 1.444 ms/op
IPv4AddressBenchmark.toLongUsingIpAddr 100000 16 avgt 5 5.338 ± 0.886 ms/op
IPv4AddressBenchmark.toLongUsingIpAddr 100000 31 avgt 5 5.035 ± 0.462 ms/op
IPv4AddressBenchmark.toLongUsingSubnetUtils 100000 16 avgt 5 7.535 ± 0.769 ms/op
IPv4AddressBenchmark.toLongUsingSubnetUtils 100000 31 avgt 5 7.298 ± 0.552 ms/op
```
The main improvement is with contains on the `LONG` typed inputs, which now no longer need converted to strings to do the match.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] rohangarg commented on a diff in pull request #12542: fix regression with ipv4_match and prefixes
Posted by GitBox <gi...@apache.org>.
rohangarg commented on code in PR #12542:
URL: https://github.com/apache/druid/pull/12542#discussion_r876694097
##########
processing/src/main/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacro.java:
##########
@@ -106,13 +106,13 @@ public ExprEval eval(final ObjectBinding bindings)
private boolean isStringMatch(String stringValue)
{
IPv4Address iPv4Address = IPv4AddressExprUtils.parse(stringValue);
- return iPv4Address != null && subnetString.contains(iPv4Address.toAddressString());
+ return iPv4Address != null && subnetString.prefixContains(iPv4Address.toAddressString());
Review Comment:
1. followed this doc : https://github.com/seancfoley/IPAddress/wiki/Code-Examples-2%3A-Subnet-Containment-and-Matching#comparing-just-the-prefix for the diff b/w contains and prefixContains in the lib - is that the right reference?
2. can also replace a couple of occurrences in `IPv4AddressBenchmark`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] rohangarg commented on a diff in pull request #12542: fix regression with ipv4_match and prefixes
Posted by GitBox <gi...@apache.org>.
rohangarg commented on code in PR #12542:
URL: https://github.com/apache/druid/pull/12542#discussion_r885408538
##########
processing/src/main/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacro.java:
##########
@@ -71,71 +73,75 @@ public Expr apply(final List<Expr> args)
throw new IAE(ExprUtils.createErrMsg(name(), "must have 2 arguments"));
}
- IPAddressString subnetInfo = getSubnetInfo(args);
- Expr arg = args.get(0);
+ try {
+ final Expr arg = args.get(0);
+ final IPAddressString blockString = getSubnetInfo(args);
+ final IPAddress block = blockString.toAddress().toPrefixBlock();
- class IPv4AddressMatchExpr extends ExprMacroTable.BaseScalarUnivariateMacroFunctionExpr
- {
- private final IPAddressString subnetString;
-
- private IPv4AddressMatchExpr(Expr arg, IPAddressString subnetString)
+ class IPv4AddressMatchExpr extends ExprMacroTable.BaseScalarUnivariateMacroFunctionExpr
{
- super(FN_NAME, arg);
- this.subnetString = subnetString;
- }
+ private IPv4AddressMatchExpr(Expr arg)
+ {
+ super(FN_NAME, arg);
+ }
- @Nonnull
- @Override
- public ExprEval eval(final ObjectBinding bindings)
- {
- ExprEval eval = arg.eval(bindings);
- boolean match;
- switch (eval.type().getType()) {
- case STRING:
- match = isStringMatch(eval.asString());
- break;
- case LONG:
- match = !eval.isNumericNull() && isLongMatch(eval.asLong());
- break;
- default:
- match = false;
+ @Nonnull
+ @Override
+ public ExprEval eval(final ObjectBinding bindings)
+ {
+ ExprEval eval = arg.eval(bindings);
+ boolean match;
+ switch (eval.type().getType()) {
+ case STRING:
+ match = isStringMatch(eval.asString());
+ break;
+ case LONG:
+ match = !eval.isNumericNull() && isLongMatch(eval.asLong());
+ break;
+ default:
+ match = false;
+ }
+ return ExprEval.ofLongBoolean(match);
}
- return ExprEval.ofLongBoolean(match);
- }
- private boolean isStringMatch(String stringValue)
- {
- IPv4Address iPv4Address = IPv4AddressExprUtils.parse(stringValue);
- return iPv4Address != null && subnetString.contains(iPv4Address.toAddressString());
- }
+ private boolean isStringMatch(String stringValue)
+ {
+ IPAddressString iPv4Address = IPv4AddressExprUtils.parseString(stringValue);
Review Comment:
s/iPv4Address/iPv4AddressString
##########
processing/src/main/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacro.java:
##########
@@ -71,71 +73,75 @@ public Expr apply(final List<Expr> args)
throw new IAE(ExprUtils.createErrMsg(name(), "must have 2 arguments"));
}
- IPAddressString subnetInfo = getSubnetInfo(args);
- Expr arg = args.get(0);
+ try {
+ final Expr arg = args.get(0);
+ final IPAddressString blockString = getSubnetInfo(args);
+ final IPAddress block = blockString.toAddress().toPrefixBlock();
Review Comment:
maybe a comment could be useful to tell about the different ways to match a CIDR prefix being used (one with prefixBlock address and one directly with blockString#prefixContains)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekrb19 commented on pull request #12542: fix regression with ipv4_match and prefixes
Posted by GitBox <gi...@apache.org>.
abhishekrb19 commented on PR #12542:
URL: https://github.com/apache/druid/pull/12542#issuecomment-1132087776
Good catch! Thanks for the fix @clintropolis.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis merged pull request #12542: fix regression with ipv4_match and prefixes
Posted by GitBox <gi...@apache.org>.
clintropolis merged PR #12542:
URL: https://github.com/apache/druid/pull/12542
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org