Skip to content

Add new 'true' filter which always returns true.#5711

Merged
jon-wei merged 5 commits intoapache:masterfrom
scrawfor:add_true_filter
Jun 28, 2018
Merged

Add new 'true' filter which always returns true.#5711
jon-wei merged 5 commits intoapache:masterfrom
scrawfor:add_true_filter

Conversation

@scrawfor
Copy link
Copy Markdown
Contributor

Introduces a new "always true" filter instead of exposing the noop filter in PR #5597

@Override
public <T> T getBitmapResult(BitmapIndexSelector selector, BitmapResultFactory<T> bitmapResultFactory)
{
return null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, maybe this should return bitmapResultFactory.wrapAllTrue(Filters.allTrue(selector)); and return true for supportsBitmapIndex.

As-is, TrueFilter shouldn't be returning true for supportsSelectivityEstimation since it doesn't support bitmap indexes, but not supporting bitmap indexes would potentially interfere with use of bitmap indexes for other subfilters when an And/Or filter contains a TrueFilter (See makeCursors() in QueryableIndexStorageAdapter and getExecutionPlan() in AutoStrategy).

@jon-wei
Copy link
Copy Markdown
Contributor

jon-wei commented Jun 15, 2018

Hi @scrawfor, are you still interested in contributing this patch?

@scrawfor
Copy link
Copy Markdown
Contributor Author

@jon-wei Hey Jonathan, I'm still planning on it. Just haven't had the time yet. I'll try to loop back around next week.

public class DimFilterUtils
{
static final byte NOOP_CACHE_ID = -0x4;
static final byte TRUE_CACHE_ID = -0x5;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be added after EXPRESSION_CACHE_ID, not here, with value 0xF

@scrawfor
Copy link
Copy Markdown
Contributor Author

Latest version should fix both comments.

@scrawfor
Copy link
Copy Markdown
Contributor Author

Since this replaces the 'noop' filter, would you rather me remove that here, or open another PR?

@jon-wei
Copy link
Copy Markdown
Contributor

jon-wei commented Jun 22, 2018

@scrawfor Let's go ahead and remove that in this PR

@scrawfor
Copy link
Copy Markdown
Contributor Author

@jon-wei No-Op removed.

@jon-wei
Copy link
Copy Markdown
Contributor

jon-wei commented Jun 27, 2018

@leventov Did you have any more comments on this PR?

@leventov
Copy link
Copy Markdown
Member

@jon-wei I didn't intend to review this PR fully. My one comment is addressed.

@jon-wei jon-wei merged commit bf2a31a into apache:master Jun 28, 2018
@dclim dclim added this to the 0.13.0 milestone Oct 8, 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.

5 participants