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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
