Don't strip punctuation inside quotes in history search queries.

Don't strip punctuation inside quotes in history search queries.

Currently, the query parser strips all punctuation from history search
queries, even inside quotes. A query like ["chromium.org"] will be passed
to SQLite as ["chromium org"], meaning that pages on chromium.org will
not be matched unless they contain the literal string "chromium org"
somewhere.

BUG=100545
TEST=Do a search on chrome://history for a domain that you have
previously visited, e.g. "chromium.org", and ensure that history
entries matching this domain are found.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121423

[Patrick Dubroy, 2012-02-06 12:03:20]

This is probably not a final solution, but I wanted to get the conversation
started with some code.

I'm thinking that we probably want to handle omnibox queries differently than
search queries on the chrome://history page. For omnibox queries, it may make
sense to continue to ignore all punctuation characters. For history search, we
may want to allow some punctation outside quotation marks, so that a query like
[google.com] is the same as ["google.com"].

What do you guys think?

[tfarina, 2012-02-06 16:39:14]

On 2012/02/06 12:03:20, dubroy wrote:
> This is probably not a final solution, but I wanted to get the conversation
> started with some code.
> 
> I'm thinking that we probably want to handle omnibox queries differently than
> search queries on the chrome://history page. For omnibox queries, it may make
> sense to continue to ignore all punctuation characters. For history search, we
> may want to allow some punctation outside quotation marks, so that a query
like
> [google.com] is the same as ["google.com"].
> 
> What do you guys think?

Scott should review this as well. If Peter is familiar with this code probably
his review is enough. Added Scott as reviewer and moved myself to CC list.

[sky, 2012-02-06 16:59:21]

I'm not intimately familiar with fts. Do we end stripping punctuation from the
content we add?

[brettw, 2012-02-06 17:43:38]

On Mon, Feb 6, 2012 at 8:59 AM,  <sky@chromium.org> wrote:
> I'm not intimately familiar with fts. Do we end stripping punctuation from
> the
> content we add?

I thought we do strip the puncutation when adding content. Are you
sure this works?

Brett

[Patrick Dubroy, 2012-02-06 18:02:38]

On 2012/02/06 17:43:38, brettw wrote:
> On Mon, Feb 6, 2012 at 8:59 AM,  <mailto:sky@chromium.org> wrote:
> > I'm not intimately familiar with fts. Do we end stripping punctuation from
> > the
> > content we add?
> 
> I thought we do strip the puncutation when adding content. Are you
> sure this works?

We don't appear to be stripping when adding content: http://goo.gl/6KwlS

From my understanding (based on reading the code, experimenting with search
terms, and reading http://www.sqlite.org/fts3.html#tokenizer), here's how it
works:

- full text content is stored in the database, indexed by "words", where the
meaning of a word is determined by the tokenizer.
- MATCH queries use the same tokenizer, so it's not necessary to strip
punctuation from the query

I've tested this patch on various queries and it seems to do the right thing. I
will also add some unit tests to make this more explicit.

[Peter Kasting, 2012-02-06 18:32:01]

Exchanged mrossetti for myself since he'll know more about whether the
autocomplete system uses the functions you're modifying.

shess should also look.

[mrossetti, 2012-02-07 01:05:57]

http://codereview.chromium.org/9316129/diff/1002/chrome/browser/history/query...
File chrome/browser/history/query_parser.cc (right):

http://codereview.chromium.org/9316129/diff/1002/chrome/browser/history/query...
chrome/browser/history/query_parser.cc:83: void AppendText(const string16& text)
{ word_ += text; }
We generally only inline simple getters and setters. This should not be inlined.

http://codereview.chromium.org/9316129/diff/1002/chrome/browser/history/query...
chrome/browser/history/query_parser.cc:385: } else if (iter.IsWord() ||
in_quotes) {
Okay, as you can see from the unit tests, trying to handle both an open quote
run AND just a regular word here (at line 385) causes the parser results for
string containing special characters to be different than with the current code.

I suggest only handling the |in_quotes| case on this branch, and handling the
unquoted words down below in those empty braces starting on line 394.

http://codereview.chromium.org/9316129/diff/1002/chrome/browser/history/query...
chrome/browser/history/query_parser.cc:390:
query_stack.back()->AddChild(current_word);
Do the set_literal here...

http://codereview.chromium.org/9316129/diff/1002/chrome/browser/history/query...
chrome/browser/history/query_parser.cc:392: if (in_quotes)
...not here.

http://codereview.chromium.org/9316129/diff/1002/chrome/browser/history/query...
chrome/browser/history/query_parser.cc:394: } else {  // Punctuation or
whitespace outside quotes.
These empty braces are just screaming for something! ;^) I believe if you change
this to:

    } else if (iter.IsWord()) {
      QueryNodeWord* word_node = new QueryNodeWord(iter.GetString());
      query_stack.back()->AddChild(word_node);
    }

it will restore the old, unquoted functionality and please the unit tests.

[Patrick Dubroy, 2012-02-08 22:06:41]

Can you guys please take another look? (Especially mrossetti, and shess if you
have any comments.)

