[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7894: #7714 ignore query options in commented out queries

[GitBox <gi...@apache.org>]

Jackie-Jiang commented on a change in pull request #7894:
URL: https://github.com/apache/pinot/pull/7894#discussion_r768261063



##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -95,7 +95,7 @@ private CalciteSqlParser() {
   //   `OPTION (<k1> = <v1>) OPTION (<k2> = <v2>) OPTION (<k3> = <v3>)`
   private static final Pattern OPTIONS_REGEX_PATTEN =
       Pattern.compile("option\\s*\\(([^\\)]+)\\)", Pattern.CASE_INSENSITIVE);
-
+  private static final Pattern COMMENTED_QUERY_PATTERN = Pattern.compile("-{2,}");

Review comment:
       Can use simply search for "--" instead of using regex match? Substring search is much cheaper comparing to regex match

##########
File path: pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
##########
@@ -122,6 +122,7 @@ public static PinotQuery compileToPinotQuery(String sql)
 
     // Extract OPTION statements from sql as Calcite Parser doesn't parse it.
     List<String> options = extractOptionsFromSql(sql);
+    sql = removeCommentedOptionsFromSql(sql);

Review comment:
       Why do we need to remove the commented options again here?
   
   I feel a simpler and more readable way to support this is to first remove the commented part from the sql, then extract the options




-- 
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@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org