Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

All Filters should work with FilteredAggregators. #2711

Merged
merged 1 commit into from
Mar 23, 2016

Conversation

gianm
Copy link
Contributor

@gianm gianm commented Mar 23, 2016

This removes Filter.makeMatcher(ColumnSelectorFactory), and adds a
ValueMatcherFactory implementation to FilteredAggregatorFactory so it can
take advantage of existing makeMatcher(ValueMatcherFactory) implementations.

This also removes the Bound-based method from ValueMatcherFactory. Its only
user was the SpatialFilter, which could use the Predicate-based method.

Fixes #2604.

Has some overlap with #2690 but takes a different approach -- switching to
makeMatcher(ValueMatcherFactory) rather than implementing the missing
makeMatcher(ColumnSelectorFactory) methods.

This removes Filter.makeMatcher(ColumnSelectorFactory) and adds a
ValueMatcherFactory implementation to FilteredAggregatorFactory so it can
take advantage of existing makeMatcher(ValueMatcherFactory) implementations.

This patch also removes the Bound-based method from ValueMatcherFactory. Its
only user was the SpatialFilter, which could use the Predicate-based method.

Fixes apache#2604.
@gianm gianm added this to the 0.9.1 milestone Mar 23, 2016
@jon-wei
Copy link
Contributor

jon-wei commented Mar 23, 2016

👍

I'll update #2690 and remove the parts of the implementation that overlap with this PR

@fjy
Copy link
Contributor

fjy commented Mar 23, 2016

👍

@fjy fjy merged commit a5d5529 into apache:master Mar 23, 2016
@gianm gianm deleted the filtered-aggregator-impls branch March 24, 2016 00:56
@gauravkumar37
Copy link
Contributor

Thanks @gianm so much for this wonderful commit 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants