[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #6942: Detect invalid column names from SQL query in BrokerRequestHandler

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

jackjlli commented on a change in pull request #6942:
URL: https://github.com/apache/incubator-pinot/pull/6942#discussion_r636348514



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -1682,6 +1685,74 @@ static void validateRequest(PinotQuery pinotQuery, int queryResponseLimit) {
     if (!queryOptions.isGroupByModeSQL() || !queryOptions.isResponseFormatSQL()) {
       throw new IllegalStateException("SQL query should always have response format and group-by mode set to SQL");
     }
+
+    // Validate column names from query
+    Set<String> columnsFromQuery = getColumnsFromQuery(pinotQuery);
+    if (!columnNamesFromSchema.containsAll(columnsFromQuery)) {
+      brokerMetrics.addMeteredTableValue(rawTableName, BrokerMeter.INVALID_COLUMN_NAMES_IN_QUERY, 1L);
+    }
+  }
+
+  /**
+   * Fetch column names from the SQL query.
+   * @param pinotQuery pinot query
+   */
+  private static Set<String> getColumnsFromQuery(PinotQuery pinotQuery) {

Review comment:
       This is a good point. I do have a PR that handles the column name validation in updateColumnNames (https://github.com/apache/incubator-pinot/pull/6066), but that doesn't seem like the correct place to put this logic, since it should only be used to fix/update the column names. Plus, there are a few places in the same `handleSQLRequest` method that do the similar operations on broker, and the overhead is very small comparing to routing the query to the server side.  
   
   If we handle the column name validation on broker side, there is no need to route the query to servers. I've updated this PR in the latest push. Thus, `DataSchemaSegmentPruner` can be deprecated.




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

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