kfaraz commented on code in PR #16358:
URL: https://github.com/apache/druid/pull/16358#discussion_r1590547340
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQWorkerTask.java:
##########
@@ -185,4 +189,22 @@ public int hashCode()
{
return Objects.hash(super.hashCode(), controllerTaskId, workerNumber,
retryCount, worker);
}
+
+ @Override
+ public LookupLoadingSpec getLookupLoadingSpec()
+ {
+ if (!getContext().containsKey(PlannerContext.CTX_LOOKUP_LOADING_MODE) ||
getContext().get(PlannerContext.CTX_LOOKUP_LOADING_MODE) == null) {
+ return LookupLoadingSpec.ALL;
+ }
+
+ String lookupLoadingMode =
getContext().get(PlannerContext.CTX_LOOKUP_LOADING_MODE).toString();
Review Comment:
Simplified a bit:
```suggestion
final Object lookupModeValue =
getContext().get(PlannerContext.CTX_LOOKUP_LOADING_MODE);
if (lookupModeValue == null) {
return LookupLoadingSpec.ALL;
}
final LookupLoadingMode lookupLoadingMode =
LookupLoadingMode.valueOf(lookupModeValue.toString());
```
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskQueryMaker.java:
##########
@@ -281,6 +282,16 @@ public QueryResponse<Object[]> runQuery(final DruidQuery
druidQuery)
.tuningConfig(new MSQTuningConfig(maxNumWorkers,
maxRowsInMemory, rowsPerSegment, indexSpec))
.build();
+ Map<String, Object> context = new HashMap<>();
Review Comment:
```suggestion
final Map<String, Object> context = new HashMap<>();
```
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQWorkerTask.java:
##########
@@ -185,4 +189,22 @@ public int hashCode()
{
return Objects.hash(super.hashCode(), controllerTaskId, workerNumber,
retryCount, worker);
}
+
+ @Override
+ public LookupLoadingSpec getLookupLoadingSpec()
+ {
+ if (!getContext().containsKey(PlannerContext.CTX_LOOKUP_LOADING_MODE) ||
getContext().get(PlannerContext.CTX_LOOKUP_LOADING_MODE) == null) {
+ return LookupLoadingSpec.ALL;
+ }
+
+ String lookupLoadingMode =
getContext().get(PlannerContext.CTX_LOOKUP_LOADING_MODE).toString();
+ if (lookupLoadingMode.equals(LookupLoadingSpec.Mode.NONE.toString())) {
+ return LookupLoadingSpec.NONE;
+ } else if
(lookupLoadingMode.equals(LookupLoadingSpec.Mode.ONLY_REQUIRED.toString())) {
+ List<String> lookupsToLoad = (List<String>)
getContext().get(PlannerContext.CTX_LOOKUPS_TO_LOAD);
Review Comment:
There should be some validation here to check if `lookupsToLoad` is null or
empty.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java:
##########
@@ -341,6 +346,14 @@ public String getSchemaResourceType(String schema, String
resourceName)
return plannerToolbox.rootSchema().getResourceType(schema, resourceName);
}
+ /**
+ * Returns the set of lookups to load for a given task.
+ */
+ public Set<String> getLookupsToLoad()
Review Comment:
Shouldn't this also use `LookupLoadingSpec`?
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQControllerTask.java:
##########
@@ -138,7 +139,6 @@ public MSQControllerTask(
this.sqlResultsContext = sqlResultsContext;
this.sqlTypeNames = sqlTypeNames;
this.nativeTypeNames = nativeTypeNames;
-
Review Comment:
Super Nit: why remove this? It offered a clean visual separation of the
logic.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQWorkerTask.java:
##########
@@ -185,4 +189,22 @@ public int hashCode()
{
return Objects.hash(super.hashCode(), controllerTaskId, workerNumber,
retryCount, worker);
}
+
+ @Override
+ public LookupLoadingSpec getLookupLoadingSpec()
+ {
+ if (!getContext().containsKey(PlannerContext.CTX_LOOKUP_LOADING_MODE) ||
getContext().get(PlannerContext.CTX_LOOKUP_LOADING_MODE) == null) {
+ return LookupLoadingSpec.ALL;
+ }
+
+ String lookupLoadingMode =
getContext().get(PlannerContext.CTX_LOOKUP_LOADING_MODE).toString();
Review Comment:
Simplified a bit:
```suggestion
final Object lookupModeValue =
getContext().get(PlannerContext.CTX_LOOKUP_LOADING_MODE);
if (lookupModeValue == null) {
return LookupLoadingSpec.ALL;
}
final LookupLoadingMode lookupLoadingMode =
LookupLoadingMode.valueOf(lookupModeValue.toString());
```
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskQueryMaker.java:
##########
@@ -281,6 +282,16 @@ public QueryResponse<Object[]> runQuery(final DruidQuery
druidQuery)
.tuningConfig(new MSQTuningConfig(maxNumWorkers,
maxRowsInMemory, rowsPerSegment, indexSpec))
.build();
+ Map<String, Object> context = new HashMap<>();
+ if (plannerContext.getLookupsToLoad() != null) {
Review Comment:
the planner context should give a `LookupLoadingSpec` which should be used
to populate the context, rather than relying on null and empty values.
--
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]
