You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/06/10 16:32:59 UTC

[GitHub] [pinot] walterddr opened a new pull request, #8880: query option to use parser.jj extension instead of regex

walterddr opened a new pull request, #8880:
URL: https://github.com/apache/pinot/pull/8880

   Background
   ===
   Currently Query Options are parsed via regex match and replaced with empty string. This causes SQLi issue. 
   
   Solution
   ===
   - using parser.jj extension.
   - backward-incompatible change, OPTIONS now can only be supported outside of a SQL statement (e.g. beginning or end of SQL string)


-- 
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

[GitHub] [pinot] siddharthteotia commented on a diff in pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8880:
URL: https://github.com/apache/pinot/pull/8880#discussion_r894856714


##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -116,30 +101,51 @@ public static SqlNodeAndOptions compileToSqlNodeAndOptions(String sql)
     // Remove the terminating semicolon from the query
     sql = removeTerminatingSemicolon(sql);
 
-    // Extract OPTION statements from sql as Calcite Parser doesn't parse it.
-    List<String> options = extractOptionsFromSql(sql);
-    if (!options.isEmpty()) {
-      sql = removeOptionsFromSql(sql);
-    }
-
     try (StringReader inStream = new StringReader(sql)) {

Review Comment:
   Not related to this PR --
   
   Looks like this function was added when INSERT grammar extension was done and is being called from broker query endpoint in `PinotClientRequest.java`.
   
   It seems like for DQL, the caller will parse the SQL statement twice. Once at line 153 when it calls this method in the parser to retrieve `SqlNodeAndOptions` and then if the type is not DML, it sends the query to `requestHandler.handleRequest(sql)` which will call the parser (`compileToPinotQuery`) the statement will be parsed again.
   
   Not sure if this is expected. I just noticed it



-- 
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

[GitHub] [pinot] walterddr commented on a diff in pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #8880:
URL: https://github.com/apache/pinot/pull/8880#discussion_r894724547


##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -25,6 +25,7 @@
 import java.util.List;
 import java.util.concurrent.TimeUnit;
 import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;

Review Comment:
   ```suggestion
   import org.apache.calcite.sql.SqlNodeList;
   ```



-- 
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

[GitHub] [pinot] siddharthteotia commented on a diff in pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8880:
URL: https://github.com/apache/pinot/pull/8880#discussion_r894854049


##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -116,30 +101,51 @@ public static SqlNodeAndOptions compileToSqlNodeAndOptions(String sql)
     // Remove the terminating semicolon from the query
     sql = removeTerminatingSemicolon(sql);
 
-    // Extract OPTION statements from sql as Calcite Parser doesn't parse it.
-    List<String> options = extractOptionsFromSql(sql);
-    if (!options.isEmpty()) {
-      sql = removeOptionsFromSql(sql);
-    }
-
     try (StringReader inStream = new StringReader(sql)) {
       SqlParserImpl sqlParser = newSqlParser(inStream);
-      return new SqlNodeAndOptions(sqlParser.parseSqlStmtEof(), options);
+      SqlNodeList sqlNodeList = sqlParser.SqlStmtsEof();
+      // Extract OPTION statements from sql.
+      return extractSqlNodeAndOptions(sqlNodeList);
     } catch (Throwable e) {
       throw new SqlCompilationException("Caught exception while parsing query: " + sql, e);
     }
   }
 
