Null Pointer Exception in MapBasedRow.compareTo

[niketh]

We get a NPE when we fire groupBy queries using Broker 0.9.1.1 and Historicals (0.10).

2017-03-07T17:52:37,896 ERROR [qtp900593187-194[groupBy_abc-groupBy2]] io.druid.server.QueryResource - Exception handling request: {class=io.druid.server.QueryRe java.lang.NullPointerException at io.druid.data.input.MapBasedRow.compareTo(MapBasedRow.java:203) ~[druid-api-0.9.1.1.jar:0.9.1.1] at io.druid.data.input.MapBasedRow.compareTo(MapBasedRow.java:39) ~[druid-api-0.9.1.1.jar:0.9.1.1] at com.google.common.collect.NaturalOrdering.compare(NaturalOrdering.java:35) ~[guava-16.0.1.jar:?] at com.google.common.collect.NaturalOrdering.compare(NaturalOrdering.java:26) ~[guava-16.0.1.jar:?] at com.google.common.collect.ByFunctionOrdering.compare(ByFunctionOrdering.java:46) ~[guava-16.0.1.jar:?] at java.util.PriorityQueue.siftUpUsingComparator(PriorityQueue.java:669) ~[?:1.8.0_112] at java.util.PriorityQueue.siftUp(PriorityQueue.java:645) ~[?:1.8.0_112] at java.util.PriorityQueue.offer(PriorityQueue.java:344) ~[?:1.8.0_112] at java.util.PriorityQueue.add(PriorityQueue.java:321) ~[?:1.8.0_112] at com.metamx.common.guava.MergeSequence$2.accumulate(MergeSequence.java:80) ~[java-util-0.27.9.jar:?] at com.metamx.common.guava.MergeSequence$2.accumulate(MergeSequence.java:62) ~[java-util-0.27.9.jar:?] at com.metamx.common.guava.YieldingAccumulators$1.accumulate(YieldingAccumulators.java:32) ~[java-util-0.27.9.jar:?] at com.metamx.common.guava.BaseSequence.makeYielder(BaseSequence.java:104) ~[java-util-0.27.9.jar:?] at com.metamx.common.guava.BaseSequence.toYielder(BaseSequence.java:81) ~[java-util-0.27.9.jar:?] at com.metamx.common.guava.BaseSequence.accumulate(BaseSequence.java:67) ~[java-util-0.27.9.jar:?] at com.metamx.common.guava.MergeSequence.toYielder(MergeSequence.java:59) ~[java-util-0.27.9.jar:?] at com.metamx.common.guava.LazySequence.toYielder(LazySequence.java:43) ~[java-util-0.27.9.jar:?] at io.druid.query.RetryQueryRunner$1.toYielder(RetryQueryRunner.java:105) ~[druid-processing-0.9.1.1.jar:0.9.1.1] at com.metamx.common.guava.YieldingSequenceBase.accumulate(YieldingSequenceBase.java:34) ~[java-util-0.27.9.jar:?] at io.druid.query.groupby.GroupByQueryQueryToolChest.makeIncrementalIndex(GroupByQueryQueryToolChest.java:319) ~[druid-processing-0.9.1.1.jar:0.9.1.1] at io.druid.query.groupby.GroupByQueryQueryToolChest.mergeGroupByResults(GroupByQueryQueryToolChest.java:257) ~[druid-processing-0.9.1.1.jar:0.9.1.1] at io.druid.query.groupby.GroupByQueryQueryToolChest.access$000(GroupByQueryQueryToolChest.java:85) ~[druid-processing-0.9.1.1.jar:0.9.1.1] at io.druid.query.groupby.GroupByQueryQueryToolChest$3.run(GroupByQueryQueryToolChest.java:134) ~[druid-processing-0.9.1.1.jar:0.9.1.1] at io.druid.query.FinalizeResultsQueryRunner.run(FinalizeResultsQueryRunner.java:103) ~[druid-processing-0.9.1.1.jar:0.9.1.1] at io.druid.query.CPUTimeMetricQueryRunner.run(CPUTimeMetricQueryRunner.java:72) ~[druid-processing-0.9.1.1.jar:0.9.1.1] at io.druid.query.FluentQueryRunnerBuilder$FluentQueryRunner.run(FluentQueryRunnerBuilder.java:58) ~[druid-processing-0.9.1.1.jar:0.9.1.1] at io.druid.query.BaseQuery.run(BaseQuery.java:151) ~[druid-processing-0.9.1.1.jar:0.9.1.1] at io.druid.query.BaseQuery.run(BaseQuery.java:146) ~[druid-processing-0.9.1.1.jar:0.9.1.1]

[niketh]

@cheddar @gianm A solution is to make the Historical use V1 incase the groupByStrategy is not specified in the query. But by changing that logic in GroupByQueryConfig, even the broker gets affected. And every query to the Broker would require us to pass in "GroupByQueryConfig"

[fjy]

@niketh please update to 0.9.2 first

[gianm]

@fjy imo, if feasible, it's best to make it easy for people to upgrade from any 0.9.x to 0.10.0.

@niketh I think we need different behavior on the broker and historical: because on the broker we want to default to v2 if not specified (since v2 is default generally) but on the historical we may want to default to v1 if not specified (for compatibility with 0.9.0 and 0.9.1 brokers). There is some precedent for behavior differences this, like #3950 which disables caching on brokers but not historicals. Something similar might work here?

[gianm]

also, another option: on the historical, we may still want to default to v2 if not specified, but in that case set a "compatibility flag" that causes them to return timestamps even when timestamps are not necessary. This would let them get the performance benefits of v2 but still be compatible with older brokers.

[niketh]

@gianm Compatibility flag makes sense 👍 . Will post a PR shortly.

[niketh]

@gianm Also why not always return back timestamps like in the case of GroupByV1 ? The payload size increase should be very minimal.

[gianm]

@niketh serialization and deserialization of the timestamp between historicals -> broker was taking a lot of time in our profiling -- see #3740 where we measured a 30% speedup by nulling out the timestamp.

[niketh]

Created PR #4022