[Xapian-devel] Re: [Xapian-commits] 9476: trunk/xapian-core/ trunk/xapian-core/include/xapian/ trunk/xapian-core/queryparser/ trunk/xapian-core/tests/

[Olly Betts]

On Fri, Oct 19, 2007 at 11:20:49AM +0100, Richard Boulton wrote:
> olly wrote:
> >Since calling QueryParser::add_prefix()
> >or QueryParser::add_boolean_prefix() a second time with the same
> >field name was ignored before (rather than overriding as we had
> >thought) it seems reasonable to change this behaviour.  This
> >also avoids the need to deprecate these methods which will force all
> >users to update their code.
> 
> I think leaving the old methods in place and undeprecated with these 
> semantics is fine, but I'd argue the the new, 3 argument, form of 
> add_prefix, and the prefix_type enum are good additions because they 
> lead to future improvements (although, they're possibly not needed yet).

The API as it is after this change is the original one you committed
(with one small semantic change).  You only changed it because I pointed
out that the behaviour it changed might actually be being used.  But I
had thought that setting the same field to a different prefix changed
it, whereas actually it's ignored, so the whole premise for changing it
was flawed.

I tried to bring this up on IRC, but got no response, so I went ahead
as it didn't seem controversial (it was your original API after all),
and SVN HEAD is currently much too far from being in a releasable state,
which I want to address.  Ideally we should be able to make a release at
short notice if we need to.

> 2. By taking the text after the field specifier, and processing it as a 
> single unit (currently, lowercase conversion only, end at space, no 
> phrases).

FWIW, it also ends at a close bracket to avoid swallowing them when used at
the end of a bracketted subexpression.

> [...]  I'm not too bothered about this 
> right now, but it does seem that add_boolean_prefix() will then become a 
> limited form of add_prefix() and would eventually be deprecated to clean 
> up the API, so adding a 3 argument form of add_prefix() now to allow 
> users to start using it instead of add_boolean_prefix() doesn't seem 
> unreasonable either.

I think it's better not to try to predict the API we'll want here before
we actually try to implement it.  We may get it wrong and force users to
update their add_prefix() calls multiple times.  Deprecating a method in
favour of another has a cost for everyone using it, so it's not
something we should do too lightly.

> The main concern I have is to check that we're still heading in the same 
> direction: making the query parser more configurable, and decoupling the 
> status of a prefix as an inline or a filter prefix from the way in which 
> term processing is done.

I'm not sure I'd use the word decouple, as there are essential
differences between the two (for example, it doesn't make sense to
have the empty field prefix being a filter, nor to have a phrase of
filters).  But certainly this should be much more configurable.

> However, I would say that changing from using an enum internally to 
> using a boolean value for the prefix type seems like a backwards step - 
> use of an enum makes the code more readable, in my opinion.

FWIW, I find it slightly more readable as currently used - the choice is
essentially:

    foo->filter

versus:

    foo->type == TYPE_FILTER

It also avoids the need to Assert(foo->type == TYPE_INLINE); after
dealing with the boolean filter case.

But the main reason I changed it was that I find "TYPE_INLINE" a rather
confusing name, and couldn't think of a better one.  To me, TYPE_INLINE
suggests a boolean filter inline in the query string (constrasted with
a boolean filter outside the query (e.g. in an HTML SELECT).

Cheers,
    Olly