cryptoe commented on code in PR #16358:
URL: https://github.com/apache/druid/pull/16358#discussion_r1588624119
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQWorkerTask.java:
##########
@@ -185,4 +189,15 @@ public int hashCode()
{
return Objects.hash(super.hashCode(), controllerTaskId, workerNumber,
retryCount, worker);
}
+
+ @Override
+ public LookupLoadingSpec getLookupLoadingSpec()
+ {
+ if (getContext().containsKey(PlannerContext.CTX_LOOKUPS_TO_LOAD)) {
Review Comment:
* I think this method of getting something from context and getting a
lookupLoading spec should be in an utility class since this would be used in
multiple places.
* I donot see the context key's being passed to the worker.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQControllerTask.java:
##########
@@ -333,4 +336,15 @@ public static boolean writeResultsToDurableStorage(final
MSQSpec querySpec)
{
return querySpec.getDestination() instanceof DurableStorageMSQDestination;
}
+
+ @Override
+ public LookupLoadingSpec getLookupLoadingSpec()
+ {
+ if
(getQuerySpec().getQuery().getContext().containsKey(PlannerContext.CTX_LOOKUPS_TO_LOAD))
{
Review Comment:
This should be none always. Controller does not require lookups yet.
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQWorkerTask.java:
##########
@@ -185,4 +189,15 @@ public int hashCode()
{
return Objects.hash(super.hashCode(), controllerTaskId, workerNumber,
retryCount, worker);
}
+
+ @Override
+ public LookupLoadingSpec getLookupLoadingSpec()
+ {
+ if (getContext().containsKey(PlannerContext.CTX_LOOKUPS_TO_LOAD)) {
+ List<String> lookupsToLoad = (List<String>)
getContext().get(PlannerContext.CTX_LOOKUPS_TO_LOAD);
+ return lookupsToLoad.isEmpty() ? LookupLoadingSpec.NONE :
LookupLoadingSpec.loadOnly(new HashSet<>(lookupsToLoad));
Review Comment:
I think it might be fine to have 2 context keys.
1. lookupLoadingMOde="ALL,NONE,partial"
2. lookupsToLoad="[a,b,c]"
Passing empty arrays seems very brittle to me. Some payload optimizations
just remove the keys while passing stuff over the wire.
Also the user basically has to set both keys
partial
and list<lookups> when he want to enable selective loading.
If he sets only one value, its an error in this case.
--
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]
