Skip to content

Remove unused limitFn in GroupByQuery#4935

Merged
gianm merged 2 commits intoapache:masterfrom
jon-wei:unused_limitfn
Oct 10, 2017
Merged

Remove unused limitFn in GroupByQuery#4935
gianm merged 2 commits intoapache:masterfrom
jon-wei:unused_limitfn

Conversation

@jon-wei
Copy link
Copy Markdown
Contributor

@jon-wei jon-wei commented Oct 10, 2017

Removes the unused limitFn in GroupByQuery, as noted here: #3873 (review)

@leventov
Copy link
Copy Markdown
Member

This field is assigned in constructor, and there is non-trivial logic to compute the value for it.

@leventov
Copy link
Copy Markdown
Member

@jon-wei this block and field that you removed in this PR were added just in #3873. Don't we add a bug or regression here? I couldn't answer this question because I don't know what #3873 is about and why this field was added.

@jon-wei
Copy link
Copy Markdown
Contributor Author

jon-wei commented Oct 10, 2017

Looks like that block was reintroduced by #3873 as part of a bad merge that restored some code that was removed by https://github.com/druid-io/druid/pull/4131/files#diff-7564ae1186b5d71c461ade89580058d3.

The limitFn and postProcFn it sets are not being used by anything so the block should be removed entirely.

The equivalent logic is now in GroupByQuery.makePostProcessingFn() and this block in GroupByStrategyV2:

    // Don't apply limit here for inner results, that will be pushed down to the BufferHashGrouper
    if (query.getContextBoolean(CTX_KEY_OUTERMOST, true)) {
      return query.postProcess(rowSequence);
    } else {
      return rowSequence;
    }

@gianm gianm merged commit 18635a1 into apache:master Oct 10, 2017
@jon-wei jon-wei added this to the 0.12.0 milestone Jan 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants