There is one funny side effect of the way max results and continuations work: Currently we make no attempt to deduplicate results from a 2i query. So, if item 1 is indexed on field f1 for values 1,2,3, and you do a range query from 1 to 3, you will get the same item 3 times. Now, if you specify max results as 2, you get the value twice, plus a continuation. If you query the continuation, you get nothing. Since we are now sorting the results, deduplicating becomes very cheap. I'm wondering if we should start doing it. On the other hand, if users rely on this property and use the number of duplicates for something, continuations might break that. Thoughts?

More on the duplicates being dropped behaviour: if return_terms is true the duplicates that we are dropping may contain information useful to the client. Or at least it's easier to imagine they might care about that, since some terms will sometimes be retrieved, sometimes lost.

I don't understand. Can you provide a test or some minimal set of data and a query that reproduce the condition, please?

https://gist.github.com/engelsanchez/5630275
While I was playing with this I noticed the above: a single entry indexed by 3 values (1,2,3). A range query from 1-3 will return 3 items. With return_terms, each item is tagged with one of (1,2,3). With max results = 2, you get two of those, but of course the continuation won't get that key again, so you don't get the third one.

I'm not saying this is bad or unintended. Just that it might interfere with what the users of return_terms might want to do with that. Maybe ask them directly if it matters?

It is bad and unintended. I really hadn't noticed it or thought of it. I'm not sure what the answer it, except to expose the start_key_incl option to the end user, even then it is a bit of a guess on their part.

OK. I think this is a straight up bug from considering the key and the index value in isolation. Let me work on it this morning, I have some ideas.

Pooshed a fix for the dropped-duplicate-key-after-continuation bug, (good catch BTW! Glad we didn't ship that.) Pooshed test for same to riak_test.

Would you say now is a good time to address your other comments (i.e. are you through with the majority of the review?)

I've verified the fix works on my example. Good work man! Simple, small and effective fix. Will keep banging on it to see what else I can find.

A couple more things: remember to look at my branch adding an EQC test for sms, which also contains a couple of proposed edge case fixes: https://github.com/basho/riak_kv/commits/eas-add-sms-eqc

And you will need to merge from master before the PR gets its green button again (there is a little conflict in riak_kv_app where capabilities are added). Please do that before the final 👍 so that I can go over the final version of the code.

I'm having trouble using max_results and return_terms together through the HTTP interface

curl http://localhost:10018/buckets/bucket/index/i1_int/1/4?return_terms=true&max_results=2
{"results":[{"1":"1"},{"2":"1"},{"2":"2"},{"3":"1"},{"3":"2"},{"4":"2"}]}

curl http://localhost:10018/buckets/bucket/index/i1_int/1/4?max_results=2&return_terms=true
{"keys":["1","1"],"continuation":"g2gCYQJtAAAAATE="}

Also, validation doesn't seem to kick in for the second parameter. Maybe I'm doing something stupid, but I haven't figured it out. I'll debug this more right after lunch.

This is why I hate changes on review. I tested this stuff in the past. Ok. I'll add some more tests to riak_test and see what is up. Thanks.

Huh. Works for me. I enquoted my url+query params thussly:

curl "http://localhost:8098/buckets/mybucket/index/field1_int/1/4?max_results=2&return_terms=true"
{"results":[{"1":"1"},{"2":"2"}],"continuation":"g2gCYQJtAAAAATI="}

and

curl "http://localhost:8098/buckets/mybucket/index/field1_int/1/4?return_terms=true&max_results=2"
{"results":[{"1":"1"},{"2":"2"}],"continuation":"g2gCYQJtAAAAATI="}

and

curl "http://localhost:8098/buckets/mybucket/index/field1_int/1/4?max_results=2&return_terms=bum"
Invalid "return_terms". "bum" is not a boolean

and

curl "http://localhost:8098/buckets/mybucket/index/field1_int/1/4?return_terms=false&max_results=hippo"
Invalid "max_results". "hippo" is not a positive integer

Are all your branches up-to-date? I'll push the latest, which merges master and your EQC test (thanks for that, great work!)

/me shrugs

Yep. I tried on the browser and it works fine too. Nothing to see here, move along, nevermind... Sorry

Heh, better safe, no worries.

Ok, I think we can call it a day. I believe all comments were addressed. New testing has not uncovered any new problems. Ship it!
👍 💃 ⛵

From the commit log:

@rdb Please check to see how much effort vs. value might be required
to address the TODO comment. If it's too much work now, then please
create an issue for fixing & removing that TODO item so that the matter
doesn't get lost.

Oh, and the bug fixed by commit 6934d65 is something that only pops up every few minutes, in my experience with the backend_eqc test. The namespace (bucket + key + index) is likely too big to hit a case where a bucket + key + index is written and then sometime later a fold_keys is attempted with a query on the exact same index.

So, whenever a developer plays with areas that would affect the code that this test exercises, it would be extremely wise to run the backend_eqc test for a much longer period of time than the usual "make test" does. See my comment on that commit for an example.

Awesome @slfritchie. Thanks for finding and fixing. I think I'll fix that test properly during this testing cycle (add all the new query types, make it use V2 queries etc.) Thanks again. It was on the list to update the test, but time is cruel.