fix topN filtering on multi-valued dimension bug

[binlijin]

When i backport #2130 to our version and use topn with it, it throw ArrayIndexOutOfBoundsException.

2016-01-12T09:35:54,627 ERROR [processing-2] io.druid.query.ChainedExecutionQueryRunner - Exception with one of the sequences!
java.lang.ArrayIndexOutOfBoundsException: 17
        at io.druid.query.topn.PooledTopNAlgorithm.aggregateDimValue(PooledTopNAlgorithm.java:255) ~[druid-processing-0.8.3-rc2.jar:0.8.3-rc2]
        at io.druid.query.topn.PooledTopNAlgorithm.scanAndAggregate(PooledTopNAlgorithm.java:226) ~[druid-processing-0.8.3-rc2.jar:0.8.3-rc2]
        at io.druid.query.topn.PooledTopNAlgorithm.scanAndAggregate(PooledTopNAlgorithm.java:37) ~[druid-processing-0.8.3-rc2.jar:0.8.3-rc2]
        at io.druid.query.topn.BaseTopNAlgorithm.run(BaseTopNAlgorithm.java:92) ~[druid-processing-0.8.3-rc2.jar:0.8.3-rc2]
        at io.druid.query.topn.TopNMapFn.apply(TopNMapFn.java:57) ~[druid-processing-0.8.3-rc2.jar:0.8.3-rc2]
        at io.druid.query.topn.TopNMapFn.apply(TopNMapFn.java:26) ~[druid-processing-0.8.3-rc2.jar:0.8.3-rc2]
        at io.druid.query.topn.TopNQueryEngine$1.apply(TopNQueryEngine.java:82) ~[druid-processing-0.8.3-rc2.jar:0.8.3-rc2]
        at io.druid.query.topn.TopNQueryEngine$1.apply(TopNQueryEngine.java:77) ~[druid-processing-0.8.3-rc2.jar:0.8.3-rc2]
        at com.metamx.common.guava.MappingYieldingAccumulator.accumulate(MappingYieldingAccumulator.java:57) ~[java-util-0.27.4.jar:?]
        at com.metamx.common.guava.FilteringYieldingAccumulator.accumulate(FilteringYieldingAccumulator.java:69) ~[java-util-0.27.4.jar:?]
        at com.metamx.common.guava.MappingYieldingAccumulator.accumulate(MappingYieldingAccumulator.java:57) ~[java-util-0.27.4.jar:?]
        at com.metamx.common.guava.BaseSequence.makeYielder(BaseSequence.java:104) ~[java-util-0.27.4.jar:?]
        at com.metamx.common.guava.BaseSequence.toYielder(BaseSequence.java:81) ~[java-util-0.27.4.jar:?]
        at com.metamx.common.guava.MappedSequence.toYielder(MappedSequence.java:46) ~[java-util-0.27.4.jar:?]
        at com.metamx.common.guava.ResourceClosingSequence.toYielder(ResourceClosingSequence.java:41) ~[java-util-0.27.4.jar:?]
        at com.metamx.common.guava.FilteredSequence.toYielder(FilteredSequence.java:52) ~[java-util-0.27.4.jar:?]
        at com.metamx.common.guava.MappedSequence.toYielder(MappedSequence.java:46) ~[java-util-0.27.4.jar:?]
        at com.metamx.common.guava.FilteredSequence.toYielder(FilteredSequence.java:52) ~[java-util-0.27.4.jar:?]        at com.metamx.common.guava.ResourceClosingSequence.toYielder(ResourceClosingSequence.java:41) ~[java-util-0.27.4.jar:?]        at com.metamx.common.guava.YieldingSequenceBase.accumulate(YieldingSequenceBase.java:34) ~[java-util-0.27.4.jar:?]        at io.druid.query.MetricsEmittingQueryRunner$1.accumulate(MetricsEmittingQueryRunner.java:118) ~[druid-processing-0.8.3-rc2.jar:0.8.3-rc2]        at com.metamx.common.guava.MappedSequence.accumulate(MappedSequence.java:40) ~[java-util-0.27.4.jar:?]
        at com.metamx.common.guava.Sequences$1.accumulate(Sequences.java:90) ~[java-util-0.27.4.jar:?]
        at io.druid.query.MetricsEmittingQueryRunner$1.accumulate(MetricsEmittingQueryRunner.java:118) ~[druid-processing-0.8.3-rc2.jar:0.8.3-rc2]
        at io.druid.query.spec.SpecificSegmentQueryRunner$2$1.call(SpecificSegmentQueryRunner.java:85) ~[druid-processing-0.8.3-rc2.jar:0.8.3-rc2]
        at io.druid.query.spec.SpecificSegmentQueryRunner.doNamed(SpecificSegmentQueryRunner.java:169) ~[druid-processing-0.8.3-rc2.jar:0.8.3-rc2]
        at io.druid.query.spec.SpecificSegmentQueryRunner.access$400(SpecificSegmentQueryRunner.java:39) ~[druid-processing-0.8.3-rc2.jar:0.8.3-rc2]
        at io.druid.query.spec.SpecificSegmentQueryRunner$2.doItNamed(SpecificSegmentQueryRunner.java:160) ~[druid-processing-0.8.3-rc2.jar:0.8.3-rc2]
        at io.druid.query.spec.SpecificSegmentQueryRunner$2.accumulate(SpecificSegmentQueryRunner.java:78) ~[druid-processing-0.8.3-rc2.jar:0.8.3-rc2]
        at io.druid.query.CPUTimeMetricQueryRunner$1.accumulate(CPUTimeMetricQueryRunner.java:83) ~[druid-processing-0.8.3-rc2.jar:0.8.3-rc2]
        at com.metamx.common.guava.Sequences$1.accumulate(Sequences.java:90) ~[java-util-0.27.4.jar:?]
        at com.metamx.common.guava.Sequences.toList(Sequences.java:113) ~[java-util-0.27.4.jar:?]
        at io.druid.query.ChainedExecutionQueryRunner$1$1$1.call(ChainedExecutionQueryRunner.java:130) [druid-processing-0.8.3-rc2.jar:0.8.3-rc2]
        at io.druid.query.ChainedExecutionQueryRunner$1$1$1.call(ChainedExecutionQueryRunner.java:120) [druid-processing-0.8.3-rc2.jar:0.8.3-rc2]
        at java.util.concurrent.FutureTask.run(FutureTask.java:262) [?:1.7.0_51]
        at io.druid.query.PrioritizedExecutorService$PrioritizedListenableFutureTask.run(PrioritizedExecutorService.java:222) [druid-processing-0.8.3-rc2.jar:0.8.3-rc2]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145) [?:1.7.0_51]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615) [?:1.7.0_51]

