Top level filters #17

igor-alexandrov · 2014-04-22T08:57:28Z

No description provided.

pyromaniac · 2014-04-22T13:04:02Z

Would you continue to fix this?

igor-alexandrov · 2014-04-22T13:05:42Z

Of course! Can you provide me a configuration for ES that you are using to run rspecs on local machine?

Thanks!

pyromaniac · 2014-04-22T13:12:46Z

Just rake elasticsearch:start

igor-alexandrov · 2014-04-27T18:06:14Z

Done. Specs run clearly on local machine.

pyromaniac · 2014-04-28T01:21:46Z

lib/chewy/query/criteria.rb

@@ -136,3 +138,142 @@ def _request_types
    end
  end
 end
+
+# module Chewy
+#   class Query


What is it? Could you remove it please?

Sorry, forget to do some cleanup.

pyromaniac · 2014-04-28T01:25:58Z

As I told previously, I'm against this changes just because It uses stupid API rules. But, I've got a solution. If you will do it as an option, like Chewy.top_level_filters = true - it would be awesome, or use your patch for the first tim and I'll do this earlier or later.

igor-alexandrov · 2014-04-28T04:43:44Z

What do you mean in stupid API rules? Filter and queries are completely not the same and producing filtered query instead of separate query and filter is completely wrong. If ES API has separate query and filter options, then we should use them. Also, I gave you and example with facets , they are completely not usable in Chewy now, because when I apply facets value as a filter, I cannot run facets again.

pyromaniac · 2014-04-28T05:06:26Z

lib/chewy/query/criteria.rb

@@ -85,8 +85,10 @@ def merge other
      end

      def request_body
-        body = (_composed_query(_request_query, _request_filter) || {}).tap do |body|


You have removed _composed_query usage, why didn't remove the definition?

Because it is used in HasRelation queries.

So why did you remove specs for it?
And according to has_child/parent filters - is there any way to use filter and query together without composing them? Here in docs http://www.elasticsearch.org/guide/en/elasticsearch/reference/current/query-dsl-has-parent-filter.html said: 'The has_parent filter also accepts a filter instead of a query' Is it true, have you tried?

Putted back specs for _composed_query. I removed them when tried to get off from this method, but then understood that it is used in HasRelation.

I will look deeply at has_child and has_parent and try to find any possibilities for optimisations.

pyromaniac · 2014-04-28T05:18:14Z

I'm just asking you to make a kind of compatibility mode, Let root filter be the default, but make an option please.

igor-alexandrov · 2014-04-28T05:56:06Z

Give me an hour, I will add configuration option.

igor-alexandrov · 2014-04-28T07:40:51Z

Done. Added Chewy.filtered_queries option which is false by default.

pyromaniac · 2014-04-28T08:06:08Z

lib/chewy/query/compose.rb

@@ -56,4 +56,4 @@ def _filters_join filters, logic
      end
    end
  end
-end
+end


Why are you doing this? :)

pyromaniac · 2014-04-28T08:06:31Z

Awesome, just add some specs and I'll merge this

igor-alexandrov · 2014-04-28T20:08:44Z

Done. Added basic specs for option.

pyromaniac · 2014-04-29T00:24:31Z

spec/chewy/query/criteria_spec.rb

@@ -156,6 +156,20 @@ def request_body &block
    specify { request_body {
      update_options(from: 10); update_sort(:field); update_fields(:field); update_queries(:query)
    }.should == {body: {query: :query, from: 10, sort: [:field], _source: ['field']}} }
+
+    context do
+      before { Chewy.filtered_queries = false }


Random specs will fail sometime, use Chewy.stub(:filtered_queries, false)

igor-alexandrov · 2014-04-29T05:15:19Z

Done. Also updated config_spec.

Top level filters

pyromaniac · 2014-04-29T05:25:26Z

Awesome work, thanks!

igor-alexandrov · 2014-04-29T05:34:21Z

No problem, waiting for new gem version.

pyromaniac · 2014-04-29T05:57:22Z

Little bit later, use github version for a while

pyromaniac · 2014-06-03T12:30:15Z

Hey! I've decided to dig this problem deeper: elastic/elasticsearch#6367 (comment)

Waiting for https://groups.google.com/forum/#!topic/elasticsearch/Y9mLM4r7a3I answers

pyromaniac · 2014-06-03T12:35:52Z

Since Chewy is for ES >= 1.0 - I'll revert your PR and will implement post-filter as additional query chain method

igor-alexandrov · 2014-06-03T12:37:25Z

I will do this tonight, no problem.

igor-alexandrov · 2014-06-03T12:37:55Z

I didn't know about post-filter.

