PPL geoip function #871

kenrickyap · 2024-11-05T17:39:33Z

Description
PPL geoip function

Issues Resolved
#672

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.

This PR is a continuation of #781 due to lacking permissions to push to forked branch in said PR

kenrickyap · 2024-11-05T23:33:26Z

Hi @YANG-DB, I heard from Anas that you had a method of implementing ip2geo functionality for Spark. I wanted to check with you that our current approach aligns with your method.

Current Plan:
Leveraging SerializableUdf create a UDF that does the follow:

Check if in-memory cache object for datasource exists.
If cache object does not exists create new in-memory cache object from csv retrieved from datasource manifest. (manifest to CsvParser logic can be stripped from geospatial ip2geo).
Search cached object for GeoIP data.
Return GeoIP data.

This PR has a stub udf implementation for better idea of how this would be implemented.

All of this would have to be implemented within the Spark library, as currently I am not aware of how to access any geospatial artifacts. If you know of a better way to implement ip2geo please let me know! Thanks!

ppl-spark-integration/src/main/java/org/opensearch/sql/expression/function/SerializableUdf.java

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

@kenrickyap please update the DCO (contributor sign-off)

YANG-DB

@kenrickyap I'm missing a more detailed design of the actual functionality that should include:

flow diagram of loading the geoIp data
storage of that data
utilizing of different datasource
pro/cons for the different approaches

please add the former discussions made into this issue for better traceability

ppl-spark-integration/src/main/antlr4/OpenSearchPPLParser.g4

ppl-spark-integration/src/main/java/org/opensearch/sql/expression/function/SerializableUdf.java

YANG-DB · 2024-11-06T18:34:28Z

ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/CatalystExpressionVisitor.java

-import org.opensearch.sql.ast.expression.When;
-import org.opensearch.sql.ast.expression.WindowFunction;
-import org.opensearch.sql.ast.expression.Xor;
+import org.opensearch.sql.ast.expression.*;


please keep the existing explicit import list ...

kenrickyap · 2024-11-06T23:32:31Z

Flow diagram for ip2geo UDF

Design Details

Ip2GeoCache will be a Gauva Cache that has datasource string as key and CidrTree as value
CidrTree will be Trie (will use apache.common PatriciaTrie as I see that apache.common is already included in project)
- Am using trie instead of map as it is more well suited for longest prefix matching task, and this task is similar to ip to cidr matching
- each CidrTreeNode will store:
  - nth bit value of cidr (n is the depth of tree)
  - geo_data if there is matching cidr row in datasource csv
  - child CidrTreeNodes
Will retrieve CsvParser from manifest using similar methodology as in geospatial ip2geo

Pros

ip2geo functionality is achieved.
implementation is simple and does not depend on any additional libraries that do not already exist in the project.

Cons

calculations are done in-memory as a UDF, this means that multiple instances of Ip2GeoCache will be created in distributed Spark systems and they will not sync.
not leveraging job-scheduler to run ip2geo task and not leveraging OpenSearch to store ip2geo data as geospatial does.

YANG-DB · 2024-11-07T00:06:00Z

Flow diagram for ip2geo UDF

Design Details

Ip2GeoCache will be a Gauva Cache that has datasource string as key and CidrTree as value

CidrTree will be Trie (will use apache.common PatriciaTrie as I see that apache.common is already included in project)

Am using trie instead of map as it is more well suited for longest prefix matching task, and this task is similar to ip to cidr matching

each CidrTreeNode will store:

nth bit value of cidr (n is the depth of tree)

geo_data if there is matching cidr row in datasource csv

child CidrTreeNodes

Will retrieve CsvParser from manifest using similar methodology as in geospatial ip2geo

Pros

ip2geo functionality is achieved.

implementation is simple and does not depend on any additional libraries that do not already exist in the project.

Cons

calculations are done in-memory as a UDF, this means that multiple instances of Ip2GeoCache will be created in distributed Spark systems and they will not sync.

not leveraging job-scheduler to run ip2geo task and not leveraging OpenSearch to store ip2geo data as geospatial does.

@kenrickyap a few questions here:

how / is the Ip2GeoCache memory shared between sessions ? is it a signleton ?
plz also add the use case where the geo_data_source will be a table / index (in OpenSearch) / service (API) - lets create a general purpose facade for this to hide different datasource drivers
I would like to see a more detailed description of the CidrTreeNode including an example and simple high level explanations (the geo tree diagram ?)
explain why is it worth while to add this trie-tree instead of using a hash-map / search ?
can u also add a pseudo-code here (in the issue) to clarity the composition ?

kenrickyap · 2024-11-08T16:23:43Z

@kenrickyap a few questions here:

how / is the Ip2GeoCache memory shared between sessions ? is it a signleton ?

