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

Reduce range filter dependence on ES syntax #10577

Closed
wants to merge 17 commits into from

Conversation

Bargs
Copy link
Contributor

@Bargs Bargs commented Feb 24, 2017

This is a small first step towards completely removing Elasticsearch syntax from our filter models. More than anything it's a proof of concept to validate my thinking on how to go about this.

My plan is to write (and enhance the existing) factory functions (and tests) for each of the filter types we support. The meta property of each filter will be extended to include all of the information necessary to actually "build" the ES query. For now I'll leave the actual query alone. I'll go about updating client code to use the new meta properties instead of inspecting the ES query directly (as I did with map_range in this PR). Once I've removed all the external dependencies on the query syntax I'll extract the query building logic from the filters and put it in separate serializers. I'll update the SearchSource code to use these new serializers when building the actual query to send to ES. Once the queries are gone from the filters we might be able to remove the meta key and put all those props at the root level.

This PR starts with the Range filter to show how I'll begin going about this process.

@Bargs
Copy link
Contributor Author

Bargs commented Feb 24, 2017

Somehow I overlooked the existing range filter tests, I'll have to reconcile those with the new tests on Monday.

@Bargs Bargs removed the request for review from lukasolson February 25, 2017 00:36
@Bargs
Copy link
Contributor Author

Bargs commented Mar 2, 2017

jenkins, test this

@Bargs Bargs force-pushed the rangeFilter branch 2 times, most recently from 2b0bed6 to ca875ac Compare April 5, 2017 21:23
@Bargs Bargs requested a review from lukasolson April 5, 2017 22:07
@Bargs
Copy link
Contributor Author

Bargs commented Apr 5, 2017

@lukasolson this is ready for a look again. I'd like to write some tests for the migration code but beyond that it should theoretically be in a state we could merge it in. If you think this approach looks good, let's chat about how to progress from here.

@Bargs Bargs added v5.5.0 and removed v5.4.0 labels Apr 5, 2017
@lukasolson lukasolson self-assigned this Apr 5, 2017
@lukasolson
Copy link
Member

So I'm thinking that we shouldn't distinguish between something that is a scripted field and something that isn't when we create the field. It should only be in the translation of that filter to an actual Elasticsearch query that we check the field, see if it's scripted, and modify the query accordingly. In other words, I think it makes sense to pull out all of this logic in buildRangeFilter. Thoughts @Bargs?

@Bargs
Copy link
Contributor Author

Bargs commented Apr 6, 2017

I agree all that should get ripped out, though I was planning on writing the serializers in a subsequent PR. Do you think it would be beneficial to see what that would look like in this PR?

As for the meta.type field, I remember debating it with myself but I can't remember what my reasoning was. For some reason I was thinking it might need to be script-range instead of just script or range. But... I think we can just make it range for now and see how that works.

@lukasolson
Copy link
Member

@Bargs I think that'd be good to explore in the context of this PR. As it currently stands, when you create a range filter on a scripted field, the syntax needed to query ES makes it into the URL. I think it'd be good to make a goal of this PR to replace that with our desired filter model. What do you think?

@Bargs
Copy link
Contributor Author

Bargs commented Apr 7, 2017

As it currently stands, when you create a range filter on a scripted field, the syntax needed to query ES makes it into the URL

That's true of regular fields as well. I'll play around with it though. I was afraid removing the ES syntax would turn this into a giant PR. If it doesn't, I agree it'll be useful to see what the full conversion will look like for one of the filter types in one PR.

@lukasolson
Copy link
Member

@Bargs Yeah, true. I guess when I read the title of this PR, to me it means that the result should be that we no longer use the ES syntax in how we store the filters.

That being said, I see value in what you've done so far, and it's definitely an incremental step that's worthwhile. If we want to limit this PR to that, maybe we could adjust the title?

@Bargs
Copy link
Contributor Author

Bargs commented Apr 7, 2017

Whoops, you're right, that's a bad title! I'll still try to do the serialization part here, I think it's at least worth an attempt. I'll hit you up when I've got an update

@lukasolson lukasolson removed their assignment Apr 11, 2017
@lukasolson
Copy link
Member

Another thought I had about this: The buildRangeFilter module should allow you to pass negate: true. You shouldn't have to go through the filterManager to get a negated filter.

@Bargs Bargs removed the request for review from lukasolson April 26, 2017 23:59
@Bargs
Copy link
Contributor Author

Bargs commented Apr 27, 2017

Putting this on hold for a bit while I exploring some other options for incrementally moving towards #10789

@Bargs
Copy link
Contributor Author

Bargs commented May 19, 2017

Probably not going to go ahead with this, closing for now

@Bargs Bargs closed this May 19, 2017
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.

3 participants