[GitHub] [pinot] ankitsultana commented on a diff in pull request #10336: [Multistage] Allow queries on multiple tables of same tenant to be executed from controller UI

["ankitsultana (via GitHub)" <gi...@apache.org>]

ankitsultana commented on code in PR #10336:
URL: https://github.com/apache/pinot/pull/10336#discussion_r1118154098


##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -135,6 +135,24 @@ public static SqlNodeAndOptions compileToSqlNodeAndOptions(String sql)
     }
   }
 
+  public static List<String> extractTableNamesFromNode(SqlNode sqlNode) {
+    List<String> tableNames = new ArrayList<>();
+    if (sqlNode instanceof SqlSelect) {
+      SqlNode fromNode = ((SqlSelect) sqlNode).getFrom();
+      tableNames.addAll(((SqlIdentifier) fromNode).names);
+      tableNames.addAll(extractTableNamesFromNode(((SqlSelect) sqlNode).getWhere()));
+    } else if (sqlNode instanceof SqlBasicCall) {
+      for (SqlNode node: ((SqlBasicCall) sqlNode).getOperandList()) {
+        tableNames.addAll(extractTableNamesFromNode(node));
+      }
+    } else if (sqlNode instanceof SqlOrderBy) {

Review Comment:
   When can the else clause be hit?



##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -135,6 +135,24 @@ public static SqlNodeAndOptions compileToSqlNodeAndOptions(String sql)
     }
   }
 
+  public static List<String> extractTableNamesFromNode(SqlNode sqlNode) {

Review Comment:
   I think we need to add UTs for this.



##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -170,10 +171,11 @@ private String getMultiStageQueryResponse(String query, String queryOptions, Htt
       return QueryException.ACCESS_DENIED_ERROR.toString();
     }
 
-    // Get brokers, only DEFAULT tenant is supported for now.
-    // TODO: implement logic that only allows executing query where all accessed tables are within the same tenant.
-    List<String> instanceIds = new ArrayList<>(_pinotHelixResourceManager.getAllInstancesForBrokerTenant(
-        TagNameUtils.DEFAULT_TENANT_NAME));
+    ObjectNode requestJson = getRequestJson(query, "false", queryOptions);
+    SqlNodeAndOptions sqlNodeAndOptions = RequestUtils.parseQuery(query, requestJson);

Review Comment:
   I think we can add an overloaded method: `RequestsUtils.parseQuery(query)` that doesn't take in the requestJson and simply returns a `SqlNodeAndOptions`. The existing method can also use the same.



##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -226,6 +228,31 @@ private String getQueryResponse(String query, @Nullable SqlNode sqlNode, String
     return sendRequestToBroker(query, instanceId, traceEnabled, queryOptions, httpHeaders);
   }
 
+  // get list of common broker instances for a given set of tables
+  private List<String> getCommonInstances(List<String> tableNames) {
+    boolean firstTable = true;
+    Set<String> brokerInstanceList = new HashSet<>();

Review Comment:
   nit: remove `List` from name since this is a set



##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -226,6 +228,31 @@ private String getQueryResponse(String query, @Nullable SqlNode sqlNode, String
     return sendRequestToBroker(query, instanceId, traceEnabled, queryOptions, httpHeaders);
   }
 
+  // get list of common broker instances for a given set of tables
+  private List<String> getCommonInstances(List<String> tableNames) {
+    boolean firstTable = true;
+    Set<String> brokerInstanceList = new HashSet<>();
+    for (String tableName : tableNames) {
+      String rawTableName = TableNameBuilder.extractRawTableName(_pinotHelixResourceManager
+              .getActualTableName(tableName));
+      List<String> tableBrokerInstances;
+      try {
+        tableBrokerInstances = _pinotHelixResourceManager.getLiveBrokersForTable(rawTableName);

Review Comment:
   I think we shouldn't convert this to a raw table name and instead pass the tableName directly. If you use a rawTableName, the `getLiveBrokersForTable` method will return an intersection of the realtime and offline brokers for hybrid tables. If a user decides to use a type-suffix for a hybrid table, we can simply honor that and return live brokers for that type.
   
   I am not sure what the current behavior is for those tables though (would be best to be consistent with that).



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