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

add prototype for mango catch-all feature #27

Closed
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@robertkowalski
Copy link
Contributor

robertkowalski commented Jan 4, 2016

Hey,

this is a prototype which is not fully implemented yet.

I want to find out if there is general interest and if it would be worth to spend more time.

The problem I am trying to solve is that I usually have a hard time explaining people how views work. Now we got Mango and I can just say: we use a syntax similar to MongoDB's query language but you have to create an index before you can use it.

At this point I usually look into sad, big eyes because no one understands why they have to create an index first and I feel there is another entry barrier for newcomers.

The idea of this patch is to just fallback on the "give me all docs and i filter afterwards"-trick that people usually use (if they know it) when they just want to test something, without creating an index which can take time for creation and requires further knowledge. Additionally the user is warned that they can create an index to make the queries faster.

@kxepal

This comment has been minimized.

Copy link
Member

kxepal commented Jan 4, 2016

+1 to address this issue. May only log at some higher level, like warning.

@tonysun83

This comment has been minimized.

Copy link
Contributor

tonysun83 commented Jan 4, 2016

I agree that this definitely needs to be addressed since it's been the biggest headache for must users. As for implementation, we don't necessarily need to add in the selector, I think we can just pass in the all_docs index to the cursor.


case length(UsableIndexes) of
0 ->
"{\"_warning\":\"no matching index found, create an index to optimize query time\",\r\n\"docs\":[";

This comment has been minimized.

@tonysun83

tonysun83 Jan 19, 2016

Contributor

If we're adding the "warning" field in the response, then I agree with other members this should be a URL.

This comment has been minimized.

@kxepal

kxepal Jan 19, 2016

Member

Also, may be just "warning", not prefixed one. We don't add prefix for "error" field returned by _all_docs view when key is not found.

This comment has been minimized.

@tonysun83

tonysun83 Jan 21, 2016

Contributor

Currently, we don't have any documentation regarding index creation in the Couchdb docs. I'm okay with this warning message as a string for this PR, but once we have proper documentation, we should change the message to a URL.

@tonysun83

This comment has been minimized.

Copy link
Contributor

tonysun83 commented Jan 21, 2016

Another thing that must be considered is how we deal with "use_index". That logic must still use error messages such as "no index found".

@robertkowalski

This comment has been minimized.

Copy link
Contributor

robertkowalski commented Jan 21, 2016

@tonysun83 what do you mean with "use_index"?

@tonysun83

This comment has been minimized.

Copy link
Contributor

tonysun83 commented Jan 25, 2016

In the _find api, you can specify an index to use such as:

{
  "selector": {
    "$text": "Pacino",
    "year": 2010
  },
  "use_index": "_design/32372935e14bed00cc6db4fc9efca0f1537d34a8"
}

So in this case, they forcefully specified an index. If the index is not found, you shouldn't try to use the _all_docs index, but instead through "no index found".

@robertkowalski robertkowalski force-pushed the robertkowalski:mango-catch-all branch from 854a0b5 to 021101a Jan 27, 2016

@robertkowalski robertkowalski force-pushed the robertkowalski:mango-catch-all branch from 021101a to 28234be Jan 27, 2016

{0, 0} ->
AllDocs = mango_idx:special(Db),
io:format("Index ~p~n", [AllDocs]),
create_cursor(Db, AllDocs, Selector, Opts);

This comment has been minimized.

@robertkowalski

robertkowalski Jan 27, 2016

Contributor

@tonysun83 I tried to follow your suggestion to pass the AllDocs index to avoid adding an additional selector. Given I obtain the All Docs index (the special index) and pass it my app crashes as columns for the special index is hardcoded to _id and no other fields are allowed: https://github.com/apache/couchdb-mango/blob/master/src/mango_idx_special.erl#L62-L63 which leaves my without a defined range in https://github.com/apache/couchdb-mango/blob/master/src/mango_cursor_view.erl#L33