-  public static PinotSqlType extractSqlType(SqlNode sqlNode) {
-    switch (sqlNode.getKind()) {
-      case OTHER_DDL:
-        if (sqlNode instanceof SqlInsertFromFile) {
-          return PinotSqlType.DML;
+  public static SqlNodeAndOptions extractSqlNodeAndOptions(SqlNodeList sqlNodeList) {
+    PinotSqlType sqlType = null;
+    SqlNode statementNode = null;
+    Map<String, String> options = new HashMap<>();
+    for (SqlNode sqlNode : sqlNodeList) {
+      if (sqlNode instanceof SqlInsertFromFile) {
+        // extract insert statement (execution statement)
+        if (sqlType == null) {
+          sqlType = PinotSqlType.DML;
+          statementNode = sqlNode;
+        } else {
+          throw new SqlCompilationException("SqlNode with statement already exist with type: " + sqlType);
         }
-        throw new SqlCompilationException("Unsupported SqlNode type - " + sqlNode.getKind());
-      default:
-        return PinotSqlType.DQL;
+      } else if (sqlNode instanceof SqlOptions) {
+        // extract options, these are non-execution statements
+        List<SqlNode> operandList = ((SqlOptions) sqlNode).getOperandList();
+        for (int i = 0; i < operandList.size(); i += 2) {

Review Comment:
   (nit) move `operandList.size()` before the loop



-- 
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

[GitHub] [pinot] Jackie-Jiang commented on pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #8880:
URL: https://github.com/apache/pinot/pull/8880#issuecomment-1156028397

   Actually after a second thought, I think it is okay to keep the current way without affecting current query options. The only query option value that is not number/boolean is `FORCE_HLC` which I believe no one is using. @siddharthteotia 
   
   There is a major issue not handled though. In `PinotClientRequest.constructSqlQueryOptions()`, the `groupByMode` and `responseFormat` value `SQL` cannot be parsed, and that can cause user not able to upgrade from 0.10.0. We need to quote the value if there is no easy way to support the current format


-- 
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

[GitHub] [pinot] Jackie-Jiang commented on pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #8880:
URL: https://github.com/apache/pinot/pull/8880#issuecomment-1156035325

   Just noticed that seems it requires an extra `;` to split the statements, then all the existing query option won't work... We should document this change in the PR description


-- 
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

[GitHub] [pinot] siddharthteotia commented on a diff in pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8880:
URL: https://github.com/apache/pinot/pull/8880#discussion_r894836874


##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/parser/SqlOptions.java:
##########
@@ -0,0 +1,64 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.sql.parsers.parser;
+
+import java.util.List;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.SqlSpecialOperator;
+import org.apache.calcite.sql.SqlWriter;
+import org.apache.calcite.sql.parser.SqlParserPos;
+
+
+/**
+ * Calcite extension for creating an INSERT sql node from a File object.

Review Comment:
   (nit) please add correct javadoc.



##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/parser/SqlOptions.java:
##########
@@ -0,0 +1,64 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.sql.parsers.parser;
+
+import java.util.List;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.SqlSpecialOperator;
+import org.apache.calcite.sql.SqlWriter;
+import org.apache.calcite.sql.parser.SqlParserPos;
+
+
+/**
+ * Calcite extension for creating an INSERT sql node from a File object.
+ *
+ * <p>Syntax: INSERT INTO [db_name.]table_name FROM [ FILE | ARCHIVE ] 'file_uri' [, [ FILE | ARCHIVE ] 'file_uri' ]
+ */

Review Comment:
   same here



-- 
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

[GitHub] [pinot] walterddr commented on a diff in pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #8880:
URL: https://github.com/apache/pinot/pull/8880#discussion_r895235472


##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -116,30 +101,51 @@ public static SqlNodeAndOptions compileToSqlNodeAndOptions(String sql)
     // Remove the terminating semicolon from the query
     sql = removeTerminatingSemicolon(sql);
 
-    // Extract OPTION statements from sql as Calcite Parser doesn't parse it.
-    List<String> options = extractOptionsFromSql(sql);
-    if (!options.isEmpty()) {
-      sql = removeOptionsFromSql(sql);
-    }
-
     try (StringReader inStream = new StringReader(sql)) {

Review Comment:
   actually the logic here is a bit more complex than simple reduce the compilation. I will create another PR to follow up. 



-- 
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

[GitHub] [pinot] walterddr commented on a diff in pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #8880:
URL: https://github.com/apache/pinot/pull/8880#discussion_r895030576


##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -698,22 +698,29 @@ public void testQueryOptions() {
     Assert.assertNull(pinotQuery.getQueryOptions());
 
     pinotQuery = CalciteSqlParser.compileToPinotQuery(
-        "select * from vegetables where name <> 'Brussels sprouts' OPTION (delicious=yes)");
+        "OPTION (delicious='yes'); select * from vegetables where name <> 'Brussels sprouts'");

Review Comment:
   oh yes absolutely. I will check



##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -698,22 +698,29 @@ public void testQueryOptions() {
     Assert.assertNull(pinotQuery.getQueryOptions());
 
     pinotQuery = CalciteSqlParser.compileToPinotQuery(
-        "select * from vegetables where name <> 'Brussels sprouts' OPTION (delicious=yes)");
+        "OPTION (delicious='yes'); select * from vegetables where name <> 'Brussels sprouts'");

Review Comment:
   oh yes absolutely. I will add to the test set



-- 
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

[GitHub] [pinot] siddharthteotia commented on a diff in pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8880:
URL: https://github.com/apache/pinot/pull/8880#discussion_r894860128


##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -698,22 +698,29 @@ public void testQueryOptions() {
     Assert.assertNull(pinotQuery.getQueryOptions());
 
     pinotQuery = CalciteSqlParser.compileToPinotQuery(
-        "select * from vegetables where name <> 'Brussels sprouts' OPTION (delicious=yes)");
+        "OPTION (delicious='yes'); select * from vegetables where name <> 'Brussels sprouts'");

Review Comment:
   Can we add tests using examples called out in https://blog.doyensec.com/2022/06/09/apache-pinot-sqli-rce.html#query-options. Since they now don't fit in the grammar, we can check if they fail ?



-- 
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

[GitHub] [pinot] siddharthteotia commented on a diff in pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8880:
URL: https://github.com/apache/pinot/pull/8880#discussion_r894859015


##########
pinot-common/src/main/codegen/includes/parserImpls.ftl:
##########
@@ -73,7 +73,74 @@ SqlInsertFromFile SqlInsertFromFile() :
     }
 }
 
-/* define the rest of the sql into SqlStmtList
+
+/**

Review Comment:
   Can we enforce this to be only at the end or beginning of the query ?



-- 
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

[GitHub] [pinot] walterddr commented on pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
walterddr commented on PR #8880:
URL: https://github.com/apache/pinot/pull/8880#issuecomment-1156546397

   > Actually after a second thought, I think it is okay to keep the current way without affecting current query options. The only query option value that is not number/boolean is `FORCE_HLC` which I believe no one is using. @siddharthteotia
   
   adding `OPTION(identifier = identifier)` is possible but the identifier cannot start with a number so we are still not backward-compatible. 
   
   > 
   > There is a major issue not handled though. In `PinotClientRequest.constructSqlQueryOptions()`, the `groupByMode` and `responseFormat` value `SQL` cannot be parsed, and that can cause user not able to upgrade from 0.10.0. We need to quote the value if there is no easy way to support the previous format
   
   This is a good catch, will fix; but also do we know if there's any other places that adds the OPTION regex to the query?
   
   > Just noticed that seems it requires an extra ; to split the statements, then all the existing query option won't work... We should document this change in the PR description
   
   Yes this is the standard SQL statement separator. however the `;` before the `<EOF>` token can be skipped thus we don't have any issue so far. I will document this. 
   
   


-- 
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

[GitHub] [pinot] walterddr commented on a diff in pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #8880:
URL: https://github.com/apache/pinot/pull/8880#discussion_r895030456


##########
pinot-common/src/main/codegen/includes/parserImpls.ftl:
##########
@@ -73,7 +73,74 @@ SqlInsertFromFile SqlInsertFromFile() :
     }
 }
 
-/* define the rest of the sql into SqlStmtList
+
+/**

Review Comment:
   yeah it is already enforces by this syntatic requirement
   ```
   StatementList:
       Statement [; Statement]*
   
   Statement:
       SELECT_STATEMENT | OPTION_STATEMENT | INSERT_STATEMENT
   ```
   
   because we only allow 1 executable statement inside one SQL string. the OPTION statement can only be at the front or back. do you think this syntatic structure is clear enough to be part of the parser.jj comment/doc? (I will add it to the section if so)



-- 
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

[GitHub] [pinot] codecov-commenter commented on pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8880:
URL: https://github.com/apache/pinot/pull/8880#issuecomment-1152836364

   # [Codecov](https://codecov.io/gh/apache/pinot/pull/8880?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#8880](https://codecov.io/gh/apache/pinot/pull/8880?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (90ff274) into [master](https://codecov.io/gh/apache/pinot/commit/ea564f0add86b4bb7b06b611ab4276934f35f061?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ea564f0) will **decrease** coverage by `3.41%`.
   > The diff coverage is `78.72%`.
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #8880      +/-   ##
   ============================================
   - Coverage     69.78%   66.37%   -3.42%     
   - Complexity     4672     4810     +138     
   ============================================
     Files          1803     1354     -449     
     Lines         93752    68396   -25356     
     Branches      13935    10677    -3258     
   ============================================
   - Hits          65426    45395   -20031     
   + Misses        23790    19769    -4021     
   + Partials       4536     3232    -1304     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `?` | |
   | integration2 | `?` | |
   | unittests1 | `66.37% <78.72%> (-0.18%)` | :arrow_down: |
   | unittests2 | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8880?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...rg/apache/pinot/sql/parsers/parser/SqlOptions.java](https://codecov.io/gh/apache/pinot/pull/8880/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9wYXJzZXIvU3FsT3B0aW9ucy5qYXZh) | `45.45% <45.45%> (ø)` | |
   | [...org/apache/pinot/sql/parsers/CalciteSqlParser.java](https://codecov.io/gh/apache/pinot/pull/8880/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9DYWxjaXRlU3FsUGFyc2VyLmphdmE=) | `86.72% <87.50%> (-1.24%)` | :arrow_down: |
   | [...rg/apache/pinot/sql/parsers/SqlNodeAndOptions.java](https://codecov.io/gh/apache/pinot/pull/8880/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9TcWxOb2RlQW5kT3B0aW9ucy5qYXZh) | `100.00% <100.00%> (ø)` | |
   | [...g/apache/pinot/sql/parsers/dml/InsertIntoFile.java](https://codecov.io/gh/apache/pinot/pull/8880/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9kbWwvSW5zZXJ0SW50b0ZpbGUuamF2YQ==) | `85.18% <100.00%> (ø)` | |
   | [...va/org/apache/pinot/core/routing/RoutingTable.java](https://codecov.io/gh/apache/pinot/pull/8880/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yb3V0aW5nL1JvdXRpbmdUYWJsZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...va/org/apache/pinot/common/config/NettyConfig.java](https://codecov.io/gh/apache/pinot/pull/8880/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL05ldHR5Q29uZmlnLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/8880/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...g/apache/pinot/common/metrics/ControllerMeter.java](https://codecov.io/gh/apache/pinot/pull/8880/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Db250cm9sbGVyTWV0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/8880/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../apache/pinot/common/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/pinot/pull/8880/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [721 more](https://codecov.io/gh/apache/pinot/pull/8880/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8880?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8880?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ea564f0...90ff274](https://codecov.io/gh/apache/pinot/pull/8880?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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

[GitHub] [pinot] Jackie-Jiang commented on pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #8880:
URL: https://github.com/apache/pinot/pull/8880#issuecomment-1156012774

   This is awesome! Should we consider also supporting identifier as value? That way the current way of putting options can work


-- 
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

[GitHub] [pinot] walterddr closed pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
walterddr closed pull request #8880: query option to use parser.jj extension instead of regex
URL: https://github.com/apache/pinot/pull/8880


-- 
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

[GitHub] [pinot] siddharthteotia commented on a diff in pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8880:
URL: https://github.com/apache/pinot/pull/8880#discussion_r894860941


##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -116,30 +101,51 @@ public static SqlNodeAndOptions compileToSqlNodeAndOptions(String sql)
     // Remove the terminating semicolon from the query
     sql = removeTerminatingSemicolon(sql);
 
-    // Extract OPTION statements from sql as Calcite Parser doesn't parse it.
-    List<String> options = extractOptionsFromSql(sql);
-    if (!options.isEmpty()) {
-      sql = removeOptionsFromSql(sql);
-    }
-
     try (StringReader inStream = new StringReader(sql)) {
       SqlParserImpl sqlParser = newSqlParser(inStream);
-      return new SqlNodeAndOptions(sqlParser.parseSqlStmtEof(), options);
+      SqlNodeList sqlNodeList = sqlParser.SqlStmtsEof();
+      // Extract OPTION statements from sql.
+      return extractSqlNodeAndOptions(sqlNodeList);
     } catch (Throwable e) {
       throw new SqlCompilationException("Caught exception while parsing query: " + sql, e);
     }
   }
 
-  public static PinotSqlType extractSqlType(SqlNode sqlNode) {
-    switch (sqlNode.getKind()) {
-      case OTHER_DDL:
-        if (sqlNode instanceof SqlInsertFromFile) {
-          return PinotSqlType.DML;
+  public static SqlNodeAndOptions extractSqlNodeAndOptions(SqlNodeList sqlNodeList) {
+    PinotSqlType sqlType = null;
+    SqlNode statementNode = null;
+    Map<String, String> options = new HashMap<>();
+    for (SqlNode sqlNode : sqlNodeList) {

Review Comment:
   Let's add one or two tests that verify that exception is thrown ?



-- 
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

[GitHub] [pinot] siddharthteotia commented on a diff in pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8880:
URL: https://github.com/apache/pinot/pull/8880#discussion_r894860128


##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java:
##########
@@ -698,22 +698,29 @@ public void testQueryOptions() {
     Assert.assertNull(pinotQuery.getQueryOptions());
 
     pinotQuery = CalciteSqlParser.compileToPinotQuery(
-        "select * from vegetables where name <> 'Brussels sprouts' OPTION (delicious=yes)");
+        "OPTION (delicious='yes'); select * from vegetables where name <> 'Brussels sprouts'");

Review Comment:
   Can we also add tests using examples called out in https://blog.doyensec.com/2022/06/09/apache-pinot-sqli-rce.html#query-options. Since they now don't fit in the grammar, we can check if they fail ?



-- 
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

[GitHub] [pinot] siddharthteotia commented on pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on PR #8880:
URL: https://github.com/apache/pinot/pull/8880#issuecomment-1152746796

   I will spend sometime sweeping the production logs to see how OPTION is being used in production (if at all) just to understand how option is being used and how it will break with this change. Will get back with the info by early next week. 


-- 
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

[GitHub] [pinot] walterddr commented on pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
walterddr commented on PR #8880:
URL: https://github.com/apache/pinot/pull/8880#issuecomment-1157786167

   closing this in favor of #8906 for a more systematic fix. 


-- 
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

[GitHub] [pinot] kishoreg commented on pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
kishoreg commented on PR #8880:
URL: https://github.com/apache/pinot/pull/8880#issuecomment-1156015909

   +1 thanks @walterddr for adding this


-- 
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

[GitHub] [pinot] walterddr commented on a diff in pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #8880:
URL: https://github.com/apache/pinot/pull/8880#discussion_r895030155


##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -116,30 +101,51 @@ public static SqlNodeAndOptions compileToSqlNodeAndOptions(String sql)
     // Remove the terminating semicolon from the query
     sql = removeTerminatingSemicolon(sql);
 
-    // Extract OPTION statements from sql as Calcite Parser doesn't parse it.
-    List<String> options = extractOptionsFromSql(sql);
-    if (!options.isEmpty()) {
-      sql = removeOptionsFromSql(sql);
-    }
-
     try (StringReader inStream = new StringReader(sql)) {

Review Comment:
   i dont think so but yeah I will take a look to address this as well



-- 
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

[GitHub] [pinot] siddharthteotia commented on a diff in pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8880:
URL: https://github.com/apache/pinot/pull/8880#discussion_r894856714


##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -116,30 +101,51 @@ public static SqlNodeAndOptions compileToSqlNodeAndOptions(String sql)
     // Remove the terminating semicolon from the query
     sql = removeTerminatingSemicolon(sql);
 
-    // Extract OPTION statements from sql as Calcite Parser doesn't parse it.
-    List<String> options = extractOptionsFromSql(sql);
-    if (!options.isEmpty()) {
-      sql = removeOptionsFromSql(sql);
-    }
-
     try (StringReader inStream = new StringReader(sql)) {

Review Comment:
   Not related to this PR --
   
   Looks like this function was added when INSERT grammar extension was done and is being called from broker query endpoint in `PinotClientRequest.java`.
   
   It seems like for DQL, the caller will parse the SQL statement twice. Once at line 153 when it calls this method in the parser to retrieve `SqlNodeAndOptions` and then if the type is not DML, it sends the query to `requestHandler.handleRequest(sql)` which will call the parser (`compileToPinotQuery(sql)`) the statement will be parsed again.
   
   Not sure if this is expected. I just noticed it



-- 
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

[GitHub] [pinot] siddharthteotia commented on a diff in pull request #8880: query option to use parser.jj extension instead of regex

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #8880:
URL: https://github.com/apache/pinot/pull/8880#discussion_r894856714


##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -116,30 +101,51 @@ public static SqlNodeAndOptions compileToSqlNodeAndOptions(String sql)
     // Remove the terminating semicolon from the query
     sql = removeTerminatingSemicolon(sql);
 
-    // Extract OPTION statements from sql as Calcite Parser doesn't parse it.
-    List<String> options = extractOptionsFromSql(sql);
-    if (!options.isEmpty()) {
-      sql = removeOptionsFromSql(sql);
-    }
-
     try (StringReader inStream = new StringReader(sql)) {

Review Comment:
   Not related to this PR --
   
   Looks like this function was added when INSERT grammar extension was done and is being called from broker query endpoint in `PinotClientRequest.java`.
   
   It seems like for DQL, the caller will parse the SQL statement twice. Once at line 153 when it calls this method in the parser to retrieve `SqlNodeAndOptions` and then if the type is not DML, it sends the query to `requestHandler.handleRequest(sql)` which will call the parser again (`compileToPinotQuery(sql)`) and the statement will be parsed again.
   
   Not sure if this is expected. I just noticed it



-- 
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