After a chat with mrossetti, I've taken a slightly different approach with this:
- Inside quotes, special characters will be left is (previously, they were
stripped).
- Outside quotes, the characters [.-/~] will not be stripped if they occur after
a regular word. Meaning, URL-like strings (e.g. [google.com], [foo.bar/blah], do
not have to be quoted.

This change will mostly affect bookmark & history searches. It will also affect
HistoryContentsProvider, but that is already ignoring any omnibox queries that
are sufficiently URL-like, and otherwise it should improve the results.

[sky, 2012-02-08 23:31:28]

http://codereview.chromium.org/9316129/diff/11001/chrome/browser/history/quer...
File chrome/browser/history/query_parser.cc (right):

http://codereview.chromium.org/9316129/diff/11001/chrome/browser/history/quer...
chrome/browser/history/query_parser.cc:86: // A QueryNodeWord is a single word
in the query.
Update description.

http://codereview.chromium.org/9316129/diff/11001/chrome/browser/history/quer...
chrome/browser/history/query_parser.cc:104: virtual void Append(const string16&
word);
This isn't part of QueryNode, so move it some where else. Additionally does it
really need to be virtual?

[mrossetti, 2012-02-09 00:27:03]

http://codereview.chromium.org/9316129/diff/11001/chrome/browser/history/quer...
File chrome/browser/history/query_parser_unittest.cc (right):

http://codereview.chromium.org/9316129/diff/11001/chrome/browser/history/quer...
chrome/browser/history/query_parser_unittest.cc:86: EXPECT_EQ("test*",
QueryToString("--test"));
Excellent unit test additions!

May I suggest adding a test where there are escaped characters:
"query&path=foo%20bar" or somesuch, for example.

http://codereview.chromium.org/9316129/diff/11001/chrome/browser/history/text...
File chrome/browser/history/text_database_manager_unittest.cc (right):

http://codereview.chromium.org/9316129/diff/11001/chrome/browser/history/text...
chrome/browser/history/text_database_manager_unittest.cc:38: const char* kURL5 =
"http://www.google.com/uiop";
Again I might suggest adding an URL with at least one escaped character and
doing a search that matches.

http://codereview.chromium.org/9316129/diff/11001/chrome/browser/history/text...
chrome/browser/history/text_database_manager_unittest.cc:553:
manager.GetTextMatches(UTF8ToUTF16("FOO drei"), options,
Use ASCIIToUTF16 instead of UTF8ToUTF16 when you have a hard-coded constant
string like this. 4 places.

Note: For extra credit you could change the other 18 places where ASCIIToUTF16
should be used instead of UTF8ToUTF16 in the existing code.

[Patrick Dubroy, 2012-02-09 14:46:59]

Thanks for the comments, I've addressed them all. I've also changed what
characters are treated as non-breaking -- now it's only '.' and '-'.

One thing I learned in writing a few more unit tests: an unquoted query like
[foo.bar] will be treated by SQLite the same as the query [foo bar]. So this
patch still doesn't 100% address the problem that when someone types
[google.com] they probably want *only* matches from google.com.

There are a couple of possible solutions:

1. Automatically insert quotes around an URL-like query, i.e. [google.com foo]
is the same as ["google.com" foo]. The limitation of this is that quoted queries
can't be prefixes; i.e. "foo-bar" wouldn't match "foo-bars".

2. Allow clients of QueryParser to handle this themselves when appropriate. This
gets around the limitation above, but obviously pushes the complexity out a bit
further. BookmarkManager is already doing this using
QueryParser::DoesQueryMatch.

Thoughts?

I think this CL is worth landing as is, however, and I can deal with the exact
match issue in a later CL, but feel free to disagree.

http://codereview.chromium.org/9316129/diff/11001/chrome/browser/history/quer...
File chrome/browser/history/query_parser.cc (right):

http://codereview.chromium.org/9316129/diff/11001/chrome/browser/history/quer...
chrome/browser/history/query_parser.cc:86: // A QueryNodeWord is a single word
in the query.
On 2012/02/08 23:31:28, sky wrote:
> Update description.

Done.

http://codereview.chromium.org/9316129/diff/11001/chrome/browser/history/quer...
chrome/browser/history/query_parser.cc:104: virtual void Append(const string16&
word);
On 2012/02/08 23:31:28, sky wrote:
> This isn't part of QueryNode, so move it some where else. Additionally does it
> really need to be virtual?

Whoops, yeah. Done.

http://codereview.chromium.org/9316129/diff/11001/chrome/browser/history/quer...
File chrome/browser/history/query_parser_unittest.cc (right):

http://codereview.chromium.org/9316129/diff/11001/chrome/browser/history/quer...
chrome/browser/history/query_parser_unittest.cc:86: EXPECT_EQ("test*",
QueryToString("--test"));
On 2012/02/09 00:27:03, mrossetti wrote:
> Excellent unit test additions!
> 
> May I suggest adding a test where there are escaped characters:
> "query&path=foo%20bar" or somesuch, for example.

Good idea, done.

http://codereview.chromium.org/9316129/diff/11001/chrome/browser/history/text...
File chrome/browser/history/text_database_manager_unittest.cc (right):

http://codereview.chromium.org/9316129/diff/11001/chrome/browser/history/text...
chrome/browser/history/text_database_manager_unittest.cc:38: const char* kURL5 =
"http://www.google.com/uiop";
On 2012/02/09 00:27:03, mrossetti wrote:
> Again I might suggest adding an URL with at least one escaped character and
> doing a search that matches.

Done.

http://codereview.chromium.org/9316129/diff/11001/chrome/browser/history/text...
chrome/browser/history/text_database_manager_unittest.cc:553:
manager.GetTextMatches(UTF8ToUTF16("FOO drei"), options,
On 2012/02/09 00:27:03, mrossetti wrote:
> Use ASCIIToUTF16 instead of UTF8ToUTF16 when you have a hard-coded constant
> string like this. 4 places.
> 
> Note: For extra credit you could change the other 18 places where ASCIIToUTF16
> should be used instead of UTF8ToUTF16 in the existing code.

Done.

[mrossetti, 2012-02-09 17:27:19]

On 2012/02/09 14:46:59, dubroy wrote:
> Thanks for the comments, I've addressed them all. I've also changed what
> characters are treated as non-breaking -- now it's only '.' and '-'.

I didn't see that change anywhere.

[Scott Hess - ex-Googler, 2012-02-09 19:02:51]

On 2012/02/06 16:59:21, sky wrote:
> I'm not intimately familiar with fts. Do we end stripping punctuation from the
> content we add?

fts strips punctuation.  A match against "google.com" should do a phrase search
where "google" and "com" appear next to each other.  It should also match plain
old "google com" and "google!com", though it's unlikely that one would see other
combinations like that anywhere (other cases will surely differ).

[I'll go review the code now that I'm back in town.]

[Scott Hess - ex-Googler, 2012-02-09 19:12:22]

On 2012/02/09 14:46:59, dubroy wrote:
> Thanks for the comments, I've addressed them all. I've also changed what
> characters are treated as non-breaking -- now it's only '.' and '-'.
> 
> One thing I learned in writing a few more unit tests: an unquoted query like
> [foo.bar] will be treated by SQLite the same as the query [foo bar]. So this
> patch still doesn't 100% address the problem that when someone types
> [google.com] they probably want *only* matches from http://google.com.

fts aimed to be Google-like, in that just finding the match with all of your
stuff is often pretty reasonable, versus traditional search, which is very
focused on precision.

> There are a couple of possible solutions:
> 
> 1. Automatically insert quotes around an URL-like query, i.e. [google.com foo]
> is the same as ["google.com" foo]. The limitation of this is that quoted
queries
> can't be prefixes; i.e. "foo-bar" wouldn't match "foo-bars".

At the point where we start rewriting queries to fake out fts, it might be
reasonable to consider adding a custom tokenizer to do it, instead.  Then
google.com would just be a token.  Unfortunately, that might be a rathole,
because then you have to ask questions like whether google.com should match
mail.google.com.  In theory this could be handled by returning multiple tokens
for a position, but I've not personally tested whether that actually works.

[Scott Hess - ex-Googler, 2012-02-09 19:21:02]

Hmm, also, if you really want to restrict to google.com, you could
also do the query in two parts.  First pass finds all the "google com"
phrases, second pass finds the cases where the document actually
contains "google.com" using a LIKE.  Doing things like:

   SELECT rowid, document FROM fts_table WHERE fts_table MATCH
'"google com"' AND document LIKE "%google.com%"

may change the optimization logic, so be careful to test things.  If
SQLite does the LIKE before the MATCH, all is lost!  It could be
forced something like:

  SELECT rowid, document FROM fts_table WHERE rowid IN (SELECT rowid
FROM fts_table WHERE fts_table MATCH '"google com"') AND document LIKE
"%google.com%"

Of course, no magic to SQLite versus just filtering in your C++
code... overall, this approach really depends on the likelihood that
your MATCH filters aggressively versus the LIKE (filtering in C++
would make it easier to see whether LIKE was filtering out a lot of
data).

-scott




On Thu, Feb 9, 2012 at 11:12 AM,  <shess@chromium.org> wrote:
> On 2012/02/09 14:46:59, dubroy wrote:
>>
>> Thanks for the comments, I've addressed them all. I've also changed what
>> characters are treated as non-breaking -- now it's only '.' and '-'.
>
>
>> One thing I learned in writing a few more unit tests: an unquoted query
>> like
>> [foo.bar] will be treated by SQLite the same as the query [foo bar]. So
>> this
>> patch still doesn't 100% address the problem that when someone types
>> [google.com] they probably want *only* matches from http://google.com.
>
>
> fts aimed to be Google-like, in that just finding the match with all of your
> stuff is often pretty reasonable, versus traditional search, which is very
> focused on precision.
>
>
>> There are a couple of possible solutions:
>
>
>> 1. Automatically insert quotes around an URL-like query, i.e. [google.com
>> foo]
>> is the same as ["google.com" foo]. The limitation of this is that quoted
>
> queries
>>
>> can't be prefixes; i.e. "foo-bar" wouldn't match "foo-bars".
>
>
> At the point where we start rewriting queries to fake out fts, it might be
> reasonable to consider adding a custom tokenizer to do it, instead.  Then
> google.com would just be a token.  Unfortunately, that might be a rathole,
> because then you have to ask questions like whether google.com should match
> mail.google.com.  In theory this could be handled by returning multiple
> tokens
> for a position, but I've not personally tested whether that actually works.
>
>
> http://codereview.chromium.org/9316129/

[Patrick Dubroy, 2012-02-09 20:01:02]

On 2012/02/09 19:12:22, shess wrote:
> On 2012/02/09 14:46:59, dubroy wrote:
> > Thanks for the comments, I've addressed them all. I've also changed what
> > characters are treated as non-breaking -- now it's only '.' and '-'.
> > 
> > One thing I learned in writing a few more unit tests: an unquoted query like
> > [foo.bar] will be treated by SQLite the same as the query [foo bar]. So this
> > patch still doesn't 100% address the problem that when someone types
> > [google.com] they probably want *only* matches from http://google.com.
> 
> fts aimed to be Google-like, in that just finding the match with all of your
> stuff is often pretty reasonable, versus traditional search, which is very
> focused on precision.
> 
> > There are a couple of possible solutions:
> > 
> > 1. Automatically insert quotes around an URL-like query, i.e. [google.com
foo]
> > is the same as ["google.com" foo]. The limitation of this is that quoted
> queries
> > can't be prefixes; i.e. "foo-bar" wouldn't match "foo-bars".
> 
> At the point where we start rewriting queries to fake out fts, it might be
> reasonable to consider adding a custom tokenizer to do it, instead.  Then
> http://google.com would just be a token.  Unfortunately, that might be a
rathole,
> because then you have to ask questions like whether http://google.com should
match
> http://mail.google.com.  In theory this could be handled by returning multiple
tokens
> for a position, but I've not personally tested whether that actually works.

My understanding of FTS is that it's indexed based on the non-punctuation words,
but it still retains the original, un-tokenized contents. See one of the unit
tests that I added to TextDatabaseManager -- it searches for ["google.de"] and
finds a match in [www.google.de]. It seems like the query will be internally
tokenized, matched against the index, and then the entire query is matched
against the original text (including punctuation).

[Patrick Dubroy, 2012-02-09 20:04:06]

On 2012/02/09 19:21:02, shess wrote:
> Hmm, also, if you really want to restrict to http://google.com, you could
> also do the query in two parts.  First pass finds all the "google com"
> phrases, second pass finds the cases where the document actually
> contains "google.com" using a LIKE.  Doing things like:
> 
>    SELECT rowid, document FROM fts_table WHERE fts_table MATCH
> '"google com"' AND document LIKE "%google.com%"
> 
> may change the optimization logic, so be careful to test things.  If
> SQLite does the LIKE before the MATCH, all is lost!  It could be
> forced something like:
> 
>   SELECT rowid, document FROM fts_table WHERE rowid IN (SELECT rowid
> FROM fts_table WHERE fts_table MATCH '"google com"') AND document LIKE
> "%google.com%"
> 
> Of course, no magic to SQLite versus just filtering in your C++
> code... overall, this approach really depends on the likelihood that
> your MATCH filters aggressively versus the LIKE (filtering in C++
> would make it easier to see whether LIKE was filtering out a lot of
> data).
> 
> -scott

Yes, that's definitely another option. Any preference to how I should do it? I'm
not dead set on doing it in the QueryParser.

Currently the bookmarks manager is actually doing this kind of filtering in C++
code -- use the index to cut down the search space, then do a linear search
looking for exact matches. I happy to do it this way if you think it's a better
approach, but mrossetti was suggesting that keeping it in QueryParser would be a
good idea. 

> On Thu, Feb 9, 2012 at 11:12 AM,  <mailto:shess@chromium.org> wrote:
> > On 2012/02/09 14:46:59, dubroy wrote:
> >>
> >> Thanks for the comments, I've addressed them all. I've also changed what
> >> characters are treated as non-breaking -- now it's only '.' and '-'.
> >
> >
> >> One thing I learned in writing a few more unit tests: an unquoted query
> >> like
> >> [foo.bar] will be treated by SQLite the same as the query [foo bar]. So
> >> this
> >> patch still doesn't 100% address the problem that when someone types
> >> [google.com] they probably want *only* matches from http://google.com.
> >
> >
> > fts aimed to be Google-like, in that just finding the match with all of your
> > stuff is often pretty reasonable, versus traditional search, which is very
> > focused on precision.
> >
> >
> >> There are a couple of possible solutions:
> >
> >
> >> 1. Automatically insert quotes around an URL-like query, i.e. [google.com
> >> foo]
> >> is the same as ["google.com" foo]. The limitation of this is that quoted
> >
> > queries
> >>
> >> can't be prefixes; i.e. "foo-bar" wouldn't match "foo-bars".
> >
> >
> > At the point where we start rewriting queries to fake out fts, it might be
> > reasonable to consider adding a custom tokenizer to do it, instead.
&nbsp;Then
> > http://google.com would just be a token. &nbsp;Unfortunately, that might be
a rathole,
> > because then you have to ask questions like whether http://google.com should
match
> > http://mail.google.com. &nbsp;In theory this could be handled by returning
multiple
> > tokens
> > for a position, but I've not personally tested whether that actually works.
> >
> >
> > http://codereview.chromium.org/9316129/

[Patrick Dubroy, 2012-02-09 20:50:35]

Here's a new version that reflects my earlier comment: now only '.' and '-' are
treated as non-breaking characters. Also added some tests to double-check that
special characters aren't being treated like regular expresssions.

[mrossetti, 2012-02-09 21:01:22]

lgtm

[commit-bot: I haz the power, 2012-02-10 08:57:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dubroy@chromium.org/9316129/15004

[Patrick Dubroy, 2012-02-10 10:02:46]

On 2012/02/10 08:57:55, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/dubroy%40chromium.org/9316129/15004

sky & brettw: As OWNERS of chrome/browser/history, can one of you give an LGTM
on this?

[commit-bot: I haz the power, 2012-02-10 10:38:31]

Change committed as 121423

[Patrick Dubroy, 2012-02-10 12:03:00]

On 2012/02/10 10:38:31, I haz the power (commit-bot) wrote:
> Change committed as 121423

Oh. I clicked the "commit" checkbox before I checked the OWNERS file.

sky & brettw, if there's anything you'd like me to change/fix, let me know.

[sky, 2012-02-10 16:53:35]

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
File chrome/browser/history/query_parser.cc (right):

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
chrome/browser/history/query_parser.cc:154: void QueryNodeWord::Append(const
string16& str) {
Definition should match declaration order (this should be above
AppendToSQLiteQuery.

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
chrome/browser/history/query_parser.cc:390: if (IsQueryQuote(last_char)) {
Should we require the trailing quote character to match the starting quote
character?

[mrossetti, 2012-02-10 17:33:43]

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
File chrome/browser/history/query_parser.cc (right):

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
chrome/browser/history/query_parser.cc:390: if (IsQueryQuote(last_char)) {
I almost commented on that as well but the existing code doesn't take that into
account. :^/

On 2012/02/10 16:53:35, sky wrote:
> Should we require the trailing quote character to match the starting quote
> character?

[brettw, 2012-02-10 17:47:11]

Whoops, there was no set noparent on history so anybody can review. Otherwise,
commit-bot wouldn't have committed it. I'll fix this.

I'm going to defer my review to Scott Hess since he wrote FTS. It seems like he
still has some fundamental questions, and Sky has some too. Can we revert this
and continue the review?

[Scott Hess - ex-Googler, 2012-02-10 22:22:32]

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/text...
File chrome/browser/history/text_database_manager_unittest.cc (right):

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/text...
chrome/browser/history/text_database_manager_unittest.cc:590: EXPECT_EQ(0U,
results.size());
I don't understand how this one is working.  When I test against a raw fts3
table, MATCH '"Including: punctiotion."' gives the same results as MATCH
'"Including punctuation"'.  Is there post-query filtering which is different
because of this change?

[Scott Hess - ex-Googler, 2012-02-10 22:31:37]

On 2012/02/10 22:22:32, shess wrote:
>
http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/text...
> File chrome/browser/history/text_database_manager_unittest.cc (right):
> 
>
http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/text...
> chrome/browser/history/text_database_manager_unittest.cc:590: EXPECT_EQ(0U,
> results.size());
> I don't understand how this one is working.  When I test against a raw fts3
> table, MATCH '"Including: punctiotion."' gives the same results as MATCH
> '"Including punctuation"'.  Is there post-query filtering which is different
> because of this change?

[Mod speling errorrs.]

[Patrick Dubroy, 2012-02-11 10:58:38]

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/text...
File chrome/browser/history/text_database_manager_unittest.cc (right):

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/text...
chrome/browser/history/text_database_manager_unittest.cc:590: EXPECT_EQ(0U,
results.size());
On 2012/02/10 22:22:32, shess wrote:
> I don't understand how this one is working.  When I test against a raw fts3
> table, MATCH '"Including: punctiotion."' gives the same results as MATCH
> '"Including punctuation"'.  Is there post-query filtering which is different
> because of this change?

Weird. It doesn't look like there is any post-query filtering going on.
And when I test on my history index, here's the behaviour I see:

/usr/local/google/dubroy/chromium/src$ out/Debug/sqlite_shell
~/.config/chromium/Default/History\ Index\ 2012-02
SQLite version 3.7.6.3
Enter ".help" for instructions
Enter SQL statements terminated with a ";"
sqlite> select url from pages where pages match 'punctuation';
http://www.corp.google.com/~dubroy/no_crawl/fts_test.html
sqlite> select url from pages where pages match '"Including: punctuation"';
http://www.corp.google.com/~dubroy/no_crawl/fts_test.html
sqlite> select url from pages where pages match '"Including punctuation"';
sqlite>

So it seems consistent with unit test behaviour. Am I doing something different
than you did?

[Scott Hess - ex-Googler, 2012-02-11 17:09:54]

On 2012/02/09 20:01:02, dubroy wrote:
> My understanding of FTS is that it's indexed based on the non-punctuation
words,
> but it still retains the original, un-tokenized contents. See one of the unit
> tests that I added to TextDatabaseManager -- it searches for ["google.de"] and
> finds a match in [www.google.de]. It seems like the query will be internally
> tokenized, matched against the index, and then the entire query is matched
> against the original text (including punctuation).

Yes, except for the entire-query clause at the end.  At the fts level, the match
only happens on tokenized words.  AFAICT, our queries don't add anything that
would change this (the operation of the offsets function involves re-tokenizing
the document).

Hmm.  One thing which could differ is that I'm just using sqlite3 directly. 
Maybe the ICU tokenizer is doing something weird.  I'll go check that ...

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/text...
File chrome/browser/history/text_database_manager_unittest.cc (right):

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/text...
chrome/browser/history/text_database_manager_unittest.cc:590: EXPECT_EQ(0U,
results.size());
On 2012/02/11 10:58:39, dubroy wrote:
> On 2012/02/10 22:22:32, shess wrote:
> > I don't understand how this one is working.  When I test against a raw fts3
> > table, MATCH '"Including: punctiotion."' gives the same results as MATCH
> > '"Including punctuation"'.  Is there post-query filtering which is different
> > because of this change?
> 
> Weird. It doesn't look like there is any post-query filtering going on.
> And when I test on my history index, here's the behaviour I see:
> 
> /usr/local/google/dubroy/chromium/src$ out/Debug/sqlite_shell
> ~/.config/chromium/Default/History\ Index\ 2012-02
> SQLite version 3.7.6.3
> Enter ".help" for instructions
> Enter SQL statements terminated with a ";"
> sqlite> select url from pages where pages match 'punctuation';
> http://www.corp.google.com/%7Edubroy/no_crawl/fts_test.html
> sqlite> select url from pages where pages match '"Including: punctuation"';
> http://www.corp.google.com/%7Edubroy/no_crawl/fts_test.html
> sqlite> select url from pages where pages match '"Including punctuation"';
> sqlite>
> 
> So it seems consistent with unit test behaviour. Am I doing something
different
> than you did?

Here's what I mean:

sqlite> CREATE VIRTUAL TABLE t USING fts3(x);
sqlite> INSERT INTO t VALUES ("FOO page one. Including: punctuation.");
sqlite> SELECT * FROM t WHERE t MATCH 'punctuation';
FOO page one. Including: punctuation.
sqlite> SELECT * FROM t WHERE t MATCH '"Including: punctuation"';
FOO page one. Including: punctuation.
sqlite> SELECT * FROM t WHERE t MATCH '"Including punctuation"';
FOO page one. Including: punctuation.
sqlite> SELECT * FROM t WHERE t MATCH '"Including_ punctuation!"';
FOO page one. Including: punctuation.

[Scott Hess - ex-Googler, 2012-02-11 17:13:51]

On 2012/02/09 20:04:06, dubroy wrote:
> On 2012/02/09 19:21:02, shess wrote:
> > Hmm, also, if you really want to restrict to http://google.com, you could
> > also do the query in two parts.  First pass finds all the "google com"
> > phrases, second pass finds the cases where the document actually
> > contains "google.com" using a LIKE.  Doing things like:
> > 
> >    SELECT rowid, document FROM fts_table WHERE fts_table MATCH
> > '"google com"' AND document LIKE "%google.com%"
> > 
> > may change the optimization logic, so be careful to test things.  If
> > SQLite does the LIKE before the MATCH, all is lost!  It could be
> > forced something like:
> > 
> >   SELECT rowid, document FROM fts_table WHERE rowid IN (SELECT rowid
> > FROM fts_table WHERE fts_table MATCH '"google com"') AND document LIKE
> > "%google.com%"
> > 
> > Of course, no magic to SQLite versus just filtering in your C++
> > code... overall, this approach really depends on the likelihood that
> > your MATCH filters aggressively versus the LIKE (filtering in C++
> > would make it easier to see whether LIKE was filtering out a lot of
> > data).
> 
> Yes, that's definitely another option. Any preference to how I should do it?
I'm
> not dead set on doing it in the QueryParser.

Probably worth figuring out why the results are as they are with the straight
fts query before going in this direction.  As-is, things feel brittle to me,
like a future SQLite import might cause unexpected issues.

[Scott Hess - ex-Googler, 2012-02-11 17:29:04]

On 2012/02/11 17:09:54, shess wrote:
> Hmm.  One thing which could differ is that I'm just using sqlite3 directly. 
> Maybe the ICU tokenizer is doing something weird.  I'll go check that ...

OK, so I am seeing similar results to you with the ICU tokenizer we use.  The
docs for that indicate:

  The ICU tokenizer implementation is very simple. It splits the input
  text according to the ICU rules for finding word boundaries and discards
  any tokens that consist entirely of white-space.

Some experimenting with NEAR/0 and NEAR/1 makes me think that in tokenization,
it is doing something like:

  "including" ":" "punctuation"

then your phrase query is matching "including : punctuation" as a phrase of
words.  I find this pretty unexpected, but it might end up being consistent
enough to depend on.  I need to think about What Does It Mean for a little,
though.  It looks like /, ?, and = are also treated this way.

[Patrick Dubroy, 2012-02-11 18:17:28]

On 2012/02/11 17:29:04, shess wrote:
> On 2012/02/11 17:09:54, shess wrote:
> > Hmm.  One thing which could differ is that I'm just using sqlite3 directly. 
> > Maybe the ICU tokenizer is doing something weird.  I'll go check that ...
> 
> OK, so I am seeing similar results to you with the ICU tokenizer we use.  The
> docs for that indicate:
> 
>   The ICU tokenizer implementation is very simple. It splits the input
>   text according to the ICU rules for finding word boundaries and discards
>   any tokens that consist entirely of white-space.
> 
> Some experimenting with NEAR/0 and NEAR/1 makes me think that in tokenization,
> it is doing something like:
> 
>   "including" ":" "punctuation"

Aha, you're right. I see the same results when I search for '"including:
punctuation"' and '"including : punctuation"'.

> then your phrase query is matching "including : punctuation" as a phrase of
> words.  I find this pretty unexpected, but it might end up being consistent
> enough to depend on.  I need to think about What Does It Mean for a little,
> though.  It looks like /, ?, and = are also treated this way.

It still feels like this patch is an improvement on the previous behaviour. The
current code assumes that the tokenizer is stripping punctuation, which is not
true. If we pass in the query unmodified, we at least the same tokenizing that
is applied to the original content. What do you think?

If we do want a quoted query to force an exact, character-by-character match,
then maybe it should be handled in QueryParser::DoesQueryMatch or TextDatabase::
GetTextMatches.

[Scott Hess - ex-Googler, 2012-02-11 19:13:18]

On 2012/02/11 18:17:28, dubroy wrote:
> On 2012/02/11 17:29:04, shess wrote:
> > then your phrase query is matching "including : punctuation" as a phrase of
> > words.  I find this pretty unexpected, but it might end up being consistent
> > enough to depend on.  I need to think about What Does It Mean for a little,
> > though.  It looks like /, ?, and = are also treated this way.
> 
> It still feels like this patch is an improvement on the previous behaviour.
The
> current code assumes that the tokenizer is stripping punctuation, which is not
> true. If we pass in the query unmodified, we at least the same tokenizing that
> is applied to the original content. What do you think?

Yeah, that's what I'm thinking, too.  Why I want to think about it, though, is
that it might imply that we should specially treat more characters than just URL
parts.  I would guess that existing phrase queries probably are looking at plain
text, so probably don't need to be modified, but I don't know about "this, that"
or "this; that".

Also, at a deeper level, it makes me wonder whether we should change the
tokenizer to not behave this way (maybe drop all-punctuation "words").  That's
obviously not in-scope for your change, but if it seems like a reasonable future
change, then it might be reasonable to not set in place a behavior which would
break in the future.  I'll talk to Brett on Monday and we can clear this up.

> If we do want a quoted query to force an exact, character-by-character match,
> then maybe it should be handled in QueryParser::DoesQueryMatch or
TextDatabase::
> GetTextMatches.

At this point, I'm thinking it might just end up being a TODO() or NOTE(),
rather than code.  Like NOTE(someone): fts3's icu tokenizer blah blah (this
discovery), if the matching changes we could blah blah (do more here).  Just so
someone in the future doesn't have to reverse-engineer things if/when we change
the backend.

[Patrick Dubroy, 2012-02-14 17:19:02]

Any more thoughts on this?

On 2012/02/11 19:13:18, shess wrote:
> On 2012/02/11 18:17:28, dubroy wrote:
> > On 2012/02/11 17:29:04, shess wrote:
> > > then your phrase query is matching "including : punctuation" as a phrase
of
> > > words.  I find this pretty unexpected, but it might end up being
consistent
> > > enough to depend on.  I need to think about What Does It Mean for a
little,
> > > though.  It looks like /, ?, and = are also treated this way.
> > 
> > It still feels like this patch is an improvement on the previous behaviour.
> The
> > current code assumes that the tokenizer is stripping punctuation, which is
not
> > true. If we pass in the query unmodified, we at least the same tokenizing
that
> > is applied to the original content. What do you think?
> 
> Yeah, that's what I'm thinking, too.  Why I want to think about it, though, is
> that it might imply that we should specially treat more characters than just
URL
> parts.  I would guess that existing phrase queries probably are looking at
plain
> text, so probably don't need to be modified, but I don't know about "this,
that"
> or "this; that".
> 
> Also, at a deeper level, it makes me wonder whether we should change the
> tokenizer to not behave this way (maybe drop all-punctuation "words").  That's
> obviously not in-scope for your change, but if it seems like a reasonable
future
> change, then it might be reasonable to not set in place a behavior which would
> break in the future.  I'll talk to Brett on Monday and we can clear this up.
> 
> > If we do want a quoted query to force an exact, character-by-character
match,
> > then maybe it should be handled in QueryParser::DoesQueryMatch or
> TextDatabase::
> > GetTextMatches.
> 
> At this point, I'm thinking it might just end up being a TODO() or NOTE(),
> rather than code.  Like NOTE(someone): fts3's icu tokenizer blah blah (this
> discovery), if the matching changes we could blah blah (do more here).  Just
so
> someone in the future doesn't have to reverse-engineer things if/when we
change
> the backend.

[Scott Hess - ex-Googler, 2012-02-17 18:05:33]

Sorry about the delay - earlier in the week we noted a crasher which snuck into
M-17 and between that and random other interrupts ...

Given how punctuation is tokenized, I wonder if this needs to have more
characters when in quotes.  Using {} to avoid confusing quotes with quotes,
{google.com} will effectively query {google OR . OR com}, and will match tons. 
{"google.com"} will match {google} followed by {.} followed by {com}, which will
pretty much match what we're looking for.  BUT, I think that
{"google.com/images"} will match {google} followed by {.} followed by {com}
followed by {images}, which will NOT match {google . com / images} in the actual
document.

Did that make sense?

At the meta level, I talked to Brett, and it seems like we can reasonably just
assume the existing tokenization system.  Someday, if we change implementation,
only a relatively small number of people will be affected, and they'll mostly be
able to figure things out.  In fact, it occurs to me that if any future
implementation of the FTS backend simply dropped all punctuation-only words,
then some variant of your change would just continue to work.

Anyhow, I think things are reasonable, so I'm going to give the code another
pass, now... thanks for your patience.

[Patrick Dubroy, 2012-02-17 18:34:01]

On 2012/02/17 18:05:33, shess wrote:
> Sorry about the delay - earlier in the week we noted a crasher which snuck
into
> M-17 and between that and random other interrupts ...

No worries...I've got plenty of stuff to keep me busy. :-)

> Given how punctuation is tokenized, I wonder if this needs to have more
> characters when in quotes.  Using {} to avoid confusing quotes with quotes,
> {google.com} will effectively query {google OR . OR com}, and will match tons.

> {"google.com"} will match {google} followed by {.} followed by {com}, which
will
> pretty much match what we're looking for.  BUT, I think that
> {"google.com/images"} will match {google} followed by {.} followed by {com}
> followed by {images}, which will NOT match {google . com / images} in the
actual
> document.

Actually, this patch retains *all* punctuation characters when inside quotes.
It's only outside quotes that the concept of "non-breaking" characters applies.

Also, the FTS doc says that query terms have an implicit AND between them, so
{google.com} would be {google AND . AND com}. But you're right, that still might
match a lot more than the user really intended. Copy/pasting from one of my
earlier comments:
>>>
There are a couple of possible solutions:

1. Automatically insert quotes around an URL-like query, i.e. [google.com foo]
is the same as ["google.com" foo]. The limitation of this is that quoted queries
can't be prefixes; i.e. "foo-bar" wouldn't match "foo-bars".

2. Allow clients of QueryParser to handle this themselves when appropriate. This
gets around the limitation above, but obviously pushes the complexity out a bit
further. BookmarkManager is already doing this using
QueryParser::DoesQueryMatch.
<<<

I kind of on the fence, but I feel like #2 is really the best solution.

> Did that make sense?
> 
> At the meta level, I talked to Brett, and it seems like we can reasonably just
> assume the existing tokenization system.  Someday, if we change
implementation,
> only a relatively small number of people will be affected, and they'll mostly
be
> able to figure things out.  In fact, it occurs to me that if any future
> implementation of the FTS backend simply dropped all punctuation-only words,
> then some variant of your change would just continue to work.

Yeah, the current implementation is already broken for quoted queries because
it's assuming incorrect things about the tokenizer.

[Scott Hess - ex-Googler, 2012-02-17 22:34:58]

On 2012/02/17 18:34:01, dubroy wrote:
> On 2012/02/17 18:05:33, shess wrote:
> > Given how punctuation is tokenized, I wonder if this needs to have more
> > characters when in quotes.  Using {} to avoid confusing quotes with quotes,
> > {google.com} will effectively query {google OR . OR com}, and will match
tons.
> 
> > {"google.com"} will match {google} followed by {.} followed by {com}, which
> will
> > pretty much match what we're looking for.  BUT, I think that
> > {"google.com/images"} will match {google} followed by {.} followed by {com}
> > followed by {images}, which will NOT match {google . com / images} in the
> actual
> > document.
> 
> Actually, this patch retains *all* punctuation characters when inside quotes.
> It's only outside quotes that the concept of "non-breaking" characters
applies.

I need to come up to your level, rather than thinking entirely from the
underlying code :-).

> Also, the FTS doc says that query terms have an implicit AND between them, so
> {google.com} would be {google AND . AND com}. But you're right, that still
might
> match a lot more than the user really intended.

Sorry, that was a brain skip on my part.

> Copy/pasting from one of my earlier comments:
> >>>
> There are a couple of possible solutions:
> 
> 1. Automatically insert quotes around an URL-like query, i.e. [google.com foo]
> is the same as ["google.com" foo]. The limitation of this is that quoted
queries
> can't be prefixes; i.e. "foo-bar" wouldn't match "foo-bars".

Worrying about users who do prefix searches might be overboard.

> 2. Allow clients of QueryParser to handle this themselves when appropriate.
This
> gets around the limitation above, but obviously pushes the complexity out a
bit
> further. BookmarkManager is already doing this using
> QueryParser::DoesQueryMatch.
> <<<
> 
> I kind of on the fence, but I feel like #2 is really the best solution.

I think I'm starting to agree with your implementation, now that I understand
why things ended up where they did.  It seems reliable on a practical level.

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
File chrome/browser/history/query_parser.cc (right):

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
chrome/browser/history/query_parser.cc:379: query_stack.push_back(root);
I don't understand why a stack is needed to parse quoted text.  If !in_quotes,
put the string in a new node and append to root, if in_quotes put the string in
a new node and append to the last node of root (which is the phrase), and if
in_quotes and embedded punctuation, append the string to the last child node of
the last node in root.  But probably not for resolving today :-).

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
chrome/browser/history/query_parser.cc:389: wchar_t last_char =
query[iter.prev()];
I'm not sure "last_char" is the best description.  AFAICT this is the break
point between the previous token and this token, when used as an index it will
be the first character of the current token.  Reading this line sounds like it's
saying the last character of the previous token.  Maybe first_char?

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
chrome/browser/history/query_parser.cc:403: // whitespace character inside
quotes.
If it's not a word, is it always a single character?  I'd say "non-whitespace
token", just in case that's not a safe assumption.

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
chrome/browser/history/query_parser.cc:418:
current_word->Append(iter.GetString());
If I understand this right, I'm not sure I buy it.  This means that unquoted
{google.com} will be passed as {google.com}, which the tokenizer will convert to
{google AND . AND com}.  It may be an improvement in that it will filter out
documents which contain {google} and {com} but not {.}, but I'm not sure the
matches will really make sense, as they'll include things like "google
commercial.".  How important do you think prefix matching is in a case like
this?  I think converting it to match {"google.com"} would make some sense.

Mostly I'm thinking that describing out it works is odd.  Saying that it ignores
punctuation is easy, saying that it finds them in that order is easy, but saying
that it treats . like a word is weird (to non-engineer types).

[Patrick Dubroy, 2012-02-20 11:00:36]

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
File chrome/browser/history/query_parser.cc (right):

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
chrome/browser/history/query_parser.cc:154: void QueryNodeWord::Append(const
string16& str) {
On 2012/02/10 16:53:35, sky wrote:
> Definition should match declaration order (this should be above
> AppendToSQLiteQuery.

Done.

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
chrome/browser/history/query_parser.cc:379: query_stack.push_back(root);
I agree. I can fix this up in a follow-up CL if you'd like.

On 2012/02/17 22:34:59, shess wrote:
> I don't understand why a stack is needed to parse quoted text.  If !in_quotes,
> put the string in a new node and append to root, if in_quotes put the string
in
> a new node and append to the last node of root (which is the phrase), and if
> in_quotes and embedded punctuation, append the string to the last child node
of
> the last node in root.  But probably not for resolving today :-).

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
chrome/browser/history/query_parser.cc:389: wchar_t last_char =
query[iter.prev()];
On 2012/02/17 22:34:59, shess wrote:
> I'm not sure "last_char" is the best description.  AFAICT this is the break
> point between the previous token and this token, when used as an index it will
> be the first character of the current token.  Reading this line sounds like
it's
> saying the last character of the previous token.  Maybe first_char?

You're right. Not sure why I named it like that. Done.

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
chrome/browser/history/query_parser.cc:390: if (IsQueryQuote(last_char)) {
On 2012/02/10 17:33:43, mrossetti wrote:
> I almost commented on that as well but the existing code doesn't take that
into
> account. :^/
> 
> On 2012/02/10 16:53:35, sky wrote:
> > Should we require the trailing quote character to match the starting quote
> > character?
> 

I think we should. I can address this in a follow-up CL.

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
chrome/browser/history/query_parser.cc:403: // whitespace character inside
quotes.
On 2012/02/17 22:34:59, shess wrote:
> If it's not a word, is it always a single character?  I'd say "non-whitespace
> token", just in case that's not a safe assumption.

Done.

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
chrome/browser/history/query_parser.cc:418:
current_word->Append(iter.GetString());
On 2012/02/17 22:34:59, shess wrote:
> If I understand this right, I'm not sure I buy it.  This means that unquoted
> {google.com} will be passed as {google.com}, which the tokenizer will convert
to
> {google AND . AND com}.  It may be an improvement in that it will filter out
> documents which contain {google} and {com} but not {.}, but I'm not sure the
> matches will really make sense, as they'll include things like "google
> commercial.".  How important do you think prefix matching is in a case like
> this?  I think converting it to match {"google.com"} would make some sense.
> 
> Mostly I'm thinking that describing out it works is odd.  Saying that it
ignores
> punctuation is easy, saying that it finds them in that order is easy, but
saying
> that it treats . like a word is weird (to non-engineer types).

I agree. I don't think it's very common to include punctuation but not want to
do an exact match. I also think that the prefix matching for such strings is
also not that important. As long as [google.com] matches things like
"www.google.com" and "google.com/images", I think that's fine. I don't think
people would suffer if a search for [google.co] didn't match "google.com".

There's still the question of whether exact matching should be implemented by
quoting such strings, or by filtering them after the fact. I'm starting to think
that it should just be done here -- it's simpler and less error-prone, I think.

So, the question is: should any punctuation character not surrounded by
whitespace trigger this behaviour? Or only some subset of characters (e.g. those
that can appear in a URL).

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/text...
File chrome/browser/history/text_database_manager_unittest.cc (right):

http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/text...
chrome/browser/history/text_database_manager_unittest.cc:590: EXPECT_EQ(0U,
results.size());
On 2012/02/11 17:09:55, shess wrote:
> On 2012/02/11 10:58:39, dubroy wrote:
> > On 2012/02/10 22:22:32, shess wrote:
> > > I don't understand how this one is working.  When I test against a raw
fts3
> > > table, MATCH '"Including: punctiotion."' gives the same results as MATCH
> > > '"Including punctuation"'.  Is there post-query filtering which is
different
> > > because of this change?
> > 
> > Weird. It doesn't look like there is any post-query filtering going on.
> > And when I test on my history index, here's the behaviour I see:
> > 
> > /usr/local/google/dubroy/chromium/src$ out/Debug/sqlite_shell
> > ~/.config/chromium/Default/History\ Index\ 2012-02
> > SQLite version 3.7.6.3
> > Enter ".help" for instructions
> > Enter SQL statements terminated with a ";"
> > sqlite> select url from pages where pages match 'punctuation';
> > http://www.corp.google.com/%257Edubroy/no_crawl/fts_test.html
> > sqlite> select url from pages where pages match '"Including: punctuation"';
> > http://www.corp.google.com/%257Edubroy/no_crawl/fts_test.html
> > sqlite> select url from pages where pages match '"Including punctuation"';
> > sqlite>
> > 
> > So it seems consistent with unit test behaviour. Am I doing something
> different
> > than you did?
> 
> Here's what I mean:
> 
> sqlite> CREATE VIRTUAL TABLE t USING fts3(x);
> sqlite> INSERT INTO t VALUES ("FOO page one. Including: punctuation.");
> sqlite> SELECT * FROM t WHERE t MATCH 'punctuation';
> FOO page one. Including: punctuation.
> sqlite> SELECT * FROM t WHERE t MATCH '"Including: punctuation"';
> FOO page one. Including: punctuation.
> sqlite> SELECT * FROM t WHERE t MATCH '"Including punctuation"';
> FOO page one. Including: punctuation.
> sqlite> SELECT * FROM t WHERE t MATCH '"Including_ punctuation!"';
> FOO page one. Including: punctuation.

We figured out that this was caused by the fact that the simple tokenizer is
used by default, which strips all punctuation characters. Chrome uses the ICU
tokenizer, which does not.

[Patrick Dubroy, 2013-04-16 13:19:00]

Hi Scott,

I'd like to revive this CL. To summarize the problem, searches on the history
page often return unexpected results. E.g.:

- a search for [google.com] will return any pages that contain [google] and
[com] as separate tokens
- a search for ["google.com"] (with the quotation marks) actually searches for
["google com"], because the tokenizer strips all punctuation, even inside
quotation marks.

This CL fixes these cases by making two changes:
- punctuation inside quotation marks is never stripped
- outside of quotation marks, things that look like domain names (i.e., a
contiguous string of alphanumerics, "-", or ".") are treated as a single token.

I believe the last remaining question we had was whether a domain-name-like
query should be implicitly quoted. If a query like [google.com] is not quoted,
the ICU tokenizer will look for anything that contains the tokens "google", ".",
and "com", in any order. This is probably not desired, and there are two ways I
can think of to fix it:

- have the QueryParser automatically turn [google.com] into ["google.com"]. The
downside is that prefix search don't work for quoted strings so if the user
searches for [google.co], it would not match a URL of google.com.

- examine the matches returned from SQLite, and discard anything that doesn't
match the QueryNodeWords from the QueryParser. This should allow us to fix the
problem without sacrificing prefix matches.

What do you think? I'd really like to get something in, because the experience
of searching on chrome://history is quite bad right now.

Thanks,

Pat

On 2012/02/20 11:00:36, dubroy wrote:
>
http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
> File chrome/browser/history/query_parser.cc (right):
> 
>
http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
> chrome/browser/history/query_parser.cc:154: void QueryNodeWord::Append(const
> string16& str) {
> On 2012/02/10 16:53:35, sky wrote:
> > Definition should match declaration order (this should be above
> > AppendToSQLiteQuery.
> 
> Done.
> 
>
http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
> chrome/browser/history/query_parser.cc:379: query_stack.push_back(root);
> I agree. I can fix this up in a follow-up CL if you'd like.
> 
> On 2012/02/17 22:34:59, shess wrote:
> > I don't understand why a stack is needed to parse quoted text.  If
!in_quotes,
> > put the string in a new node and append to root, if in_quotes put the string
> in
> > a new node and append to the last node of root (which is the phrase), and if
> > in_quotes and embedded punctuation, append the string to the last child node
> of
> > the last node in root.  But probably not for resolving today :-).
> 
>
http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
> chrome/browser/history/query_parser.cc:389: wchar_t last_char =
> query[iter.prev()];
> On 2012/02/17 22:34:59, shess wrote:
> > I'm not sure "last_char" is the best description.  AFAICT this is the break
> > point between the previous token and this token, when used as an index it
will
> > be the first character of the current token.  Reading this line sounds like
> it's
> > saying the last character of the previous token.  Maybe first_char?
> 
> You're right. Not sure why I named it like that. Done.
> 
>
http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
> chrome/browser/history/query_parser.cc:390: if (IsQueryQuote(last_char)) {
> On 2012/02/10 17:33:43, mrossetti wrote:
> > I almost commented on that as well but the existing code doesn't take that
> into
> > account. :^/
> > 
> > On 2012/02/10 16:53:35, sky wrote:
> > > Should we require the trailing quote character to match the starting quote
> > > character?
> > 
> 
> I think we should. I can address this in a follow-up CL.
> 
>
http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
> chrome/browser/history/query_parser.cc:403: // whitespace character inside
> quotes.
> On 2012/02/17 22:34:59, shess wrote:
> > If it's not a word, is it always a single character?  I'd say
"non-whitespace
> > token", just in case that's not a safe assumption.
> 
> Done.
> 
>
http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/quer...
> chrome/browser/history/query_parser.cc:418:
> current_word->Append(iter.GetString());
> On 2012/02/17 22:34:59, shess wrote:
> > If I understand this right, I'm not sure I buy it.  This means that unquoted
> > {google.com} will be passed as {google.com}, which the tokenizer will
convert
> to
> > {google AND . AND com}.  It may be an improvement in that it will filter out
> > documents which contain {google} and {com} but not {.}, but I'm not sure the
> > matches will really make sense, as they'll include things like "google
> > commercial.".  How important do you think prefix matching is in a case like
> > this?  I think converting it to match {"google.com"} would make some sense.
> > 
> > Mostly I'm thinking that describing out it works is odd.  Saying that it
> ignores
> > punctuation is easy, saying that it finds them in that order is easy, but
> saying
> > that it treats . like a word is weird (to non-engineer types).
> 
> I agree. I don't think it's very common to include punctuation but not want to
> do an exact match. I also think that the prefix matching for such strings is
> also not that important. As long as [google.com] matches things like
> "www.google.com" and "google.com/images", I think that's fine. I don't think
> people would suffer if a search for [google.co] didn't match "google.com".
> 
> There's still the question of whether exact matching should be implemented by
> quoting such strings, or by filtering them after the fact. I'm starting to
think
> that it should just be done here -- it's simpler and less error-prone, I
think.
> 
> So, the question is: should any punctuation character not surrounded by
> whitespace trigger this behaviour? Or only some subset of characters (e.g.
those
> that can appear in a URL).
> 
>
http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/text...
> File chrome/browser/history/text_database_manager_unittest.cc (right):
> 
>
http://codereview.chromium.org/9316129/diff/15004/chrome/browser/history/text...
> chrome/browser/history/text_database_manager_unittest.cc:590: EXPECT_EQ(0U,
> results.size());
> On 2012/02/11 17:09:55, shess wrote:
> > On 2012/02/11 10:58:39, dubroy wrote:
> > > On 2012/02/10 22:22:32, shess wrote:
> > > > I don't understand how this one is working.  When I test against a raw
> fts3
> > > > table, MATCH '"Including: punctiotion."' gives the same results as MATCH
> > > > '"Including punctuation"'.  Is there post-query filtering which is
> different
> > > > because of this change?
> > > 
> > > Weird. It doesn't look like there is any post-query filtering going on.
> > > And when I test on my history index, here's the behaviour I see:
> > > 
> > > /usr/local/google/dubroy/chromium/src$ out/Debug/sqlite_shell
> > > ~/.config/chromium/Default/History\ Index\ 2012-02
> > > SQLite version 3.7.6.3
> > > Enter ".help" for instructions
> > > Enter SQL statements terminated with a ";"
> > > sqlite> select url from pages where pages match 'punctuation';
> > > http://www.corp.google.com/%25257Edubroy/no_crawl/fts_test.html
> > > sqlite> select url from pages where pages match '"Including:
punctuation"';
> > > http://www.corp.google.com/%25257Edubroy/no_crawl/fts_test.html
> > > sqlite> select url from pages where pages match '"Including punctuation"';
> > > sqlite>
> > > 
> > > So it seems consistent with unit test behaviour. Am I doing something
> > different
> > > than you did?
> > 
> > Here's what I mean:
> > 
> > sqlite> CREATE VIRTUAL TABLE t USING fts3(x);
> > sqlite> INSERT INTO t VALUES ("FOO page one. Including: punctuation.");
> > sqlite> SELECT * FROM t WHERE t MATCH 'punctuation';
> > FOO page one. Including: punctuation.
> > sqlite> SELECT * FROM t WHERE t MATCH '"Including: punctuation"';
> > FOO page one. Including: punctuation.
> > sqlite> SELECT * FROM t WHERE t MATCH '"Including punctuation"';
> > FOO page one. Including: punctuation.
> > sqlite> SELECT * FROM t WHERE t MATCH '"Including_ punctuation!"';
> > FOO page one. Including: punctuation.
> 
> We figured out that this was caused by the fact that the simple tokenizer is
> used by default, which strips all punctuation characters. Chrome uses the ICU
> tokenizer, which does not.

[Scott Hess - ex-Googler, 2013-04-16 20:34:43]

On 2013/04/16 13:19:00, dubroy wrote:
> I'd like to revive this CL.

Looking at the timeframe, I think the last time around someone was attempting to
make me work on an annoying project, which is why I'm busy reviving and landing
year-old CLs myself!  I'm digging through things.

[Scott Hess - ex-Googler, 2013-04-17 18:14:02]

AFAICT, I think my previous reservations about whether-this-is-right-at-all are
all addressed.  My remaining concern is mostly about whether the logic is right
if there are scripts which don't throw up English-style word/non-word/word
sequences.

https://chromiumcodereview.appspot.com/9316129/diff/37002/chrome/browser/hist...
File chrome/browser/history/query_parser.cc (right):

https://chromiumcodereview.appspot.com/9316129/diff/37002/chrome/browser/hist...
chrome/browser/history/query_parser.cc:92: void Append(const string16& word);
I think this sense of "Append" is enough different from the sense of "Append"
inherited from QueryNode that maybe it would be worth changing to something
else.  Accumulate?  And I prefer the |str| usage over |word|, or some other term
implying bag-o-chars.

https://chromiumcodereview.appspot.com/9316129/diff/37002/chrome/browser/hist...
chrome/browser/history/query_parser.cc:402: } else if (iter.IsWord() ||
(in_quotes && !IsWhitespace(first_char))) {
Previously, if there were a sequence of IsWord() tokens without intervening
!IsWord() tokens, it would become a sequence of QueryNodeWord children.  Now
they will be appended together into a single QueryNodeWord.

https://chromiumcodereview.appspot.com/9316129/diff/37002/chrome/browser/hist...
chrome/browser/history/query_parser.cc:419:
current_word->Append(iter.GetString());
I believe this won't back out the symbol(s) if the next token is a non-word. 
Intentional to allow training non-breaking symbols?  If so, the comment should
be modified.

I'm trying to think of how this could just accumulate the word using prev() and
pos(), then capture it once, but AFAICT it would require too many points to
commit the word.  You could accumulate the non-breaking symbols in a string16,
and change the above to accumulate them on the end of the previous word before
the new word, then clear the symbol accumulator when opening a new current_word.

I think that could also handle the adjacent IsWord() problem mentioned above. 
Append if there is a current_word _and_ the non-breaking accumulator has
content, otherwise push a new QueryNodeWord.

[Patrick Dubroy, 2013-05-15 09:48:06]

Sigh. Unfortunately I am going to have to put this off again for a while. Thanks
for you help though.

On 2013/04/17 18:14:02, shess wrote:
> AFAICT, I think my previous reservations about whether-this-is-right-at-all
are
> all addressed.  My remaining concern is mostly about whether the logic is
right
> if there are scripts which don't throw up English-style word/non-word/word
> sequences.
> 
>
https://chromiumcodereview.appspot.com/9316129/diff/37002/chrome/browser/hist...
> File chrome/browser/history/query_parser.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9316129/diff/37002/chrome/browser/hist...
> chrome/browser/history/query_parser.cc:92: void Append(const string16& word);
> I think this sense of "Append" is enough different from the sense of "Append"
> inherited from QueryNode that maybe it would be worth changing to something
> else.  Accumulate?  And I prefer the |str| usage over |word|, or some other
term
> implying bag-o-chars.
> 
>
https://chromiumcodereview.appspot.com/9316129/diff/37002/chrome/browser/hist...
> chrome/browser/history/query_parser.cc:402: } else if (iter.IsWord() ||
> (in_quotes && !IsWhitespace(first_char))) {
> Previously, if there were a sequence of IsWord() tokens without intervening
> !IsWord() tokens, it would become a sequence of QueryNodeWord children.  Now
> they will be appended together into a single QueryNodeWord.
> 
>
https://chromiumcodereview.appspot.com/9316129/diff/37002/chrome/browser/hist...
> chrome/browser/history/query_parser.cc:419:
> current_word->Append(iter.GetString());
> I believe this won't back out the symbol(s) if the next token is a non-word. 
> Intentional to allow training non-breaking symbols?  If so, the comment should
> be modified.
> 
> I'm trying to think of how this could just accumulate the word using prev()
and
> pos(), then capture it once, but AFAICT it would require too many points to
> commit the word.  You could accumulate the non-breaking symbols in a string16,
> and change the above to accumulate them on the end of the previous word before
> the new word, then clear the symbol accumulator when opening a new
current_word.
> 
> I think that could also handle the adjacent IsWord() problem mentioned above. 
> Append if there is a current_word _and_ the non-breaking accumulator has
> content, otherwise push a new QueryNodeWord.