[fjy]

👍

[drcrallen]

io.druid.query.dimension.RegexFilteredDimensionSpecTest has no tests for correctness. This makes the correctness of this patch harder to judge.

If I'm reading this correctly, the error comes from the fact that the topN is returning ALL dimension values in a multi-value dimension instead of just the ones that match the filter, is that correct?

If so, then it would make more sense to me to have the topN function as expected. From the contract on io.druid.segment.DimensionSelector#getValueCardinality it LOOKS like io.druid.query.dimension.RegexFilteredDimensionSpec#decorate is behaving properly.

Can you comment a bit more on why the prior cardinality methods were insufficient?

[himanshug]

this breaks contract of getValueCardinality(), this should return cardinality "after" filtering.

[himanshug]

@binlijin i have to dig more into the original issue you faced but getValueCardinality() impls are doing the right thing as per the contract of those methods.
It will be useful if you could explain why existing implementations are causing problems for TopN query?

[fjy]

@himanshug, how does this pass unit tests? I'm surprised no UT caught this

[himanshug]

@fjy FilteredDimensionSpec is new feature and is tested with groupBy queries in the unit tests. looks like TopN can't handle the new DimensionSpec with correct cardinality implementation for some reason.

[binlijin]

@drcrallen @himanshug
TopN and GroupBy they all iterate Cursor to all valid rows. For all the valid rows if have multi-value dimensions will can be have valid dimension values together with invalid dimension values.
TopN also need the dimension's cardinality and will iterate all the dimension values(can be valid and invalid), but groupBy do not need dimension's cardinality and values, so the groupby have no problem.
I think ListFilteredDimensionSpec and RegexFilteredDimensionSpec replace a row's value (which include valid and invalid value) to just valid value(do not include invalid value), they should not change the dimension's ValueCardinality.

[binlijin]

@himanshug
If we change the dimension's ValueCardinality, which will smaller than the first one, but we don't replace the corresponding dimIndex which will big than the changed ValueCardinality, so the ArrayIndexOutOfBoundsException will happen.

final IndexedInts dimValues = dimSelector.getRow();

[himanshug]

@binlijin yes, but we really need to fix the mapping instead of returning higher cardinality.
I have taken your commit and put the change on top. see #2267 . closing this one.
(created new PR so that I can address review comments from people on the approach chosen).

[himanshug]

also, the reason that we want to work with as small cardinality as possible because topN algorithm will allocate some buffers according to cardinality. if we return higher cardinality then those buffer will be bigger than they need to be. For example, say you used List filtered spec and with only 1 item in the list, then with reduced cardinality those buffers will be of size 1 only.