2016-01-27 15:26:09.833 [error] node1@127.0.0.1 <0.3711.0> req_err(3969470405) unknown_error : function_clause
    [<<"mango_idx_special:start_key/1 L72">>,<<"mango_cursor_view:execute/3 L83">>,<<"mango_httpd:handle_find_req/2 L185">>,<<"mango_httpd:handle_req/2 L37">>,<<"chttpd:process_request/1 L290">>,<<"chttpd:handle_request_int/1 L227">>,<<"mochiweb_http:headers/6 L122">>,<<"proc_lib:init_p_do_apply/3 L237">>]

This comment has been minimized.

@tonysun83

tonysun83 Jan 27, 2016

Contributor

@robertkowalski:

So this is an interesting case because we need to manually construct the range. In your previous commit, when you do

{[{<<"$and">>, [Selector, {[{<<"_id">>, {[{<<"$gt">>, null}]}}]}]}]}

that's essentially doing the same manual range construction, but we are modifying the selector.

In order to do it without modifying the selector you can:

  1. Add some case statements in here and manually set the range in the end.
    https://github.com/apache/couchdb-mango/blob/master/src/mango_cursor_view.erl#L31-L49
    (I don't like this because it breaks the abstraction layer).

  2. Create a new module mango_cursor_special, with a new create function that specifically creates "all_docs" cursor with the range set. However, you'd have to modify these lines to incorporate it:

https://github.com/apache/couchdb-mango/blob/master/src/mango_idx.erl#L247-L248
https://github.com/apache/couchdb-mango/blob/master/src/mango_cursor.erl#L127-L130

At execution time, it will now call mango_cursor_special:execute, but I inside there, you can just defer the call to mango_cursor_view:execute.
I like this because it's cleaner and more modularized.

Let me know if that suggestion helps.

robertkowalski added some commits Feb 5, 2016

@@ -239,7 +239,7 @@ def test_empty(self):
try:
self.db.find({})
except Exception, e:
assert e.response.status_code == 400
assert e.response.status_code == 200

This comment has been minimized.

@tonysun83

tonysun83 Feb 5, 2016

Contributor

Since it's an all_docs request, I'd like to see we get all the results back. I think there's 20 rows for that particular set of tests so just assert that.

Also, it would be good to have a use_index test to ensure we still get that error back when they specify use_index, here's an example:

https://github.com/apache/couchdb-mango/blob/master/test/05-index-selection-test.py#L60

mango_cursor_view:execute(Cursor0, UserFun, UserAcc).

handle_message(Msg, Cursor) ->
mango_cursor_view:mango_cursor_view(Msg, Cursor).

This comment has been minimized.

@tonysun83

tonysun83 Feb 5, 2016

Contributor

typo? shouldn't this be mango_cursor_view:handle_message?

robertkowalski added some commits Feb 8, 2016

@robertkowalski robertkowalski force-pushed the robertkowalski:mango-catch-all branch from 3f10a60 to bf81dc5 Feb 8, 2016

@robertkowalski

This comment has been minimized.

Copy link
Contributor

robertkowalski commented Feb 8, 2016

@tonysun83 cool thanks for reviewing! i made the suggested changes. good idea with the additional test

@tonysun83

This comment has been minimized.

Copy link
Contributor

tonysun83 commented Feb 9, 2016

+1 for me, but would be comfortable if someone else who's okay with the warning message in the rows would also give it a +1.

@kxepal

This comment has been minimized.

Copy link
Member

kxepal commented Feb 9, 2016

+1 after squash, but still warning, not _warning as we use error instead of _error - no reason for the _ prefix here.

@robertkowalski

This comment has been minimized.

Copy link
Contributor

robertkowalski commented Feb 10, 2016

thanks will fix

asfgit pushed a commit that referenced this pull request Feb 10, 2016

add mango catch-all feature
fall back on the all_docs index if a index for the currently
searched field does not exist.

warn about slower execution and advise to create an index.

PR: #27
PR-URL: #27
Reviewed-By: Alexander Shorin
Reviewed-By: Tony Sun <tony.sun@cloudant.com>
@robertkowalski

This comment has been minimized.

Copy link
Contributor

robertkowalski commented Feb 10, 2016

yay thank oyu! merged as 01252f9

@robertkowalski

This comment has been minimized.

Copy link
Contributor

robertkowalski commented Feb 10, 2016

(with the fixed warning)

@robertkowalski robertkowalski deleted the robertkowalski:mango-catch-all branch Feb 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment