Re: [PR] [multistage]partition assignment refactor [pinot]

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

walterddr commented on code in PR #12079:
URL: https://github.com/apache/pinot/pull/12079#discussion_r1414670200


##########
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/WorkerManager.java:
##########
@@ -58,15 +58,22 @@
 public class WorkerManager {
   private static final Logger LOGGER = LoggerFactory.getLogger(WorkerManager.class);
   private static final Random RANDOM = new Random();
+  private static final String DEFAULT_PARTITION_FUNCTION = "Hashcode";

Review Comment:
   not sure if this is the best way to go. i am also ok with having to ask this pass in as mandatory



##########
pinot-query-runtime/src/test/resources/queries/QueryHints.json:
##########
@@ -83,6 +83,8 @@
       },
       {
         "description": "Colocated JOIN with partition column with partition parallelism in first table",
+        "ignored": true,
+        "comment": "partition parallelism mismatched in hint, this query shouldn't work at all",

Review Comment:
   previously we decided to allow this but i guess we should not. putting an ignore here first but i think we should not allow this and should throw exception



##########
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/WorkerManager.java:
##########
@@ -333,26 +371,11 @@ private void assignWorkersToIntermediateFragment(PlanFragment fragment, Dispatch
       throw new IllegalStateException(
           "No server instance found for intermediate stage for tables: " + Arrays.toString(tableNames.toArray()));
     }
-    if (metadata.isRequiresSingletonInstance()) {
-      // require singleton should return a single global worker ID with 0;
-      metadata.setWorkerIdToServerInstanceMap(Collections.singletonMap(0,
-          new QueryServerInstance(serverInstances.get(RANDOM.nextInt(serverInstances.size())))));
-    } else {
-      Map<String, String> options = context.getPlannerContext().getOptions();
-      int stageParallelism = Integer.parseInt(options.getOrDefault(QueryOptionKey.STAGE_PARALLELISM, "1"));
-      Map<Integer, QueryServerInstance> workerIdToServerInstanceMap = new HashMap<>();
-      int workerId = 0;
-      for (ServerInstance serverInstance : serverInstances) {
-        QueryServerInstance queryServerInstance = new QueryServerInstance(serverInstance);
-        for (int i = 0; i < stageParallelism; i++) {
-          workerIdToServerInstanceMap.put(workerId++, queryServerInstance);
-        }
-      }
-      metadata.setWorkerIdToServerInstanceMap(workerIdToServerInstanceMap);
-    }
+    return serverInstances;
   }
 
-  private ColocatedTableInfo getColocatedTableInfo(String tableName, String partitionKey, int numPartitions) {
+  private ColocatedTableInfo getColocatedTableInfo(String tableName, String partitionKey, int numPartitions,

Review Comment:
   technically the class should be named "PartitionTableInfo". this doesn't indicate "co-locate" at all



##########
pinot-query-planner/src/test/resources/queries/PinotHintablePlans.json:
##########
@@ -1,6 +1,26 @@
 {
   "pinot_hint_option_tests": {
     "queries": [
+      {
+        "description": "hint table without partitioning should throw exception",
+        "sql": "EXPLAIN PLAN FOR SELECT * FROM d /*+ tableOptions(partition_key='col1', partition_size='4') */ LIMIT 10",
+        "expectedException": "Error composing query plan for:.*"

Review Comment:
   TODO: fix this test, it should always check the nested reason b/c that's always wrapped in parser throw or planner throw. doesn't make sense to check just the top level msg



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