couchbaselabs / cbft

*THIS PROJECT HAS MOVED* from couchbaselabs TO: https://github.com/couchbase/cbft -- no further development will be done here on couchbaselabs/cbft
Other
27 stars 5 forks source link

documentation confusing about "query" #151

Closed mschoch closed 9 years ago

mschoch commented 9 years ago

A user in IRC was confused by this section of the docs:

http://labs.couchbase.com/cbft/api-ref/#index-querying

In particular, they were trying to perform a query string style query, which necessitates a 3rd level nesting of the name "query". The docs just imply "query goes here", without any further guidance on what that might be, or that possibly a 3rd level of "query" is required.

mschoch commented 9 years ago

This same section of the doc shows size: 0 which is also confusing as running this query doesn't return the actual hits.

mschoch commented 9 years ago

To illustrate the confusion, the user first didn't get that there was even a 3rd level query. Next after explaining this, he moved all the properties to the 3rd level, which was also wrong...

mschoch commented 9 years ago

I'm actually thinking now that the top 2 levels should probably become one.

In bleve the distinction we make is between "what we want" and "how we want it". We call the "how we want it" the request which is the outer-most section. Then we embed inside that the "what we want" which we call the query. Then, somewhat confusingly we have a query type called "query string" which uses the field name "query" to encode the query (this was copied from elasticsearch).

In cbft, there seems to be the outer-request with cbft-level "how" things. Then there is a section called query, which is really the embedded bleve request.

I think this is sort of an implementation code-reuse leaking detail. We shouldn't let code-reuse affect the clarity of the API.

steveyen commented 9 years ago

Makes sense. On the cbft "how" things, it has fields called "timeout" and "consistency", so as long as those don't conflict with any bleve (or any other index-type specific) fields, it should be no problem.

One more thought, perhaps in the future add yet another REST endpoint, like queryString, that's more convenient to use. Some index-backends might not implement it, though.

mschoch commented 9 years ago

Speaking of which, when I was inspecting the request sent by the web page, I saw another top-level element "q" which seemed to be a copy of the query string. Is that just an artifact of the javascript that is ignored by the system?

steveyen commented 9 years ago

I recall now that the 'q' field was from the unfinished days when I was trying to get "views lite" to work. So, some index-backends actually might use that raw q string.

steveyen commented 9 years ago

Latest proposal:

The main JSON structure becomes the index backend's JSON structure (like bleve's SearchRequest struct) and there's just a single additional field, control or "ctl". The ctl field would contain all the cbft-related fields like timeout, consistency, and any future cross-index-backend cbft parameters (like auth, debug, resource hints, etc).

This would reduce the namespace conflicts with index-backends to just the "ctl" field.

Example...

{
    "ctl": {
        "timeout": 1000,
        "consistency": null
       ...room for future cbft-related fields...
    },
    ... fields from bleve's SearchRequest struct ...
    ... (actually, fields that are dependent on the actual index-backend) ...
}

For bleve indexes, this removes the 3 level query/query/query down to 2 levels.

steveyen commented 9 years ago

Series of recent commits, most recently with commit 284e2a02 which incompatibly change the REST query API from a 3-level structure to a two level structure, per the previous comment on the "ctl" JSON proposal.

Per semver rules, the next expected release gets a bump from 0.1.0 to at least 1.0.0.

Docs have been updated, too, but even though I'm closing this bug, more doc improvements should continue.

mschoch commented 9 years ago

Steve, can you clarify when you expect this version bump to happen? The phrase "next expected release" has cause some confusion in the IRC channel.

As @vmx points out, it probably should remain < 1.0.0 until we do the first Couchbase integrated release. As the rules state, "Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable."

steveyen commented 9 years ago

Thanks much! Didn't realize that there was an exception for < 1.0.0 semver rules. The expected version bump will be now to 0.2.0 (instead of to 1.0.0) so that everyone will less likely think that this is ready for production yet.

And, we can pull the trigger on 0.2.0 at anytime (how 'bout today?), imho, unless there's other things I'm not realizing that we must or should have for 0.2.0.

commit 8950ec6 made the REST API docs change back down to "0.2.0"