Add CIDR function to PPL (#3036) #3110

currantw · 2024-10-23T19:30:18Z

Description

Adds the cidr function to PPL, which returns whether an IP address is within an IP address range specified in CIDR notation.

Related Issues

Resolves #3036.

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
[N/A] API changes companion pull request created.
Commits are signed per the DCO using --signoff.
[?] Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

jduo

There should be an integration test as well.
Please check if there is an explain test needed and others such as AstBuiderTest, AstExpressionBuliderTest

core/src/main/java/org/opensearch/sql/utils/IPUtils.java

.../test/java/org/opensearch/sql/expression/operator/predicate/BinaryPredicateOperatorTest.java

currantw · 2024-10-29T04:22:30Z

@jduo Currently working on integration tests, which don't appear to yet support the IP address type in OpenSearch.

jduo

Generally a good practice is to mark local variables as final when possible.

core/src/main/java/org/opensearch/sql/expression/ip/IPFunction.java

core/src/test/java/org/opensearch/sql/expression/ip/IPFunctionTest.java

.../test/java/org/opensearch/sql/expression/operator/predicate/BinaryPredicateOperatorTest.java

integ-test/src/test/java/org/opensearch/sql/ppl/IPFunctionIT.java

core/src/main/java/org/opensearch/sql/expression/operator/predicate/UnaryPredicateOperator.java

core/build.gradle

core/src/main/java/org/opensearch/sql/expression/ip/IPFunction.java

- Update Lexer and Parser. - Add `DSL::cidr` and BuiltinFunctionName.CIDR`. - Add `BinaryPredicateOperator::cidr`. - Add `IPUtils` with stub implementation of `isAddressInRange`. Signed-off-by: currantw <[email protected]>

Signed-off-by: currantw <[email protected]>

…nge` implementation to return `false` rather than raising an exception. Signed-off-by: currantw <[email protected]>

doctest/test_docs.py

...earch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java

...h/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java

MaxKsyunz

Changing base type for OpenSearchIP from UNKNOWN to STRING has far reaching consequence and I think best reverted. At the very least needs a dedicated PR and explicit discussion.

See latest action run -- several integration tests are now failing.

Have you seen prometheus module? It defines prometheus-specific function that work on prometheus-specific types.

core/src/main/java/org/opensearch/sql/expression/operator/predicate/UnaryPredicateOperator.java

Signed-off-by: currantw <[email protected]>

…ort for IP address type). Signed-off-by: currantw <[email protected]>

currantw · 2024-11-22T18:05:59Z

integ-test/src/test/resources/indexDefinitions/weblogs_index_mapping.json

        "type": "ip"
      },
