Mailing List Archive

On Sep 6, 2007, at 4:07 PM, Matthew O'Connor wrote:

> Marvin, (resending to list)

Thanks, I definitely prefer to handle this sort of thing out in the
public forum. :)

> So if I have field "foo" and terms "bar" and "baz" then "foo:(bar
> baz)" is equivalent to "(foo:bar foo:baz)". The expression "foo:
> (bar baz)" formerly had no semantics, but my implementation seems
> like a reasonable interpretation.

I think this is a very nice improvement in terms of consistency.

I see that your patch also adds a second argument to the public API
of QueryParser->parse:


=head1 METHODS
@@ -373,12 +377,16 @@

=head2 parse

- my $query = $query_parser->parse( $query_string );
+ my $query = $query_parser->parse( $query_string, $fields );

Turn a query string into a Query object. Depending on the contents
of the
query string, the returned object could be any one of several
subclasses of
L<KinoSearch::Search::Query|KinoSearch::Search::Query>.

+The default fields to search are taken from the C<$fields>
arrayref. If this
+is not passed in then the default fields from the query parser
object are
+used.
+
=head1 COPYRIGHT


The underlying functionality is needed because of how your patch
works (kudos on its simplicity and elegance, BTW), but I think we
ought to leave that second argument private. It's not needed because
the constructor allows you to specify custom fields already and we're
just duplicating that functionality. If you really need to set up
custom fields, you can create multiple QueryParser objects -- they
aren't cumbersome.

Marvin Humphrey
Rectangular Research
http://www.rectangular.com/



_______________________________________________
KinoSearch mailing list
KinoSearch@rectangular.com
http://www.rectangular.com/mailman/listinfo/kinosearch

On Sep 6, 2007, at 4:33 PM, Marvin Humphrey wrote:

>
> On Sep 6, 2007, at 4:07 PM, Matthew O'Connor wrote:
>
>> Marvin, (resending to list)
>
> Thanks, I definitely prefer to handle this sort of thing out in the
> public forum. :)
>
>> So if I have field "foo" and terms "bar" and "baz" then "foo:(bar
>> baz)" is equivalent to "(foo:bar foo:baz)". The expression "foo:
>> (bar baz)" formerly had no semantics, but my implementation seems
>> like a reasonable interpretation.
>
> I think this is a very nice improvement in terms of consistency.

While testing this patch at Socialtext we ran into a few surprises,
which I am not sure are wrong. I originally intended that "foo:(bar
baz)" would be the same as "foo:bar foo:baz" regardless of what foo,
bar, and baz were. However, that's certainly not the case when "bar"
or "baz" end in a colon.

In the case of "foo:(bar: baz)" we have that being the same as
"bar:baz" and not "foo:bar: foo:baz" as I originally thought. I
think this is probably okay and that the way it works, even though I
didn't intend that, is probably less surprising on the whole. One
generally expects their "tag:" prefixes to have an effect and having
them not would be weird.

Does that make sense? And if so, do you agree?

> The underlying functionality is needed because of how your patch
> works (kudos on its simplicity and elegance, BTW), but I think we
> ought to leave that second argument private. It's not needed
> because the constructor allows you to specify custom fields already
> and we're just duplicating that functionality. If you really need
> to set up custom fields, you can create multiple QueryParser
> objects -- they aren't cumbersome.

I agree, leaving it private is probably for the best. I was just
being a dutiful documenter. I removed it from the patch and reattched.


-matthew