chunkPeriod parameter in context with TopN query produces incorrect results

[himanshug]

resultset seem to contain data only for last chunk instead of all.

see https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/druid-user/RmLszymJ3Ro/2SbW9tksBgAJ

[gianm]

@himanshug do you know if this is still an issue? We ran into a similar issue with groupBy v2 and I'm wondering if IntervalChunkingQueryRunner is just generally broken. It chunks up the query and then concats the returned sequences without putting them through a merger, which seems like a bad idea…

[himanshug]

@gianm

if IntervalChunkingQueryRunner is just generally broken. It chunks up the query and then concats the returned sequences without putting them through a merger, which seems like a bad idea…

it does put the results obtained from individual chunks through toolChest.mergeResults(..) before concatenation...

pipeline is like.....

new FluentQueryRunnerBuilder<>(toolChest)
        .create(
            new RetryQueryRunner<>(
                baseClient,
                toolChest,
                retryConfig,
                objectMapper
            )
        )
        .applyPreMergeDecoration()  //chunking, merging of individual chunk results and their concatenation happens here
        .mergeResults()
        .applyPostMergeDecoration()
        .emitCPUTimeMetric(emitter)
        .postProcess(postProcessing);

there was another issue reported https://groups.google.com/forum/#!topic/druid-user/S8zfWDIrncQ .... i investigated that one in detail and found #2586 was the cause ... it was fixed in #2601 ... and the issue reported was acknowledged to be fixed by that patch.

I can't remember if I really looked much into topN case , however chunkPeriod was mostly intended to optimize the groupBy-v1 queries where it served well... we should probably document to use it only with groupBy.

it should work with groupByV2 ideally, does it not work even for flat groupBy-v2 queries?

however benefits of chunking with sort-merge in v2 might not be same, so may be it is time to retire chunking along with groupBy-V1 .

[gianm]

What do you mean by "flat" groupBy queries? No subqueries? The query we ran into problems on was non-nested and had granularity "all". It did have a union datasource though.

I was talking about this part of IntervalChunkingQueryRunner:

    return Sequences.concat(
        Lists.newArrayList(
            FunctionalIterable.create(chunkIntervals).transform(
                new Function<Interval, Sequence<T>>()
                {
                  @Override
                  public Sequence<T> apply(Interval singleInterval)
                  {
                    return new AsyncQueryRunner<T>(
                        //Note: it is assumed that toolChest.mergeResults(..) gives a query runner that is
                        //not lazy i.e. it does most of its work on call to run() method
                        toolChest.mergeResults(
                            new MetricsEmittingQueryRunner<T>(
                                emitter,
                                new Function<Query<T>, ServiceMetricEvent.Builder>()
                                {
                                  @Override
                                  public ServiceMetricEvent.Builder apply(Query<T> input)
                                  {
                                    return toolChest.makeMetricBuilder(input);
                                  }
                                },
                                baseRunner,
                                "query/intervalChunk/time",
                                ImmutableMap.of("chunkInterval", singleInterval.toString())
                            ).withWaitMeasuredFromNow()
                        ),
                        executor, queryWatcher
                    ).run(
                        query.withQuerySegmentSpec(new MultipleIntervalSegmentSpec(Arrays.asList(singleInterval))),
                        responseContext
                    );
                  }
                }
            )
        )
    );

Each individual chunk of results is merged with mergeResults but then the Sequences.concat is applied to the chunks. I was thinking that concat of the chunk results makes it not possible to do a streaming merge later on, which is done by all the query types other than groupBy v1 (it materializes and re-sorts rather than doing a streaming merge).

I think the current code should work ok for granularity "all" in query types like topN and timeseries but might not work for granularity != "all". I haven't tested this yet though. Do you know if the topN query that was confirmed fixed by #2601 used granularity "all" or not?

[gianm]

I think the current code should work ok for granularity "all" in query types like topN and timeseries but might not work for granularity != "all".

Fwiw the reason I'm guessing this is because with granularity "all", for topN and timeseries, there will only be one result in the end. So concat is fine since they'll all end up getting merged together. With granularity != "all" there should be multiple results in the end and the concat could make results that should get merged be non-adjacent when they come from different chunks, and that non-adjacency would mess up streaming merging.

[gianm]

Maybe it would be enough to use new MergeSequence(query.getResultOrdering(), ...) rather than Sequences.concat(...).

[himanshug]

@gianm good point... yeah, switching to MergeSequence(..) would fix it for streaming merge.... when I did it , I was probably too much into groupBy-v1 merging .
with that change, it should start working for groupBy V2

however , we should still limit it only to groupBy queries probably ... topN e.g. might still produce different result with/without chunking due to the limit push down on aggregator and approximate nature.

[himanshug]

also, after MergeSequence change and with streaming merge in groupBy v2, most likely toolchest.mergeResults(..) call inside chunking query runner can be removed.

[gianm]

@himanshug I can start working on a patch for this unless you already are… please let me know either way

[himanshug]

@gianm feel free to add the patch ... i haven't started on anything. thanks

[gianm]

Raised a patch with tests in #4004