rohangarg commented on code in PR #15465:
URL: https://github.com/apache/druid/pull/15465#discussion_r1424146811
##########
server/src/test/java/org/apache/druid/server/QueryStackTests.java:
##########
@@ -135,35 +161,10 @@ public <T, QueryType extends Query<T>> QueryToolChest<T,
QueryType> getToolChest
},
joinableFactory,
new RetryQueryRunnerConfig(),
- TestHelper.makeJsonMapper(),
+ injector.getInstance(ObjectMapper.class), //
TestHelper.makeJsonMapper(), // X$#%$#
Review Comment:
extra diff
##########
processing/src/main/java/org/apache/druid/query/CacheStrategy.java:
##########
@@ -155,10 +157,16 @@ static void fetchAggregatorsFromCache(
throw new ISE("Ran out of objects while reading aggregators from
cache!");
}
- if (isResultLevelCache) {
- addToResultFunction.apply(aggregator.getName(), i, resultIter.next());
- } else {
+ ColumnType resultType = aggregator.getResultType();
+ ColumnType intermediateType = aggregator.getIntermediateType();
+
+ boolean needsDeserialize = !isResultLevelCache
+ || (!resultType.isPrimitive() &&
resultType.equals(intermediateType));
Review Comment:
is there a specific reason for adding `!resultType.isPrimitive` ?
##########
server/src/test/java/org/apache/druid/server/QueryStackTests.java:
##########
@@ -135,35 +161,10 @@ public <T, QueryType extends Query<T>> QueryToolChest<T,
QueryType> getToolChest
},
joinableFactory,
new RetryQueryRunnerConfig(),
- TestHelper.makeJsonMapper(),
+ injector.getInstance(ObjectMapper.class), //
TestHelper.makeJsonMapper(), // X$#%$#
serverConfig,
- null /* Cache */,
- new CacheConfig()
- {
- @Override
- public boolean isPopulateCache()
- {
- return false;
- }
-
- @Override
- public boolean isUseCache()
- {
- return false;
- }
-
- @Override
- public boolean isPopulateResultLevelCache()
- {
- return false;
- }
-
- @Override
- public boolean isUseResultLevelCache()
- {
- return false;
- }
- },
+ injector.getInstance(Key.get(Cache.class, Testrelated.class)),
Review Comment:
is the `Testrelated` interface really needed? if so, we can assign a better
name to it.
##########
server/src/main/java/org/apache/druid/segment/metadata/AbstractSegmentMetadataCache.java:
##########
@@ -881,11 +882,16 @@ public Sequence<SegmentAnalysis> runSegmentMetadataQuery(
querySegmentSpec,
new AllColumnIncluderator(),
false,
- // disable the parallel merge because we don't care about the merge
and don't want to consume its resources
QueryContexts.override(
internalQueryConfig.getContext(),
- QueryContexts.BROKER_PARALLEL_MERGE_KEY,
- false
+ ImmutableMap.of(
+ // disable the parallel merge because we don't care about the
merge and don't want to consume its resources
+ QueryContexts.BROKER_PARALLEL_MERGE_KEY,
+ false,
+ // dont use result cache for metadata
+ QueryContexts.POPULATE_RESULT_LEVEL_CACHE_KEY,
Review Comment:
why was this change needed?
##########
server/src/test/java/org/apache/druid/server/QueryStackTests.java:
##########
@@ -135,35 +161,10 @@ public <T, QueryType extends Query<T>> QueryToolChest<T,
QueryType> getToolChest
},
joinableFactory,
new RetryQueryRunnerConfig(),
- TestHelper.makeJsonMapper(),
+ injector.getInstance(ObjectMapper.class), //
TestHelper.makeJsonMapper(), // X$#%$#
serverConfig,
- null /* Cache */,
- new CacheConfig()
- {
- @Override
- public boolean isPopulateCache()
- {
- return false;
- }
-
- @Override
- public boolean isUseCache()
- {
- return false;
- }
-
- @Override
- public boolean isPopulateResultLevelCache()
- {
- return false;
- }
-
- @Override
- public boolean isUseResultLevelCache()
- {
- return false;
- }
- },
+ injector.getInstance(Key.get(Cache.class, Testrelated.class)),
Review Comment:
Also, can we consider getting these objects from a static method or a simple
factory instead of passing injector to constructors?
--
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]
