Skip to content

Filters: Add filter.toFilter method, use that instead of the instanceof chain in Filters.#2713

Merged
fjy merged 1 commit intoapache:masterfrom
gianm:filter-extensions
Mar 24, 2016
Merged

Filters: Add filter.toFilter method, use that instead of the instanceof chain in Filters.#2713
fjy merged 1 commit intoapache:masterfrom
gianm:filter-extensions

Conversation

@gianm
Copy link
Copy Markdown
Contributor

@gianm gianm commented Mar 23, 2016

I believe that the instanceof chain in Filters exists because in the past, Filter
and DimFilter were in different packages (DimFilter was in druid-client and Filter
was in druid-processing). And since druid-client didn't depend on druid-processing,
DimFilter couldn't have a toFilter method. But now it can.

@gianm gianm added this to the 0.9.1 milestone Mar 23, 2016
@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Mar 23, 2016

This should make it possible to do Filters as an extension, like #2613 wants to do.

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Mar 23, 2016

👍

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Mar 23, 2016

@gianm sound merge confclits with your other PR

@gianm gianm force-pushed the filter-extensions branch from fa0e12c to 2644160 Compare March 23, 2016 20:53
@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Mar 23, 2016

repaired conflicts

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.

Should we throw an exception instead if a DimFilter was null?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fair, changed.

@fjy fjy closed this Mar 23, 2016
@fjy fjy reopened this Mar 23, 2016
@jon-wei
Copy link
Copy Markdown
Contributor

jon-wei commented Mar 23, 2016

Had 1 comment/question on null DimFilter handling, but 👍 after travis aside from that

…of chain in Filters.

I believe that the instanceof chain in Filters exists because in the past, Filter
and DimFilter were in different packages (DimFilter was in druid-client and Filter
was in druid-processing). And since druid-client didn't depend on druid-processing,
DimFilter couldn't have a toFilter method. But now it can.
@gianm gianm force-pushed the filter-extensions branch from 2644160 to 7130620 Compare March 24, 2016 00:03
@navis
Copy link
Copy Markdown
Contributor

navis commented Mar 24, 2016

👍

@fjy fjy merged commit 8750f4b into apache:master Mar 24, 2016
@fjy fjy mentioned this pull request Mar 24, 2016
@gianm gianm deleted the filter-extensions branch March 27, 2016 16:13
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