From e037672b3d995ae68f91d48fc31321c7e926502c Mon Sep 17 00:00:00 2001
From: Kenrick Yap <kenrick.yap@improving.com>
Date: Thu, 19 Dec 2024 10:08:02 -0800
Subject: [PATCH] address review comments

Signed-off-by: Kenrick Yap <kenrick.yap@improving.com>
---
 docs/ppl-lang/planning/ppl-geoip-command.md      | 15 ---------------
 .../sql/ppl/parser/AstExpressionBuilder.java     | 16 +++++++++-------
 2 files changed, 9 insertions(+), 22 deletions(-)

diff --git a/docs/ppl-lang/planning/ppl-geoip-command.md b/docs/ppl-lang/planning/ppl-geoip-command.md
index 59ca2df08..aaed6c156 100644
--- a/docs/ppl-lang/planning/ppl-geoip-command.md
+++ b/docs/ppl-lang/planning/ppl-geoip-command.md
@@ -57,18 +57,3 @@ geoip function to add information about the geographical location of an IPv4 or
     - API data sources - if users have their own geoip provided will create ability for users to configure and call said endpoints
     - OpenSearch geospatial client - once geospatial client is published we can leverage client to utilize OpenSearch geo2ip functionality.
 - Additional datasource connection params will be provided through spark config options.
-
-### New syntax definition in ANTLR
-
-```ANTLR
-  
-// functions
-evalFunctionCall
-   : evalFunctionName LT_PRTHS functionArgs RT_PRTHS
-   | geoipFunction
-   ;  
-  
-geoipFunction
-   : GEOIP LT_PRTHS (datasource = functionArg COMMA)? ipAddress = functionArg (COMMA properties = stringLiteral)? RT_PRTHS
-   ;
-```
diff --git a/ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java b/ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java
index f376c23ae..4b2c90b71 100644
--- a/ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java
+++ b/ppl-spark-integration/src/main/java/org/opensearch/sql/ppl/parser/AstExpressionBuilder.java
@@ -457,13 +457,7 @@ public UnresolvedExpression visitGeoIpPropertyList(OpenSearchPPLParser.GeoIpProp
         if (ctx != null) {
             for (OpenSearchPPLParser.GeoIpPropertyContext property : ctx.geoIpProperty()) {
                 String propertyName = property.getText().toUpperCase();
-
-                try {
-                    GeoIpCatalystLogicalPlanTranslator.GeoIpProperty.valueOf(propertyName);
-                } catch (NullPointerException | IllegalArgumentException e) {
-                    throw new IllegalArgumentException("Invalid properties used.");
-                }
-
+                validateGeoIpProperty(propertyName);
                 properties.add(new Literal(propertyName, DataType.STRING));
             }
         }
@@ -471,6 +465,14 @@ public UnresolvedExpression visitGeoIpPropertyList(OpenSearchPPLParser.GeoIpProp
         return new AttributeList(properties.build());
     }
 
+    private static void validateGeoIpProperty(String propertyName) {
+        try {
+            GeoIpCatalystLogicalPlanTranslator.GeoIpProperty.valueOf(propertyName);
+        } catch (NullPointerException | IllegalArgumentException e) {
+            throw new IllegalArgumentException("Invalid properties used.");
+        }
+    }
+
     private List<UnresolvedExpression> timestampFunctionArguments(
             OpenSearchPPLParser.TimestampFunctionCallContext ctx) {
         List<UnresolvedExpression> args =