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

Filters extraction function #76

Merged
merged 4 commits into from
Feb 14, 2018
Merged

Filters extraction function #76

merged 4 commits into from
Feb 14, 2018

Conversation

gaetano-guerriero
Copy link
Contributor

This adds support for filters of type extraction, using an extraction function.

Extraction functions are also supported with filters of compatible type (this works only with druid >= 0.9.1)

@magor
Copy link

magor commented Jul 26, 2017

Where can I vote for this to be merged? :)

_FILTERS_WITH_EXTR_FN = ('selector', 'regex', 'javascript', 'in', 'bound',
'extraction')

def __init__(self, extraction_function=None, **args):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the extraction_function argument be added to the constructor of the child filters like Bound, Dimension etc?

Copy link
Contributor Author

@gaetano-guerriero gaetano-guerriero Aug 8, 2017

Choose a reason for hiding this comment

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

Right, I've added extraction_function argument to Bound and Interval constructors.
Dimension and Javascript are just shortcuts for simple filters so I think extraction_function is not useful.

@magor
Copy link

magor commented Aug 24, 2017

bump; looks like it's ready, should I wait for this (any ETA?) or use my own patched version?

@dyangelo-grullon
Copy link

anyone home?

@gianm
Copy link
Member

gianm commented Oct 30, 2017

I think none of the druid core committers are actively using pydruid so that explains why it hasn't been committed yet. Is anyone here using this patch locally and/or has confirmed that it passes unit tests? If so I can commit it.

@Erbureth
Copy link

Erbureth commented Nov 8, 2017

Hi,

we have tested it and it looks OK - at least it doesn't break any passing tests in PyDruid test suite nor our own.

One small remark - we had to create our own extraction class so we could use the lookups in filters:

class RegisteredLookupExtraction(LookupExtraction):

    extraction_type = 'registeredLookup'

    def __init__(self, lookup, **kwargs):
        super(RegisteredLookupExtraction, self).__init__(**kwargs)
        self._lookup = lookup

    def build_lookup(self):
        return self._lookup

@gaetano-guerriero
Copy link
Contributor Author

I can confirm this is working right here.

@Erbureth I cannot understand, something like this is working for me:

topn(
            ...
            filter=Filter(
                 extraction_function=MapLookupExtraction({'iPhone': 'N9'}),
                 type='in', values=['N9'], dimension='device',
                 .....
             ....
)

@Erbureth
Copy link

@gaetano-guerriero Simple map lookups are insufficient for us, we have big dictionaries fed from Kafka topics, each registered under specific name and running with corresponding tasks on middle managers, brokers and historical nodes. This helper class is perfectly usable for our scenario and working well with your code.

@mistercrunch
Copy link
Member

@gaetano-guerriero I'm taking over maintaining this library. Mind rebasing this PR?

@gaetano-guerriero
Copy link
Contributor Author

Hi,
this is now rebased.

@ghulands
Copy link

@gaetano-guerriero Looks like you need to fix some code formatting.

@gaetano-guerriero
Copy link
Contributor Author

I guess this was failing in master too ? Indeed #115 did same fixes. Anyway I added a commit for flake8, in case you want to merge this PR first.

@mistercrunch
Copy link
Member

Still merge conflicts, mind rebasing again?

@gaetano-guerriero
Copy link
Contributor Author

rebased

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.

8 participants