igor-alexandrov · 2014-06-03T12:39:14Z

Basically we should rename filter to post-filter, revert filter method to your implementation and remove filtered_queries option. Yes?

pyromaniac · 2014-06-03T12:44:57Z

Nope, you need to revert your changes and just add additional post_filter method working the same way as filter method, but result should be placed in post_filter section of body. We can even do additional post_filter_mode method for post-filter merge method setup.

pyromaniac · 2014-06-03T12:46:53Z

In this case there will no be incompatible changes in next release. Thanks Buddha I didn't releas this yet ;)

igor-alexandrov · 2014-06-03T12:47:16Z

Ok, then better revert my changes and I will do new PR tonight.

pyromaniac · 2014-06-03T12:48:42Z

You can do it in new PR, I still have no answers for other questions, so release will be after next weekend I suppose

igor-alexandrov · 2014-06-03T12:49:50Z

Ok, no problem. My vacation starts on Tuesday, so better for me to do this changes ASAP.

pyromaniac · 2014-06-04T14:34:23Z

Hey, just noticing you, I've reverted your code partially (filtered queries are now the only option). The only thing left - is to add post_filter to query. If you will not do this until this weekend, I'll do this on the weekend.

igor-alexandrov · 2014-06-05T13:30:41Z

Ok, where is code with reverted PR?

pyromaniac · 2014-06-05T13:33:41Z

In master already, it was reverted partially and manually :)

igor-alexandrov · 2014-06-05T13:36:38Z

Ok, I will do the rest.

igor-alexandrov · 2014-06-08T16:00:31Z

I regret, but it seems to me that I will not have enough time to do this tonight (having some production issues). Maybe tomorrow.

pyromaniac · 2014-06-09T00:03:41Z

It's done already, check it out in master please. Anyway, thanks for pointing me on this problem.

igor-alexandrov added 2 commits April 22, 2014 12:54

Fixed issue toptal#16. Add filter as a top level option.

92e304e

Reverted spec_helper

d185054

igor-alexandrov mentioned this pull request Apr 22, 2014

Filters as a top level query option #16

Closed

Fixed specs for Issue toptal#17.

d3e39f7

pyromaniac reviewed Apr 28, 2014
View reviewed changes

Removed garbage

98c4c60

pyromaniac reviewed Apr 28, 2014
View reviewed changes

Putted back _composed_query spec

0fcb0de

Added Chewy.filtered_queries option for compatibility

8df8125

pyromaniac reviewed Apr 28, 2014
View reviewed changes

Added basic spec for filtered_queries? configuration option

065808f

pyromaniac reviewed Apr 29, 2014
View reviewed changes

Updated specs

8254f53

pyromaniac added a commit that referenced this pull request Apr 29, 2014

Merge pull request #17 from igor-alexandrov/top_level_filters

e6db5b1

Top level filters

pyromaniac merged commit e6db5b1 into toptal:master Apr 29, 2014

@@ @@ -56,4 +56,4 @@ def _filters_join filters, logic @@
                     end
                   end
                 end
-              end
+              end

Top level filters #17

Top level filters #17

Conversation

igor-alexandrov commented Apr 22, 2014

pyromaniac commented Apr 22, 2014

igor-alexandrov commented Apr 22, 2014

pyromaniac commented Apr 22, 2014

igor-alexandrov commented Apr 27, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyromaniac commented Apr 28, 2014

igor-alexandrov commented Apr 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pyromaniac commented Apr 28, 2014

igor-alexandrov commented Apr 28, 2014

igor-alexandrov commented Apr 28, 2014

Choose a reason for hiding this comment

pyromaniac commented Apr 28, 2014

igor-alexandrov commented Apr 28, 2014

Choose a reason for hiding this comment

igor-alexandrov commented Apr 29, 2014

pyromaniac commented Apr 29, 2014

igor-alexandrov commented Apr 29, 2014

pyromaniac commented Apr 29, 2014

pyromaniac commented Jun 3, 2014

pyromaniac commented Jun 3, 2014

igor-alexandrov commented Jun 3, 2014

igor-alexandrov commented Jun 3, 2014

igor-alexandrov commented Jun 3, 2014

pyromaniac commented Jun 3, 2014

pyromaniac commented Jun 3, 2014

igor-alexandrov commented Jun 3, 2014

pyromaniac commented Jun 3, 2014

igor-alexandrov commented Jun 3, 2014

pyromaniac commented Jun 4, 2014

igor-alexandrov commented Jun 5, 2014

pyromaniac commented Jun 5, 2014

igor-alexandrov commented Jun 5, 2014

igor-alexandrov commented Jun 8, 2014

pyromaniac commented Jun 9, 2014