Yes we will create Ip2GeoCache to be a singleton. From my understanding this should allow the cache to be accessible between sessions

plz also add the use case where the geo_data_source will be a table / index (in OpenSearch) / service (API) - lets create a general purpose facade for this to hide different datasource drivers

Will update the flow diagram to reflect facade for different datasource.

However, would it be possible to provide a service (API) example usage? I am not to sure what such a datasource would be expected to return.

Also would there be a fixed schema for an index in opensource?

I would like to see a more detailed description of the CidrTreeNode including an example and simple high level explanations (the geo tree diagram ?)

In hindsight I will just use a hashmap to store cidr geo_data where the cidr bitstring will be the key and geo_data will be a value. To preform the ip cidr matching will implement lookup function to convert ip to bitstring and reduce bit length till key is found in map.

Initially I wanted to leverage the prefixMap function as I thought this would find the best fitting cidr mask for a given ip in O(1), which would mean I wouldn't have to implement my own lookup function. However as I was trying implement this noticed that prefixMap takes a prefix and finds all keys that have the prefix, which is the opposite with what I want.

explain why is it worth while to add this trie-tree instead of using a hash-map / search ?

As mentioned above will use a hash-map instead of trie.

can u also add a pseudo-code here (in the issue) to clarity the composition ?

Will have pseudo-code and code added by EOD

normanj-bitquill · 2024-11-08T17:59:54Z

docs/ppl-lang/planning/ppl-geoip.md

+
+geoip function to add information about the geographical location of an IPv4 or IPv6 address
+
+1. **Proposed syntax**


Perhaps also list out the values that can be used for properties.

we need a larger set of examples here plz

added additional examples in ppl_ip.md doc

normanj-bitquill · 2024-11-08T18:08:06Z

ppl-spark-integration/src/main/antlr4/OpenSearchPPLParser.g4

@@ -894,6 +895,10 @@ coalesceFunctionName
   : COALESCE
   ;

+geoIpFunctionCall
+   : GEOIP LT_PRTHS (datasource = functionArg COMMA)? ipAddress = functionArg (COMMA properties = stringLiteral)? RT_PRTHS


You could do further parsing here of properties. Something like:

... (COMMA properties = geoipPropertiesList)? ... geoipPropertiesList : geoipProperty (COMMA geoipProperty)* geoipProperty : city | lat | lon

I like this idea !!

normanj-bitquill · 2024-11-08T18:08:27Z

ppl-spark-integration/src/main/java/org/opensearch/sql/ast/AbstractNodeVisitor.java

-import org.opensearch.sql.ast.expression.Field;
-import org.opensearch.sql.ast.expression.FieldList;
-import org.opensearch.sql.ast.expression.LambdaFunction;
+import org.opensearch.sql.ast.expression.*;


Avoid using * imports.

+1. please config your IDE to disable the auto-merging.

...on/src/test/scala/org/opensearch/flint/spark/ppl/PPLLogicalPlanEvalTranslatorTestSuite.scala

YANG-DB

@kenrickyap can u plz update the PR status and progress ?
thanks

ppl-spark-integration/src/main/java/org/opensearch/sql/common/geospatial/CidrGeoMap.java

kenrickyap · 2024-11-12T23:01:56Z

@kenrickyap can u plz update the PR status and progress ? thanks

@YANG-DB we are nearly done implementation, am currently testing implementation of geoip functionality on TestDatasourceDao that provides a mock stream of geo data.

@jduo is implementing the manifest dao, in parallel

Also would it be ok to move API and OpenSearch index DAO implementation to another ticket? We are not sure what the specifications for this would be as there are no examples, and am not sure this would be done by Nov 15th.

YANG-DB · 2024-11-13T05:05:03Z

@kenrickyap plz fix the DCO error ...

YANG-DB · 2024-11-13T05:05:51Z

docs/ppl-lang/planning/ppl-geoip.md

+
+geoip function to add information about the geographical location of an IPv4 or IPv6 address
+
+1. **Proposed syntax**


we need a larger set of examples here plz

ppl-spark-integration/src/main/java/org/opensearch/sql/common/geospatial/CidrGeoMap.java

ppl-spark-integration/src/main/java/org/opensearch/sql/common/geospatial/DatasourceDao.java

ppl-spark-integration/src/main/java/org/opensearch/sql/common/geospatial/TestDatasourceDao.java

kenrickyap · 2024-12-17T19:04:23Z

@LantaoJin would you be able to provide a review?

Signed-off-by: Kenrick Yap <[email protected]>

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

docs/ppl-lang/functions/ppl-ip.md

docs/ppl-lang/planning/ppl-geoip-command.md

integ-test/src/integration/scala/org/opensearch/flint/spark/ppl/FlintSparkPPLGeoipITSuite.scala

