Skip to content
This repository has been archived by the owner on Jun 30, 2018. It is now read-only.

fuzzy query dsl support + term api improvement #289

Closed
wants to merge 5 commits into from
Closed

fuzzy query dsl support + term api improvement #289

wants to merge 5 commits into from

Conversation

plentz
Copy link
Contributor

@plentz plentz commented Mar 25, 2012

For newcomers(like me - but not just me), It's pretty hard to find out how to add support to fuzzy searchs. It would be nicer to have some api to easier archive querys with fuzzy(and maybe flt).

disclaimer: I know that Tire.search 'customers', :query => { :fuzzy => { :name => 'John' } } work, but it's pretty hard to include things like pagination, etc.

this is an initial implementation and could be improved. I also added a little improvement to the term api, which allow a option's hash parameter to be passed.

@plentz plentz mentioned this pull request Mar 25, 2012
@plentz
Copy link
Contributor Author

plentz commented Mar 25, 2012

ps: this pull request is still missing tests. I looked at the code but didn't find where the query's tests should live. There's text_query_test.rb, range_queries_test.rb, query_string_test.rb. Could someone point me if there's some kind of pattern for queries integration tests?

@karmi
Copy link
Owner

karmi commented Mar 25, 2012

Hi, thanks for the patches!

1/ Regarding the fuzzy query support, I'm on the fence if this should go the Tire core or contrib.

2/ The fuzzy query can, in fact, be quite easily be done with the "raw API", by passing a Hash, just as the Readme instructs. We could or maybe should support stuff like pagination better with the "raw API".

3/ Yes, every query type needs corresponding tests: unit tests to check the raw behaviour (proper encoding etc), and an integration test. The convention is to put unit tests in the test/unit/search_query_test.rb file, in a separate context, for the time being. I think we will split it into multiple files in the future. Every query type has a corresponding separate integration test file, in this case it would be called test/integration/fuzzy_queries_test.rb.

4/ The missing support for options looks like a bug. Could you add a unit/integration test demonstrating the bug and filing a separate issue?

@plentz
Copy link
Contributor Author

plentz commented Mar 25, 2012

  1. Since elasticsearch support it out-of-the-box, I think we should support it natively too, so, +1 for core.

  2. Fully agree.

  3. Thanks, I will update my pull request adding the corresponding tests.

  4. Actually it already support passing options, but I think it could be a better looking(and simpler) API. Today, we have to do something like this:

term :name, query

but, when you want to pass options, you have to changed it to:

term :name, { term: query, boost: 2 }

which I think it's confusing(even more for newcomers). After the change I've made, all you have to do is pass the options

term :name, query, boost: 2

@BlackdolphinMedia
Copy link

+1 for the core

@plentz
Copy link
Contributor Author

plentz commented Mar 25, 2012

well, I think that's it. unit + integration tests created.

@gregoriokusowski
Copy link

+1 for core

@fmeyer
Copy link

fmeyer commented Mar 26, 2012

👍 for core

@rafaelsteil
Copy link

+1 for core

3 similar comments
@cmilfont
Copy link

+1 for core

@peleteiro
Copy link

+1 for core

@andreazevedo
Copy link

+1 for core

karmi pushed a commit that referenced this pull request Mar 27, 2012
See <http://www.elasticsearch.org/guide/reference/query-dsl/fuzzy-query.html>

This is a combination of 4 commits:

* initial implementation of fuzzy api
* just a bit improvement to the docs to make fuzzy example easier to read
* unit tests for fuzzy query
* integration test for fuzzy query
karmi pushed a commit that referenced this pull request Mar 27, 2012
This is a combination of 3 commits:

* little improvement of term api
* fixing term tests after api changing
* unit tests for term query with an option hash
karmi added a commit that referenced this pull request Mar 27, 2012
karmi added a commit that referenced this pull request Mar 27, 2012
…test

This is what you probably want to use when exposing the search interface to your users.

See <http://www.elasticsearch.org/guide/reference/query-dsl/text-query.html>
karmi added a commit that referenced this pull request Mar 27, 2012
@karmi
Copy link
Owner

karmi commented Mar 27, 2012

@BlackdolphinMedia, @gregoriokusowski, @fmeyer, @rafaelsteil, @cmilfont, @peleteiro, @andreazevedo, I appreciate your interest in Tire! However this is not Rails, please add something meaningful to the discussion...

@karmi
Copy link
Owner

karmi commented Mar 27, 2012

@plentz Cleaned up and merged the code, thanks! For the time being it stays in core, the structure of code fo queries must be improved.

Notice, that for "fuzzy" queries, there's a standard Lucene operator for the "query_string" type. In majority of cases, it actually makes sense to use the "text" query with the fuzziness option, as @kimchy suggested on IRC.

You can see the example in the integration test, commit 137c981.

@karmi karmi closed this Mar 27, 2012
@karmi
Copy link
Owner

karmi commented Mar 27, 2012

@plentz, this is just a summary of what I did when splitting your commit plentz/tire@eb9992b, which combined different features/changes into one.

  # Get the changes and update from master via rebase
  git checkout -b issues/289
  git pull https://github.com/plentz/tire.git master
  git rebase master

  # Start interactive rebase of all commits not on master
  git rebase -i master

  # For eb9992b commit, mark it as "edit"
  # Now, reset the commit, but leave the changes on disk -- preparing the commit "split"
  git reset HEAD^
  # Add only the portions we want
  git add -p
  # Commit only the selected changes as a split commit
  git commit -v -c eb9992b
  # Repeat with the rest of the code, splitting the commit...
  # Continue the rebase...
  git rebase --continue
  # (We also squashed the commits)
  # Now let's check what we have prepared
  git log HEAD --oneline --not master
  # We can safely merge that into master

See these sources:

Evan-M pushed a commit to Evan-M/tire that referenced this pull request Apr 21, 2012
* upstream/master: (39 commits)
  Release 0.4.0
  [GEMS] Relaxed gem dependencies for "rest-client", "multi_json" and "bundler"
  Release 0.4.0.rc
  [karmi#218] Cleaned up the test suite for document_type in Index#bulk_store
  [karmi#218] Fixed the incorrect serialization of `document_type` in Index#bulk_store
  [FIX] Fixed displaying of Rake task usage
  [karmi#289] Update README and documentation, use the `prefix` query as the example of "unsupported" query
  [karmi#289] Added an example of fuzzy query in the Text query integration test
  [TEST] Improved code in the Text Query integration test
  [TEST] Improved code legibility in the Explanation integration test
  [TEST] Improved code legibility in the DSL integration test
  [karmi#289] Cleaned up the code for "term" and "fuzzy" queries and restructured the search query test suite
  [karmi#289] Improvements to the "term" query type
  [karmi#289] Added the "fuzzy" query type
  [GEMS] Updated and cleaned up gem dependencies
  Restructured the test suite for Ruby 1.8 unit test compatibility
  Added Ruby 1.8 compatibility for `Utils.escape/unescape`
  [TEST] Removed the "supermodel" gem and used the "redis-persistence" gem
  [ACTIVEMODEL] Fixed and cleaned up URL-escaping of document type
  [UTILS] Added the `Tire::Utils` module
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants