kfaraz commented on code in PR #16358:
URL: https://github.com/apache/druid/pull/16358#discussion_r1590642465
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQWorkerTask.java:
##########
@@ -185,4 +190,26 @@ public int hashCode()
{
return Objects.hash(super.hashCode(), controllerTaskId, workerNumber,
retryCount, worker);
}
+
+ @Override
+ public LookupLoadingSpec getLookupLoadingSpec()
+ {
+ final Object lookupModeValue =
getContext().get(PlannerContext.CTX_LOOKUP_LOADING_MODE);
+ if (lookupModeValue == null) {
+ return LookupLoadingSpec.ALL;
+ }
+
+ final LookupLoadingSpec.Mode lookupLoadingMode =
LookupLoadingSpec.Mode.valueOf(lookupModeValue.toString());
+ if (lookupLoadingMode == LookupLoadingSpec.Mode.NONE) {
+ return LookupLoadingSpec.NONE;
+ } else if (lookupLoadingMode == LookupLoadingSpec.Mode.ONLY_REQUIRED) {
+ List<String> lookupsToLoad = (List<String>)
getContext().get(PlannerContext.CTX_LOOKUPS_TO_LOAD);
+ if (lookupsToLoad == null) {
+ throw InvalidInput.exception("Set of lookups to load cannot be NULL
for mode = ONLY_REQUIRED.");
Review Comment:
```suggestion
throw InvalidInput.exception("Set of lookups to load cannot be NULL
for mode[ONLY_REQUIRED].");
```
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java:
##########
@@ -343,6 +349,28 @@ public String getSchemaResourceType(String schema, String
resourceName)
return plannerToolbox.rootSchema().getResourceType(schema, resourceName);
}
+ /**
+ * Add a lookup name to load in the lookup loading spec.
+ */
+ public void addLookupToLoad(String lookupName)
+ {
+ if (lookupLoadingSpec.getLookupsToLoad() == null) {
+ lookupLoadingSpec =
LookupLoadingSpec.loadOnly(Collections.singleton(lookupName));
+ } else {
+ Set<String> existingLookupsToLoad = new
HashSet<>(lookupLoadingSpec.getLookupsToLoad());
+ existingLookupsToLoad.add(lookupName);
+ lookupLoadingSpec = LookupLoadingSpec.loadOnly(existingLookupsToLoad);
+ }
+ }
+
+ /**
+ * Returns the lookup loading spec for a given task.
Review Comment:
```suggestion
* Lookup loading spec for MSQ tasks.
```
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java:
##########
@@ -343,6 +349,28 @@ public String getSchemaResourceType(String schema, String
resourceName)
return plannerToolbox.rootSchema().getResourceType(schema, resourceName);
}
+ /**
+ * Add a lookup name to load in the lookup loading spec.
+ */
+ public void addLookupToLoad(String lookupName)
+ {
+ if (lookupLoadingSpec.getLookupsToLoad() == null) {
+ lookupLoadingSpec =
LookupLoadingSpec.loadOnly(Collections.singleton(lookupName));
+ } else {
+ Set<String> existingLookupsToLoad = new
HashSet<>(lookupLoadingSpec.getLookupsToLoad());
+ existingLookupsToLoad.add(lookupName);
+ lookupLoadingSpec = LookupLoadingSpec.loadOnly(existingLookupsToLoad);
Review Comment:
As an alternative, I suppose we could have just maintained a set of lookups
to load in this class as you were doing originally.
To start with, the set would be empty and we would keep adding stuff to it
in `addLookupToLoad()`.
We would still have `getLookupLoadingSpec()` where we could have this logic:
```java
return lookupsToLoad.isEmpty() ? LookupLoadingSpec.NONE :
LookupLoadingSpec.loadOnly(lookupsToLoad);
```
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java:
##########
@@ -343,6 +349,28 @@ public String getSchemaResourceType(String schema, String
resourceName)
return plannerToolbox.rootSchema().getResourceType(schema, resourceName);
}
+ /**
+ * Add a lookup name to load in the lookup loading spec.
Review Comment:
```suggestion
* Adds the given lookup name to the lookup loading spec.
```
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQWorkerTask.java:
##########
@@ -185,4 +190,26 @@ public int hashCode()
{
return Objects.hash(super.hashCode(), controllerTaskId, workerNumber,
retryCount, worker);
}
+
+ @Override
+ public LookupLoadingSpec getLookupLoadingSpec()
+ {
+ final Object lookupModeValue =
getContext().get(PlannerContext.CTX_LOOKUP_LOADING_MODE);
+ if (lookupModeValue == null) {
+ return LookupLoadingSpec.ALL;
+ }
+
+ final LookupLoadingSpec.Mode lookupLoadingMode =
LookupLoadingSpec.Mode.valueOf(lookupModeValue.toString());
+ if (lookupLoadingMode == LookupLoadingSpec.Mode.NONE) {
+ return LookupLoadingSpec.NONE;
+ } else if (lookupLoadingMode == LookupLoadingSpec.Mode.ONLY_REQUIRED) {
+ List<String> lookupsToLoad = (List<String>)
getContext().get(PlannerContext.CTX_LOOKUPS_TO_LOAD);
+ if (lookupsToLoad == null) {
Review Comment:
We should check for empty too.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java:
##########
@@ -343,6 +349,28 @@ public String getSchemaResourceType(String schema, String
resourceName)
return plannerToolbox.rootSchema().getResourceType(schema, resourceName);
}
+ /**
+ * Add a lookup name to load in the lookup loading spec.
+ */
+ public void addLookupToLoad(String lookupName)
+ {
+ if (lookupLoadingSpec.getLookupsToLoad() == null) {
+ lookupLoadingSpec =
LookupLoadingSpec.loadOnly(Collections.singleton(lookupName));
+ } else {
+ Set<String> existingLookupsToLoad = new
HashSet<>(lookupLoadingSpec.getLookupsToLoad());
+ existingLookupsToLoad.add(lookupName);
+ lookupLoadingSpec = LookupLoadingSpec.loadOnly(existingLookupsToLoad);
Review Comment:
So, every time we call `addLookupToLoad()`, we will end up creating a new
instance of `LookupLoadingSpec`?
I suppose we don't have a choice since the set of lookups in
`LookupLoadingSpec` is immutable.
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java:
##########
@@ -142,6 +146,8 @@ public class PlannerContext
// set of attributes for a SQL statement used in the EXPLAIN PLAN output
private ExplainAttributes explainAttributes;
private PlannerLookupCache lookupCache;
+ // Lookup loading spec for a given task
Review Comment:
```suggestion
/**
* Lookup loading spec used by MSQ tasks.
*/
```
--
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]