ppl-spark-integration/src/main/antlr4/OpenSearchPPLLexer.g4

andy-k-improving · 2024-12-18T02:13:57Z

@kenrickyap I only conducted the review for half of the changes, will continue tomorrow, also checkstyle is failing on CI, would you mind to fix?
Thanks,

YANG-DB · 2024-12-18T02:47:22Z

@kenrickyap can u see why the build is failing ?

Signed-off-by: Kenrick Yap <[email protected]>

kenrickyap · 2024-12-18T18:35:16Z

@kenrickyap can u see why the build is failing ?

fixed the styling issues

...st/scala/org/opensearch/flint/spark/ppl/PPLLogicalPlanGeoipFunctionTranslatorTestSuite.scala

...tegration/src/main/java/org/opensearch/sql/ppl/utils/GeoIpCatalystLogicalPlanTranslator.java

Signed-off-by: Kenrick Yap <[email protected]>

docs/ppl-lang/planning/ppl-geoip-command.md

ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/CatalystQueryPlanVisitor.java

ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java

YANG-DB

@kenrickyap I've added a few comments
plz review - thanks

Signed-off-by: Kenrick Yap <[email protected]>

kenrickyap · 2024-12-19T18:09:48Z

@kenrickyap I've added a few comments plz review - thanks

@YANG-DB have addressed new PR comments, not sure if I fully understand some of the comments, have added my understanding of unaddressed comments

Signed-off-by: Kenrick Yap <[email protected]>

YANG-DB

OK LGTM !

andy-k-improving · 2024-12-19T20:09:04Z

Thx for taking the time to address comments. :)

kenrickyap requested review from dai-chen, rupal-bq, mengweieric, vmmusings, penghuo, seankao-az, anirudha, kaituo, YANG-DB, noCharger, LantaoJin and ykmr1224 as code owners November 5, 2024 17:39

YANG-DB added Lang:PPL Pipe Processing Language support 0.6 labels Nov 5, 2024

kenrickyap marked this pull request as draft November 5, 2024 21:59

kenrickyap commented Nov 5, 2024

View reviewed changes

ppl-spark-integration/src/main/java/org/opensearch/sql/expression/function/SerializableUdf.java Outdated Show resolved Hide resolved

YANG-DB requested changes Nov 6, 2024

View reviewed changes

normanj-bitquill reviewed Nov 8, 2024

View reviewed changes

...on/src/test/scala/org/opensearch/flint/spark/ppl/PPLLogicalPlanEvalTranslatorTestSuite.scala Outdated Show resolved Hide resolved

YANG-DB requested changes Nov 12, 2024

View reviewed changes

ppl-spark-integration/src/main/java/org/opensearch/sql/common/geospatial/CidrGeoMap.java Outdated Show resolved Hide resolved

YANG-DB requested changes Nov 13, 2024

View reviewed changes

kenrickyap changed the title ~~[WIP] PPL geoip function pt. 2~~ PPL geoip function pt. 2 Dec 11, 2024

kenrickyap and others added 2 commits December 18, 2024 00:17

Fixed integration tests

daf11fa

Signed-off-by: Kenrick Yap <[email protected]>

Merge branch 'main' into geoip

9a7dace

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

andy-k-improving suggested changes Dec 18, 2024

View reviewed changes

linting

543740d

Signed-off-by: Kenrick Yap <[email protected]>

YANG-DB requested changes Dec 18, 2024

View reviewed changes

...st/scala/org/opensearch/flint/spark/ppl/PPLLogicalPlanGeoipFunctionTranslatorTestSuite.scala Outdated Show resolved Hide resolved

...st/scala/org/opensearch/flint/spark/ppl/PPLLogicalPlanGeoipFunctionTranslatorTestSuite.scala Show resolved Hide resolved

andy-k-improving reviewed Dec 18, 2024

View reviewed changes

...tegration/src/main/java/org/opensearch/sql/ppl/utils/GeoIpCatalystLogicalPlanTranslator.java Show resolved Hide resolved

andy-k-improving reviewed Dec 18, 2024

View reviewed changes

...tegration/src/main/java/org/opensearch/sql/ppl/utils/GeoIpCatalystLogicalPlanTranslator.java Outdated Show resolved Hide resolved

andy-k-improving suggested changes Dec 18, 2024

View reviewed changes

14yapkc1 and others added 3 commits December 19, 2024 00:35

addressing PR comments (added addtional integ tests, doc changes)

91ac8e5

Signed-off-by: Kenrick Yap <[email protected]>

fixed new integ tests

89cdb4b

Signed-off-by: Kenrick Yap <[email protected]>

addressing pr comments

26ab5b0

Signed-off-by: Kenrick Yap <[email protected]>