Add support for 'fl' SOLR parameter #86

Merged
merged 12 commits into from Sep 14, 2011

Projects

None yet

2 participants

@gpascale
Contributor
gpascale commented Sep 6, 2011

(for backstory on this issue, see #78)

The 'fl' parameter is described on the SOLR wiki (http://wiki.apache.org/solr/CommonQueryParameters):
"This parameter can be used to specify a set of fields to return, limiting the amount of information in the response. When returning the results to the client, only fields in this list will be included."

My implementation does not support the full syntax described there, but I believe the important use cases are covered. My main motivation was to enable the "keys only" scenario, where only keys are returned and index documents are not fetched, but I support a generalized list of fields as well.

Valid values for fl are

  • 'id', which causes only the document id to be returned - the index document is not fetched.
  • '*' (default), which causes the full index document to be fetched.
  • 'x,y,...,z' where each element in the list is a field in the index document. Only those fields will be returned.
gpascale added some commits Aug 31, 2011
@gpascale gpascale Add support for 'fl' SOLR parameter. Valid values are
- 'id', which causes only the document id to be returned, '*' without fetching the index document.
- '*' (default), which causes the full index document to be fetched.
- 'x,y,...,z' where each element in the list is a field in the index document. Only those fields will be returned.
7093002
@gpascale gpascale fl parameter code cleanup 37baf64
@rzezeski
Contributor
rzezeski commented Sep 9, 2011

Greg,

So I took a good chunk of the day to review this. The good news is that, after merging master into the branch, it worked (you branched during the middle of a flurry of commits getting ready for 1.0). The not-so-bad news is I still think it needs some tweaks.

My biggest concern is the fact that in the case where fl=id the code is special cased and bypasses the application of the sort param. This means that something like sort=first_name&fl=id would break semantics for people coming from Solr proper (Granted, Riak's Solr is already a far cry from being fully Solr compatible as it is, but I figure there is no reason to add to that incompatibility if it can be avoided). In that case I think the best thing to do would be to return a 400 - cannot sort when fl=id.

The other issue is that the unique_key field can be changed in the schema, but defaults to id. The code should pull/match against the value of this field, not hard-code it to id.

Finally, there is basically copy/paste/modify of the xml/json rendering code. It would be nice to refactor that so that one function could be used but branch on the parts that you need to during to special case. For example in xml_response you could case around the sort and write a function for generating the doc bodies that matches against the special case.

All that said, I think with the above fixes this is a worthy patch. I'd like to see this in 1.0 if possible but time is definitely running short. If I have some time Monday I might try to fix it up but if you wanted to take a stab at it over the weekend that would be great.

@gpascale
Contributor

Thanks Ryan,

I think all of that makes sense. I didn't have any time this weekend, but let me see what I can do. I'm hitting the limits of my knowledge of Erlang and the Riak code so I'm not super confident I can pull off all the refactoring you'd like, but I think I can take care or your first two suggestions.

@gpascale gpascale Improvements to FL parameter code
- Return an error if a sort parameter is specified when fl = id.
- Use Schema's unique_key() method rather than hardcoding id
- Refactor code to be smaller and clearer
e3803c8
@gpascale
Contributor

Ryan,

I made the changes you've suggested. To make the code cleaner, I also refactored xml_response and json_response such that there are no more ids_only versions. Please let me know what you think.

@rzezeski
Contributor

Good stuff. I'll take a look at this tomorrow.

@rzezeski rzezeski commented on the diff Sep 13, 2011
src/riak_solr_searcher_wm.erl
@@ -59,11 +69,12 @@ malformed_request(Req, State) ->
try
{ok, QueryOps} = Client:parse_query(Schema, SQuery#squery.q),
{ok, FilterOps} = Client:parse_filter(Schema, SQuery#squery.filter),
- {false, Req, State#state{schema=Schema,
- squery=SQuery, query_ops=QueryOps, filter_ops=FilterOps,
- sort=wrq:get_qs_value("sort", "none", Req),
- wt=wrq:get_qs_value("wt", "standard", Req),
- presort=to_atom(string:to_lower(wrq:get_qs_value("presort", "score", Req)))}}
+ {false, Req, validate_state(State#state{schema=Schema,
+ squery=SQuery, query_ops=QueryOps, filter_ops=FilterOps,
+ sort=wrq:get_qs_value("sort", "none", Req),
+ wt=wrq:get_qs_value("wt", "standard", Req),
+ presort=to_atom(string:to_lower(wrq:get_qs_value("presort", "score", Req))),
+ fl=wrq:get_qs_value("fl", "*", Req)})}
catch _ : Error ->
@rzezeski
rzezeski Sep 13, 2011 Contributor

Not wild about how we are using exceptions here to determine a malformed request but this code was already here long ago. Just adding the comment in hopes that I'll remember in the future.

@rzezeski
Contributor

Greg, I've made some of my own updates to this patch. Since I can't push to your repo I added the rz-fl branch to the Basho repo. All you should have to do is fetch that, merge it into your branch and then push to your repo.

I think it's good to be merged but I might bring in another set of eyes and add some integration tests first.

@gpascale
Contributor

Ok - should be all merged up. Let me know if you need anything else from me

@rzezeski
Contributor

Greg, can you fetch/merge/push one more time.

@gpascale
Contributor

Done

@rzezeski rzezeski merged commit 44031bc into basho:master Sep 14, 2011
@rzezeski rzezeski referenced this pull request in basho/yokozuna May 3, 2013
Merged

Add Protobuf support to Yokozuna #38

@EriksRemess EriksRemess pushed a commit to geeklist/riak-js-geeklist that referenced this pull request Jan 7, 2014
@gpascale gpascale Add 'fl' to the list of http parameters. This was added to riak_searc…
…h in my pull request basho/riak_search#86
00a351f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment