Support custom Druid queries. #57
Conversation
| List<Result> results; | ||
| switch (queryType) { | ||
| List<Result> results = null; | ||
| switch (defaultQueryType) { |
There was a problem hiding this comment.
The "missing" break is in the default case.
|
Do we want to make a test on DruidResponseParserSpec to exercise the unsupported operation exception on a non enum instance of the query type interface. |
| - [Fili now supports custom Druid query types](https://github.com/yahoo/fili/pull/57) | ||
| * `QueryType` has been turned into an interface, backed by an enum `DefaultQueryType`. | ||
| * `DruidResponseParser` is now injectable by overriding `AbstractBinderFactory::buildDruidResponseParser` method. | ||
| * `DruidQueryBuilder` is now injectable by overriding `AbstractBinderFactory::buildDruidQueryBuilder` method. |
There was a problem hiding this comment.
Wasn't this already overridable?
| /* Get dimension and metric columns */ | ||
| Set<DimensionColumn> dimensionColumns = schema.getColumns(DimensionColumn.class); | ||
| Set<MetricColumn> metricColumns = schema.getColumns(MetricColumn.class); | ||
|
|
||
| List<Result> results; | ||
| switch (queryType) { | ||
| List<Result> results = null; |
There was a problem hiding this comment.
Is this needed? It defaults to null
There was a problem hiding this comment.
It's needed to make the java compiler happy. Otherwise, it complains that results may not have been initialized. Likely because it doesn't realize that the default case throws an exception, now that the exception throwing is hidden behind a method.
| * | ||
| * @return the set of results | ||
| */ | ||
| public ResultSet parse(JsonNode jsonResult, ZonedSchema schema, QueryType queryType) { | ||
|
|
||
| LOG.trace("Parsing druid query {} by json result: {} using schema: {}", queryType, jsonResult, schema); | ||
|
|
||
|
|
There was a problem hiding this comment.
Extra whitespace not needed
| @@ -114,7 +115,12 @@ public void postDruidQuery( | |||
| } | |||
|
|
|||
| // Set the response to use based on the type of the query we're processing | |||
| switch (lastQuery.getQueryType()) { | |||
| if (!(lastQuery.getQueryType() instanceof DefaultQueryType)) { | |||
| throw new IllegalArgumentException("Illegal query type : " + lastQuery.getQueryType()); | |||
There was a problem hiding this comment.
We should add a note (likely to the changelog) that this test extension point doesn't support new types of queries.
|
|
||
| if (!(queryType instanceof DefaultQueryType)) { | ||
| // Throw an exception for unsupported query types | ||
| unsupportedQueryType(queryType); |
There was a problem hiding this comment.
We should add a note (likely to the changelog) that this test extension point doesn't support new types of queries.
| postAggregations = Collections.<PostAggregation>singletonList(new ConstantPostAggregation("count", weight)); | ||
| postAggregations = Collections.singletonList(new ConstantPostAggregation("count", weight)); | ||
|
|
||
| if (!(innerQuery.getQueryType() instanceof DefaultQueryType)) { |
There was a problem hiding this comment.
We should add a note (likely to the changelog) that this test extension point doesn't support new types of queries.
archolewa
force-pushed
the
support-custom-query-types
branch
from
1aad05f to
37e2030
Compare
|
👍 |
--`QueryType` has been turned into an interface backed by an enum. --The `DruidResponseParser` and `DruidQueryBuilder` are now injectable.
archolewa
force-pushed
the
support-custom-query-types
branch
from
37e2030 to
463d267
Compare
--
QueryTypehas been turned into an interface backed by an enum.--The
DruidResponseParserandDruidQueryBuilderare now injectable.