+      "host_string": {
+        "type": "keyword"
+      },
      "method": {
        "type": "text"


Split into to fields with IP address and string data types, respectively. Should facilitate testing both once we add support for IP address data type in #3145.

currantw · 2024-11-22T18:07:02Z

...search/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprIpValueTest.java

-  void equal() {
-    assertTrue(ipValue.equal(new OpenSearchExprIpValue("192.168.0.1")));
+  void testEqual() {
+    assertTrue(ipValue.equal(new OpenSearchExprIpValue(ipString)));
  }


Changes no longer necessary because chanegs to IP data type were reverted, but figured it wasn't necessary to "undo" any code cleanup work 🤷

MaxKsyunz

LGTM.

YANG-DB · 2024-11-27T18:07:38Z

ppl/src/main/antlr/OpenSearchPPLParser.g4

add CIDR to the can-be-Id commands list

Added conditionFunctionName (LIKE, ISNULL, ISNOTNULL, and CIDRMATCH) to the keywordsCanBeId group.

YANG-DB · 2024-11-27T18:16:30Z

core/src/main/java/org/opensearch/sql/expression/operator/predicate/UnaryPredicateOperator.java

nice - but next time lets separate general code refactory from a specific feature plz

docs/user/ppl/functions/ip.rst

YANG-DB

@currantw LGTM - only a few minor comments and we can merge !!
thanks

Signed-off-by: currantw <[email protected]>

acarbonetto · 2024-12-02T17:16:53Z

core/src/main/java/org/opensearch/sql/expression/DSL.java

@@ -563,6 +563,10 @@ public static FunctionExpression regexp(Expression... expressions) {
    return compile(FunctionProperties.None, BuiltinFunctionName.REGEXP, expressions);
  }

+  public static FunctionExpression cidrmatch(Expression... expressions) {


not sure why you added the match here:

Suggested change

public static FunctionExpression cidrmatch(Expression... expressions) {

public static FunctionExpression cidr(Expression... expressions) {

ok, to match with spark documentation: https://github.com/opensearch-project/opensearch-spark/blob/main/docs/ppl-lang/functions/ppl-ip.md

@acarbonetto good catch 👍

Yup, as mentioned, I used cidrmatch for consistency with Spark (even though I agree that cidr is a better name...)

core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionName.java

core/src/main/java/org/opensearch/sql/expression/ip/IPFunction.java

core/src/main/java/org/opensearch/sql/expression/function/BuiltinFunctionRepository.java

core/src/main/java/org/opensearch/sql/expression/ip/IPFunction.java

YANG-DB · 2024-12-02T17:57:24Z

@currantw I'll let u update the code according to @acarbonetto comments ...
ping me once its ready & I'll merge - thanks !!

Signed-off-by: currantw <[email protected]>

currantw requested review from ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, rupal-bq, mengweieric, vmmusings, Swiddis, penghuo, seankao-az, MaxKsyunz, Yury-Fridlyand, anirudha, forestmvey, acarbonetto, GumpacG, ykmr1224, LantaoJin and noCharger as code owners October 23, 2024 19:30

jduo suggested changes Oct 23, 2024

View reviewed changes

jduo suggested changes Oct 29, 2024

View reviewed changes

andy-k-improving reviewed Oct 29, 2024

View reviewed changes

core/src/main/java/org/opensearch/sql/expression/operator/predicate/UnaryPredicateOperator.java Show resolved Hide resolved

currantw commented Oct 31, 2024

View reviewed changes

core/build.gradle Show resolved Hide resolved

currantw commented Oct 31, 2024

View reviewed changes

core/src/main/java/org/opensearch/sql/expression/ip/IPFunction.java Outdated Show resolved Hide resolved

currantw added 3 commits October 31, 2024 10:11

Add stub implementation of cidr:

bd264a0

- Update Lexer and Parser. - Add `DSL::cidr` and BuiltinFunctionName.CIDR`. - Add `BinaryPredicateOperator::cidr`. - Add `IPUtils` with stub implementation of `isAddressInRange`. Signed-off-by: currantw <[email protected]>

Initial implementation of isAddressInRange method.

c2ecdf7

Signed-off-by: currantw <[email protected]>

Add BinaryPredicateOperator tests for cidr. Update `isAddressInRa…

bfcbbf5

…nge` implementation to return `false` rather than raising an exception. Signed-off-by: currantw <[email protected]>

MaxKsyunz requested a review from andy-k-improving November 14, 2024 17:53

jduo reviewed Nov 14, 2024

View reviewed changes

doctest/test_docs.py Outdated Show resolved Hide resolved

jduo reviewed Nov 14, 2024

View reviewed changes

...earch/src/main/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactory.java Outdated Show resolved Hide resolved

jduo reviewed Nov 14, 2024

View reviewed changes

...h/src/test/java/org/opensearch/sql/opensearch/data/value/OpenSearchExprValueFactoryTest.java Show resolved Hide resolved

MaxKsyunz requested changes Nov 14, 2024

View reviewed changes

core/src/main/java/org/opensearch/sql/expression/operator/predicate/UnaryPredicateOperator.java Outdated Show resolved Hide resolved

core/src/main/java/org/opensearch/sql/expression/operator/predicate/UnaryPredicateOperator.java Show resolved Hide resolved

currantw added 3 commits November 19, 2024 09:59

Address minor review comments.

8324c4c

Signed-off-by: currantw <[email protected]>

Revert changes to cast IP to string.

e758077

Signed-off-by: currantw <[email protected]>

Fix failing tests and add TODOs for opensearch-project#3145 (add supp…

677bdc8

…ort for IP address type). Signed-off-by: currantw <[email protected]>

currantw commented Nov 22, 2024

View reviewed changes

MaxKsyunz previously approved these changes Nov 26, 2024

View reviewed changes

YANG-DB reviewed Nov 27, 2024

View reviewed changes

docs/user/ppl/functions/ip.rst Show resolved Hide resolved

YANG-DB previously approved these changes Nov 27, 2024

View reviewed changes

Update ip.rst for consistency with Spark

43d05d6

Signed-off-by: currantw <[email protected]>

currantw dismissed stale reviews from YANG-DB and MaxKsyunz via 43d05d6 November 28, 2024 00:13

Update ip.rst for consistency with Spark

23219cb

Signed-off-by: currantw <[email protected]>

YANG-DB previously approved these changes Nov 28, 2024

View reviewed changes

acarbonetto previously approved these changes Dec 2, 2024

View reviewed changes

Rename "function" and "operator" utility classes to be plural.

d3be058

Signed-off-by: currantw <[email protected]>

currantw dismissed stale reviews from acarbonetto and YANG-DB via d3be058 December 2, 2024 20:34

currantw added 2 commits December 2, 2024 12:47

Minor documentation updates

c216763

Signed-off-by: currantw <[email protected]>

Apply spotless

1b608c6

Signed-off-by: currantw <[email protected]>

YANG-DB approved these changes Dec 2, 2024

View reviewed changes

acarbonetto approved these changes Dec 3, 2024

View reviewed changes

acarbonetto merged commit 6712526 into opensearch-project:main Dec 3, 2024
14 of 15 checks passed

acarbonetto deleted the opensearch-3036 branch December 3, 2024 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CIDR function to PPL (#3036) #3110

Add CIDR function to PPL (#3036) #3110

currantw commented Oct 23, 2024 •

edited by acarbonetto

Loading

jduo left a comment

currantw commented Oct 29, 2024

jduo left a comment

MaxKsyunz left a comment

currantw Nov 22, 2024

currantw Nov 22, 2024

MaxKsyunz left a comment

YANG-DB Nov 27, 2024

currantw Nov 28, 2024

YANG-DB Nov 27, 2024

YANG-DB left a comment

acarbonetto Dec 2, 2024

acarbonetto Dec 2, 2024

YANG-DB Dec 2, 2024

currantw Dec 2, 2024

YANG-DB commented Dec 2, 2024

	public static FunctionExpression cidrmatch(Expression... expressions) {
	public static FunctionExpression cidr(Expression... expressions) {

Add CIDR function to PPL (#3036) #3110

Add CIDR function to PPL (#3036) #3110

Conversation

currantw commented Oct 23, 2024 • edited by acarbonetto Loading

Description

Related Issues

Check List

jduo left a comment

Choose a reason for hiding this comment

currantw commented Oct 29, 2024

jduo left a comment

Choose a reason for hiding this comment

MaxKsyunz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxKsyunz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YANG-DB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YANG-DB commented Dec 2, 2024

currantw commented Oct 23, 2024 •

edited by acarbonetto

Loading