NPE when using max_by/min_by

[martint]

java.lang.NullPointerException
    at com.facebook.presto.spi.type.DoubleType.compareTo(DoubleType.java:72)
    at com.facebook.presto.operator.aggregation.MaxBy.combine(MaxBy.java:125)
    at com.facebook.presto.$gen.VarcharVarcharDoubleMaxByGroupedAccumulator_576.addIntermediate(Unknown Source)
    at com.facebook.presto.operator.HashAggregationOperator$Aggregator.processPage(HashAggregationOperator.java:388)
    at com.facebook.presto.operator.HashAggregationOperator$GroupByHashAggregationBuilder.processPage(HashAggregationOperator.java:296)
    at com.facebook.presto.operator.HashAggregationOperator$GroupByHashAggregationBuilder.access$200(HashAggregationOperator.java:263)
    at com.facebook.presto.operator.HashAggregationOperator.addInput(HashAggregationOperator.java:212)
    at com.facebook.presto.operator.Driver.processInternal(Driver.java:357)
    at com.facebook.presto.operator.Driver.processFor(Driver.java:283)
    at com.facebook.presto.execution.SqlTaskExecution$DriverSplitRunner.processFor(SqlTaskExecution.java:557)
    at com.facebook.presto.execution.TaskExecutor$PrioritizedSplitRunner.process(TaskExecutor.java:444)
    at com.facebook.presto.execution.TaskExecutor$Runner.run(TaskExecutor.java:578)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)

[martint]

This can happen if the state contains a single-valued block for the key field with a "null" position coming out of the worker. When the object is deserialized on the other side, instead of recreating the single-valued block, it leave the key null. The combine method doesn't expect "other" to ever be null, so it blows up.

The issue is partly due to inconsistent handling of nulls in MaxBy/MinBy. We should pick whether we're going to represent them as single-valued blocks with a single null position or as plain old null reference.

[martint]

We also need better test coverage for these aggregation functions.

[huanghuashou]

I wanna work on this issue.

[martint]

@huanghuashou are you still working on this?

[huanghuashou]

Sorry for late, working on it now. Will provide a diff soon.

[lstyls]

Hi @martint, I'm going to be working on this one. I'm new to presto and DB engine development in general. Can you advise on how to repro the exception?

[martint]

I haven't been able to come up with a query that reproduces this deterministically. You might need to write a unit test that exercises the API of those functions directly.