Assorted fili cleanups #107
Conversation
|
👍 |
| * If this test fails for you you may need to update ClassScanner to handle your special argument types, | ||
| * add values to the class scanner value cache above, or add your class to the ignored list in | ||
| * getClassesDeclaring. | ||
| * |
There was a problem hiding this comment.
These blank lines in the JavaDoc description should have <p> instead of just the blank line to make sure the JavaDoc has paragraphs in it.
| * @param aggregation the underlying aggregation | ||
| * @param filter the filter to filter the aggregation by | ||
| */ | ||
| public FilteredAggregationMaker(MetricDictionary metricDictionary, Aggregation aggregation, Filter filter) { |
There was a problem hiding this comment.
While I think this is fine for now, since it makes it possible to build filtered metrics, I think we should re-visit this maker and beef it up a bit so that it's usable in the same sort of building-block way that the other makers can be used.
Ideally, this Maker could take a single LogicalMetric and convert it to be a filtered version of that metric. To do so, there are 2 main steps that have to happen:
- Replace all of the aggregations on the dependent metric by wrapping them aggregations in a
FilteredAggregation- Each new aggregator should also be given a new name that is deterministically created from the filter. This is needed to prevent metric merging from accidentally using a filtered aggregation as if it were non-filtered. The deterministic part is so that equally filtered aggregations will be properly merged if they can be
- Re-build the post-agg tree using these new aggregators
Once this Maker works this way, the aggregation no longer needs to be passed into the constructor, only the Filter to apply will need to be passed in at construction time.
We already have code to support sketch intersection metrics (specified in the MetricsFilterSetBuilder interface) that can do some of this, though it's done in a bit of an awkward way. We should consider deprecating that existing mechanism, and instead opt for this Maker-based mechanism. If we have a proper FilteredMetricMaker, then we can easily use that along with the *SketchSetOperationMaker to build the intersection metrics at query time.
| public FilteredAggregationMaker(MetricDictionary metricDictionary, Aggregation aggregation, Filter filter) { | ||
| super(metricDictionary); | ||
| this.aggregation = aggregation; | ||
| this.filter = filter; |
There was a problem hiding this comment.
Perhaps it would be better to pass in ApiFilter objects instead of Druid's filter model? The idea is that defining metrics is a logical operation, not a physical operation tied to Druid. Since the FilteredAggregation ultimately needs a Druid filter, however, this means that you'd also need to pass in a DruidFilterBuilder instance.
If it's the case that the API filters are not rich enough, passing in a Collection<ApiFilter> would likely also work as well.
Note: I'm also OK coming back to this when we re-work this maker, as long as this is mentioned in that ticket.
jacobtolar
force-pushed
the
fili-cleanups
branch
from
5a7dc63 to
0952e0f
Compare
jacobtolar
force-pushed
the
fili-cleanups
branch
from
0952e0f to
ba89f5e
Compare
|
Added a bunch of Filed #120 to address the FilteredAggregationMaker concerns. |
|
Merged. Thanks for the contribution! |
Note: this is the first child of #104