828344 - "contains all of the words" no longer looks for all words within the same comment or flag

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[:glob ✱]

consider the following scenario:

you have a bug "A" with three comments:
"first"
"second"
"third"

and another bug "B" with a a single comment:
"first second third"

in 4.0 and earlier, performing a custom search for:

"comment"
"contains all of the strings"
"first second third"

will return only bug B.  it's matching bugs where a *single* comment matches all of the words.

in 4.2 and higher, both the query returns both bugs A and B.  it's matching bugs where *any* comment contains any of the words.


i think the 4.2 behavour is incorrect.

Comment 1

[:glob ✱]

Attachment: patch 0 (wip)

here's my wip patch.

this changes comment searching from a subselect to a condition on the where clause.

i'm working on updating the xt tests.

Comment 2

[Frédéric Buclin]

Any progress?

Comment 3

[:glob ✱]

yes, i'm down to 6 failed xt tests now.  most of them are double-negative, eg:

#   Failed test 'NOT(longdesc-notsubstring-<1>): contains bug 1 (499)'
#   at xt/lib/Bugzilla/Test/Search/FieldTest.pm line 643.
#    Value: '1-comment-0tnejuZxFR3g28h80ZJ25Dj'
# Expected: [2,3,4,5]
#  Results: []

what i've done is change bug 5 so its comment contains content, and i'm using that for the longdesc word searches.  i've also added a <1-first-value> value to the test suite to facilitate multi_boolean testing of longdescs.

Comment 4

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #3)
> yes, i'm down to 6 failed xt tests now.

Do they fail because your current patch breaks stuff or because these xt tests are invalid/wrong?

Comment 5

[:glob ✱]

(In reply to Frédéric Buclin from comment #4)
> Do they fail because your current patch breaks stuff or because these xt
> tests are invalid/wrong?

because the xt/ tests were testing for the old behavour, and need updating.

Comment 6

[Frédéric Buclin]

Comment on attachment 700195 [details] [diff] [review]
patch 0 (wip)

Besides the xt/ tests needing to be updated, is this patch ready for review or do you have an updated one?

Comment 7

[:glob ✱]

(In reply to Frédéric Buclin from comment #6)
> Comment on attachment 700195 [details] [diff] [review]
> patch 0 (wip)
> 
> Besides the xt/ tests needing to be updated, is this patch ready for review
> or do you have an updated one?

while working through xt/, i haven't had to make any changes to that part of the patch yet.  if you ignore xt, then it's ready for review.

Comment 9

[:glob ✱]

'contains all the words/strings' is also broken when searching flags.

Comment 11

[:glob ✱]

i've split the work to upgrade the xt/ tests into a separate bug, so we can land this quicker.

Comment 12

[:glob ✱]

Attachment: patch v1

Comment 13

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Searching for a flag without a requestee also is broken
ie. 
flag, contains the string, sec-review
flag requestee, is empty

Comment 14

[:glob ✱]

(In reply to Curtis Koenig [:curtisk] from comment #13)
> Searching for a flag without a requestee also is broken
> ie. 
> flag, contains the string, sec-review
> flag requestee, is empty

that isn't related to this issue, which is about "contains all of the words" issues.
filed as bug 851404.

Comment 15

[Frédéric Buclin]

Comment on attachment 722675 [details] [diff] [review]
patch v1

I'm by far not familiar with the code here, so I may ask obvious questions. Sorry for that if that's the case.

>+sub _long_desc_nonchanged {

>+    my $join_args = {
>+        chart_id   => $chart_id,
>+        sequence   => $chart_id,
>+        field      => 'longdesc',
>+        full_field => "$table.thetext",
>+        operator   => $operator,
>+        value      => $value,
>+        all_values => $value,
>+        quoted     => $dbh->quote($value),
>+        joins      => [],
>+        bugs_table => $bugs_table,
>+    };

Is there a place where all these keys are documented and what they are used for? I have *no idea* which ones must be set, and what they are used for.


>+    $self->_do_operator_function($join_args);

Why _do_operator_function() instead of do_search_function()?


>+    if (!$self->_user->is_insider) {
>+        $join_args->{term} .= " AND $table.isprivate = 0";
>+    }
>+    $args->{term} =  "$table.comment_id IS NOT NULL";

What's the difference between $join_args->{term} and $args->{term}? The first one is used for the JOIN condition and the 2nd one for the WHERE part?


>+sub _flagtypes_nonchanged {

I have mostly the same questions for this method.


I did some basic testing with flags, and everything seems to work fine when using is/contains, but negated queries seem to be ignored/broken. Here are the tests I did:

I have some bugs with two flagtypes (at the bug level, not the attachment level): approval and tested. I wanted to get all bugs with approval? *and* tested+, i.e. ignoring requests for approval if the bug hasn't been successfully tested yet. So I ran the following query:

Match ALL of the following separately:
Flags contains the string approval?
AND
NOT Flags contains the string tested-
AND
NOT Flags contains the string tested?

The last 2 criteria are simply ignored, or at least do not work. If I write:

Match ALL of the following separately:
Flags contains the string approval?
AND
Flags contains the string tested+

then I get the expected buglist.

I also tried using QuickSearch: "ALL approval? -flag:tested- -flag:tested?", but this didn't work either.

I also noticed that if I add a 4th criteria, MySQL suddenly takes ages to complete. I always had to kill the process. With 3 criteria, it's very fast. So I don't know what's going wrong here.

I didn't test comments yet as I suspect they may suffer the same problems.

Comment 16

[Frédéric Buclin]

About slowness, SHOW PROCESSLIST says that it's wasting time copying data to the temporary table.

Comment 17

[:glob ✱]

(In reply to Frédéric Buclin from comment #15)
> >+    my $join_args = {
> >+        chart_id   => $chart_id,
> >+        sequence   => $chart_id,
> >+        field      => 'longdesc',
> >+        full_field => "$table.thetext",
> >+        operator   => $operator,
> >+        value      => $value,
> >+        all_values => $value,
> >+        quoted     => $dbh->quote($value),
> >+        joins      => [],
> >+        bugs_table => $bugs_table,
> >+    };
> 
> Is there a place where all these keys are documented and what they are used
> for? I have *no idea* which ones must be set, and what they are used for.

i'm not aware of any documentation.
i'll rough out what i know:
 
chart_id      - integer representing the boolean chart row, also used to bind queries
                grouped by the same field together
sequence      - used to build some terms (not sure why you need both chart_id and
                sequence, but i haven't ventured into code which deals with sequences yet)
field         - field name (from the chart)
operator      - operator (from the chart)
value         - value (from the chart)
full_field    - full field name, may contain sql (eg. subselects, coalesce, concat)
all_values    - used for search functions which operate on multiple values
quoted        - the value, but quoted.  numeric values aren't quoted
joins         - arrayref of tables to join to (see below)
bugs_table    - name of the bugs table (may not be 'bugs' if grouping by a single field)
table_suffix  - no idea, looks unused
term          - raw WHERE sql
value_is_id   - used only when pronouns are provided, true when the value is an id
_extra_where  - used by multiselect override to provide additional WHERE sql
_select_field - used by multiselect override to set the selected field for the subselect

joins:
table - name of the table to join to
as    - name of the joined table
from  - field name we're joining from (on the bugs table)
to    - joined table field name
extra - arrayref of raw sql which is added to the join sql

> >+    $self->_do_operator_function($join_args);
> 
> Why _do_operator_function() instead of do_search_function()?

because we're in an override function, we can't call do_search_function() as that will call overrides... we just need the final term.

> What's the difference between $join_args->{term} and $args->{term}? The
> first one is used for the JOIN condition and the 2nd one for the WHERE part?

yes.

$join_args is used just to generate the {term} via _do_operator_function().  this is used on the join's 'where' by stuffing it into $join->{extra}.  as you correctly stated, $args->{term} is used on the main 'where' part to check the join matched something.

> I did some basic testing with flags, and everything seems to work fine when
> using is/contains, but negated queries seem to be ignored/broken. Here are
> the tests I did:

thanks, will test.

> I also noticed that if I add a 4th criteria, MySQL suddenly takes ages to
> complete. I always had to kill the process. With 3 criteria, it's very fast.
> So I don't know what's going wrong here.

does 4.0 also experience the same perf hit?  (the flag sql generated was ported straight from 4.0).

the fix for comments is similar, but different; it's best to not assume they both suffer from the same issues.

Comment 18

[:glob ✱]

just so i have it documented, this is the sort of query we'll have to build to get flags searching correctly:

SELECT 
  bugs.bug_id,
  GROUP_CONCAT(DISTINCT CONCAT(flagtypes_1.name, flags_1.status) separator ' ') AS flags
FROM bugs
  LEFT JOIN attachments AS attachments_1 ON bugs.bug_id = attachments_1.bug_id
  LEFT JOIN flags AS flags_1 ON bugs.bug_id = flags_1.bug_id
    AND (flags_1.attach_id = attachments_1.attach_id OR flags_1.attach_id IS NULL)
  LEFT JOIN flagtypes AS flagtypes_1 ON flags_1.type_id = flagtypes_1.id
GROUP BY bugs.bug_id
HAVING
  INSTR(flags, 'approval?') > 0
  AND NOT (INSTR(flags, 'review-') > 0)
  AND NOT (INSTR(flags, 'review?') > 0)
ORDER BY bugs.bug_id

this combines all the flags into a single string with group_concat, which makes searching for flags with weird combinations easy and predictable.

Comment 19

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #18)
> GROUP_CONCAT(DISTINCT CONCAT(flagtypes_1.name, flags_1.status) separator ' ') AS flags

You don't want to use a whitespace as separator, isn't it? They should be concatenated with no space between them, else you cannot match e.g. review+.

Comment 20

[:glob ✱]

(In reply to Frédéric Buclin from comment #19)
> (In reply to Byron Jones ‹:glob› from comment #18)
> > GROUP_CONCAT(DISTINCT CONCAT(flagtypes_1.name, flags_1.status) separator ' ') AS flags
> 
> You don't want to use a whitespace as separator, isn't it? They should be
> concatenated with no space between them, else you cannot match e.g. review+.

the CONCAT joins the name and status without a space, the separator is part of the GROUP_CONCAT which joins multiple rows.

you end up with: "approval? review+"

Comment 21

[:glob ✱]

Attachment: patch v2

this revision implements flag searching via group_concat and having.

note: this is a -w patch, as i've removed all trailing whitespace from the search code to stop vim yelling at me.

Comment 22

[Frédéric Buclin]

Comment on attachment 735655 [details] [diff] [review]
patch v2

On PostgreSQL, typing "review?LpSolit" in the QuickSearch box makes Pg to fail. IIRC, the reason is that if you have something like "SELECT some_function AS foo", you must write "HAVING some_function" instead of "HAVING foo". I will do some more testing on MySQL meanwhile, and then also test with Oracle and SQLite.


DBD::Pg::db selectcol_arrayref failed: ERROR:  column "flag_grouped_bugs" does not exist
LINE 12: ...NG POSITION(LOWER('review?'::text)::text IN LOWER(flag_group...
                                                              ^ [for Statement "SELECT bugs.bug_id AS bug_id, array_to_string(array_accum(DISTINCT (CAST(flagtypes_1.name AS text) || CAST(flags_1.status AS text))), ' ') AS flag_grouped_bugs
  FROM bugs
LEFT JOIN bug_group_map AS security_map ON bugs.bug_id = security_map.bug_id
LEFT JOIN attachments AS attachments_1 ON bugs.bug_id = attachments_1.bug_id AND attachments_1.isprivate = 0
LEFT JOIN flags AS flags_1 ON bugs.bug_id = flags_1.bug_id AND ((flags_1.attach_id = attachments_1.attach_id  OR flags_1.attach_id IS NULL))
LEFT JOIN flagtypes AS flagtypes_1 ON flags_1.type_id = flagtypes_1.id
LEFT JOIN (SELECT DISTINCT bug_id FROM flags AS requestees_login_name_2 INNER JOIN profiles AS name_requestees_login_name_2 ON requestees_login_name_2.requestee_id = name_requestees_login_name_2.userid AND POSITION(LOWER('lpsolit'::text)::text IN LOWER(COALESCE(name_requestees_login_name_2.login_name, '')::text)::text) > 0) AS requestees_login_name_2_2 ON bugs.bug_id = requestees_login_name_2_2.bug_id
 WHERE bugs.creation_ts IS NOT NULL
   AND security_map.group_id IS NULL
   AND  bugs.bug_status IN ('UNCONFIRMED','NEW','ASSIGNED','SUSPENDED','WAITING','REOPENED')  AND requestees_login_name_2_2.bug_id IS NOT NULL
GROUP BY bugs.bug_id
HAVING POSITION(LOWER('review?'::text)::text IN LOWER(flag_grouped_bugs::text)::text) > 0
ORDER BY bug_id
LIMIT 500
"] at Bugzilla/Search.pm line 727
	Bugzilla::Search::data('Bugzilla::Search=HASH(0x9d07bc8)') called at /var/www/html/bugzilla-pg/buglist.cgi line 721

Comment 23

[Frédéric Buclin]

Another example, on Pg:

DBD::Pg::db selectcol_arrayref failed: ERROR:  column "flag_grouped_bugs" does not exist
LINE 12: HAVING flag_grouped_bugs = 'review'
                ^ [for Statement "SELECT bugs.bug_id AS bug_id, array_to_string(array_accum(DISTINCT (CAST(flagtypes_1.name AS text) || CAST(flags_1.status AS text))), ' ') AS flag_grouped_bugs
  FROM bugs
LEFT JOIN bug_group_map AS security_map ON bugs.bug_id = security_map.bug_id
LEFT JOIN attachments AS attachments_1 ON bugs.bug_id = attachments_1.bug_id AND attachments_1.isprivate = 0
LEFT JOIN flags AS flags_1 ON bugs.bug_id = flags_1.bug_id AND ((flags_1.attach_id = attachments_1.attach_id  OR flags_1.attach_id IS NULL))
LEFT JOIN flagtypes AS flagtypes_1 ON flags_1.type_id = flagtypes_1.id
LEFT JOIN (SELECT DISTINCT bug_id FROM flags AS requestees_login_name_2 INNER JOIN profiles AS name_requestees_login_name_2 ON requestees_login_name_2.requestee_id = name_requestees_login_name_2.userid AND POSITION(LOWER('LpSolit'::text)::text IN LOWER(COALESCE(name_requestees_login_name_2.login_name, '')::text)::text) > 0) AS requestees_login_name_2_2 ON bugs.bug_id = requestees_login_name_2_2.bug_id
 WHERE bugs.creation_ts IS NOT NULL
   AND security_map.group_id IS NULL
   AND  bugs.resolution IN ('')  AND requestees_login_name_2_2.bug_id IS NOT NULL
GROUP BY bugs.bug_id
HAVING flag_grouped_bugs = 'review'
ORDER BY bug_id
LIMIT 500
"] at Bugzilla/Search.pm line 727
	Bugzilla::Search::data('Bugzilla::Search=HASH(0xa36eda8)') called at /var/www/html/bugzilla-pg/buglist.cgi line 721

Comment 24

[Frédéric Buclin]

(In reply to Frédéric Buclin from comment #22)
> fail. IIRC, the reason is that if you have something like "SELECT
> some_function AS foo", you must write "HAVING some_function" instead of
> "HAVING foo".

Yes, I was right:

On MySQL, this works:

SELECT bug_status, count(*) as number FROM bugs GROUP BY bug_status HAVING number > 10 ORDER BY number DESC;

On PostgreSQL, this same query fails, because it doesn't recognize number as a valid column. Instead, you must write:

SELECT bug_status, count(*) as number FROM bugs GROUP BY bug_status HAVING count(*) > 10 ORDER BY number DESC;

So ORDER BY is happy to use the alias "number", but HAVING is not; you must rewrite count(*) instead. This makes SQL queries terribly ugly when the aggregate function is pretty complex. I wonder if we shouldn't implement a new $dbh->sql_having() which takes two arguments: the aggregate function and its alias, and then MySQL can use the alias, and PostgreSQL can use the aggregate function.

Comment 25

[Frédéric Buclin]

SQLite also lets you use aliases in HAVING, but Oracle doesn't. So you need to pass the whole aggregate function into HAVING for PostgreSQL and Oracle.

Comment 26

[Frédéric Buclin]

If I type "review?lpsolit" in the QuickSearch box on MySQL, it incorrectly finds bugs which have "review?" and "needinfo?lpsolit". But maybe that's a bug in the QS code itself.

Comment 27

[:glob ✱]

Attachment: patch v3

doing $dbh->sql_having() wasn't practical, so instead i don't use the alias at all.

Comment 28

[Frédéric Buclin]

Comment on attachment 736287 [details] [diff] [review]
patch v3

As discussed on IRC, using flags in the Advanced Search page is ignored. Bugzilla throws: "You may not search, or create saved searches, without any search terms."

Also, it looks like QuickSearch incorrectly parses "review?foo" as it's unable to join review? and foo together, i.e. it also matches review?bar and needinfo?foo. But that's probably a bug in Search/Quicksearch.pm itself as the code there is older than the rewrite of Search.pm and so is unaware of the "against the same field" feature.

Comment 29

[Frédéric Buclin]

Comment on attachment 736287 [details] [diff] [review]
patch v3

>+sub _flagtypes_nonchanged {

>+    # add a placeholder term (an empty term will result in this condition being ignored)
>+    $args->{term} = ' ';

Hum, doesn't this explain why flags are ignored?

Comment 30

[:glob ✱]

flags aren't being ignored; that error is only triggered if there are no other criteria.

my plan is to remove the whitespace hack, and to change all places which check for an empty term to instead check for an empty term|having.

Comment 31

[:glob ✱]

Attachment: patch v4

changes
- removes the term-contains-just-whitespace hack, instead check for term||having
- fixes buglist_params_required check to also do term||having
- fixes quicksearch flag+requestee searching

Comment 32

[Frédéric Buclin]

Comment on attachment 736696 [details] [diff] [review]
patch v4

My basic tests involving one single flag seem to work fine now. But all my tests involving several flags on MySQL fail, with both QuickSearch and the Advanced Search page.

"review+ approval?" doesn't work. It generates the SQL query below. Note how slow the query is despite my DB is tiny (only 1275 bugs). Note also that both flags are built using the *_0 tables. This means that the *_1 tables are not used. I guess this is responsible for the huge slowness. I didn't try with PostgreSQL nor Oracle as I doubt things would be better. I wonder if using VIEWs would make things easier.


SELECT bugs.bug_id AS bug_id, GROUP_CONCAT(DISTINCT CONCAT(flagtypes_0.name, flags_0.status, flag_profiles_0.login_name) SEPARATOR ' ') FROM bugs LEFT JOIN bug_group_map AS security_map ON bugs.bug_id = security_map.bug_id AND NOT ( security_map.group_id IN (1,12,10,30,11,13,9,4,8,5,6,7,3,2,29,26,28,27,18,14,20,15,16) ) LEFT JOIN cc AS security_cc ON bugs.bug_id = security_cc.bug_id AND security_cc.who = 1 LEFT JOIN attachments AS attachments_0 ON bugs.bug_id = attachments_0.bug_id LEFT JOIN flags AS flags_0 ON bugs.bug_id = flags_0.bug_id AND ((flags_0.attach_id = attachments_0.attach_id OR flags_0.attach_id IS NULL)) LEFT JOIN flagtypes AS flagtypes_0 ON flags_0.type_id = flagtypes_0.id LEFT JOIN profiles AS flag_profiles_0 ON flags_0.requestee_id = flag_profiles_0.userid LEFT JOIN attachments AS attachments_1 ON bugs.bug_id = attachments_1.bug_id LEFT JOIN flags AS flags_1 ON bugs.bug_id = flags_1.bug_id AND ((flags_1.attach_id = attachments_1.attach_id OR flags_1.attach_id IS NULL)) LEFT JOIN flagtypes AS flagtypes_1 ON flags_1.type_id = flagtypes_1.id LEFT JOIN profiles AS flag_profiles_1 ON flags_1.requestee_id = flag_profiles_1.userid WHERE bugs.creation_ts IS NOT NULL AND (security_map.group_id IS NULL OR (bugs.reporter_accessible = 1 AND bugs.reporter = 1) OR (bugs.cclist_accessible = 1 AND security_cc.who IS NOT NULL) OR bugs.assigned_to = 1 OR bugs.qa_contact = 1) GROUP BY bugs.bug_id HAVING INSTR(GROUP_CONCAT(DISTINCT CONCAT(flagtypes_0.name, flags_0.status, flag_profiles_0.login_name) SEPARATOR ' '), 'review+') > 0 AND INSTR(GROUP_CONCAT(DISTINCT CONCAT(flagtypes_0.name, flags_0.status, flag_profiles_0.login_name) SEPARATOR ' '), 'approval?') > 0 ORDER BY bug_id LIMIT 500

Execution time: 4.547525 seconds

| id | select_type | table           | type        | possible_keys                                                                   | key                      | key_len | ref                                                   | rows | Extra                    |
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
| 1  | SIMPLE      | bugs            | index       | bugs_assigned_to_idx,bugs_creation_ts_idx,bugs_reporter_idx,bugs_qa_contact_idx | PRIMARY                  | 3       | NULL                                                  | 315  | Using where              |
| 1  | SIMPLE      | security_map    | ref         | bug_group_map_bug_id_idx,bug_group_map_group_id_idx                             | bug_group_map_bug_id_idx | 3       | bugs_cvs.bugs.bug_id                                  | 1    | Using where; Using index |
| 1  | SIMPLE      | security_cc     | eq_ref      | cc_bug_id_idx,cc_who_idx                                                        | cc_bug_id_idx            | 6       | bugs_cvs.bugs.bug_id,const                            | 1    | Using where; Using index |
| 1  | SIMPLE      | attachments_0   | ref         | attachments_bug_id_idx                                                          | attachments_bug_id_idx   | 3       | bugs_cvs.bugs.bug_id                                  | 1    | Using index              |
| 1  | SIMPLE      | flags_0         | ref_or_null | flags_bug_id_idx,fk_flags_attach_id_attachments_attach_id                       | flags_bug_id_idx         | 7       | bugs_cvs.bugs.bug_id,bugs_cvs.attachments_0.attach_id | 2    | Using where              |
| 1  | SIMPLE      | flagtypes_0     | eq_ref      | PRIMARY                                                                         | PRIMARY                  | 2       | bugs_cvs.flags_0.type_id                              | 1    | Using where              |
| 1  | SIMPLE      | flag_profiles_0 | eq_ref      | PRIMARY                                                                         | PRIMARY                  | 3       | bugs_cvs.flags_0.requestee_id                         | 1    | Using where              |
| 1  | SIMPLE      | attachments_1   | ref         | attachments_bug_id_idx                                                          | attachments_bug_id_idx   | 3       | bugs_cvs.bugs.bug_id                                  | 1    | Using index              |
| 1  | SIMPLE      | flags_1         | ref_or_null | flags_bug_id_idx,fk_flags_attach_id_attachments_attach_id                       | flags_bug_id_idx         | 7       | bugs_cvs.bugs.bug_id,bugs_cvs.attachments_1.attach_id | 2    | Using where              |

Comment 33

[Frédéric Buclin]

Comment on attachment 736696 [details] [diff] [review]
patch v4

>+sub _flagtypes_nonchanged {

>+        my $concat = $dbh->sql_string_concat("$flagtypes_table.name",
>+                                             "$flags_table.status",
>+                                             "$flag_profile_table.login_name");


I think I found what's wrong with your code, or at least a part of it. If there is no requestee, then CONCAT(flag, status, NULL) returns NULL, see http://dev.mysql.com/doc/refman/5.5/en/string-functions.html#function_concat:

 "CONCAT() returns NULL if any argument is NULL."

This could explain why it's unable to find bugs having e.g. review+ and approval?. This means you must exclude the requestee from CONCAT() and handle it separately.


While playing with MySQL, I found that using views could help a lot:

    CREATE flags_view (id, bug_id, flag, requestee)
 AS SELECT flags.id, flags.bug_id, CONCAT(flagtypes.name, flags.status), profiles.login_name
      FROM flags
INNER JOIN flagtypes ON flagtypes.id = flags.type_id
 LEFT JOIN profiles ON profiles.userid = flags.requestee_id;


This generates a table of the form:

desc flags_view;
+-----------+--------------+------+-----+---------+-------+
| Field     | Type         | Null | Key | Default | Extra |
+-----------+--------------+------+-----+---------+-------+
| id        | mediumint(9) | NO   |     | 0       |       |
| bug_id    | mediumint(9) | NO   |     | NULL    |       |
| flag      | varchar(51)  | NO   |     |         |       |
| requestee | varchar(255) | YES  |     | NULL    |       |
+-----------+--------------+------+-----+---------+-------+
4 rows in set (0.00 sec)


And then, all I have to do is:

SELECT DISTINCT bugs.bug_id
           FROM bugs
      LEFT JOIN flags_view f1 ON f1.bug_id = bugs.bug_id
      LEFT JOIN flags_view f2 ON f2.bug_id = bugs.bug_id
          WHERE f1.flag = 'review+'
            AND f2.flag = 'approval?';

+--------+
| bug_id |
+--------+
|   1020 |
+--------+
1 row in set (0.01 sec)

This makes the SQL query way easier to write.

Comment 34

[Frédéric Buclin]

Another example:

SELECT DISTINCT bugs.bug_id
           FROM bugs
      LEFT JOIN flags_view f1 ON f1.bug_id = bugs.bug_id
      LEFT JOIN flags_view f2 ON f2.bug_id = bugs.bug_id
          WHERE f1.flag = 'doc-review?'
            AND INSTR(f1.requestee, 'security') > 0
            AND f2.flag = 'tested?'
            AND INSTR(f2.requestee, 'LpSolit') > 0;

This looks much nicer IMO, as there is no need to include the flag name and status inside INSTR(). Unfortunately, there is no index on 'flag'.

Comment 35

[Frédéric Buclin]

(In reply to Frédéric Buclin from comment #33)
>     CREATE flags_view (id, bug_id, flag, requestee)

I of course meant CREATE VIEW flags_view ....

Comment 36

[Frédéric Buclin]

I looked at the DB schema and I think that to improve performance (which is actually pretty bad, see comment 32) you shouldn't use INSTR(flagtypes.name, flags.status) at all, but instead let the flag name match its flags.type_id equivalent so that the DB can use the flags_type_id_idx index. Else things become way too slow, even with two flags on a tiny DB. This would hopefully avoid the full table scan I saw with some queries when using your patch.

Comment 37

[:glob ✱]

(In reply to Frédéric Buclin from comment #33)
> While playing with MySQL, I found that using views could help a lot

i don't think adding the first VIEW to the bugzilla database is wise in a maintenance release.  file a new bug to track investigating this for bugzilla 5.0.

Comment 38

[:glob ✱]

> I think I found what's wrong with your code, or at least a part of it. If
> there is no requestee, then CONCAT(flag, status, NULL) returns NULL.

ah, nice catch.  was this is only issue you are referring to when you said that it fails?  if not, can you please detail the failures you encountered (just saying "it failed" doesn't help me fix it!).

> This means you must exclude the requestee from CONCAT() and handle it separately.

or use COALESCE :)

> Note also that both flags are built using the *_0 tables.
> This means that the *_1 tables are not used

oops, that's a trivial fix.

> I looked at the DB schema and I think that to improve performance (which is
> actually pretty bad, see comment 32) you shouldn't use INSTR(flagtypes.name,
> flags.status) at all, but instead let the flag name match its flags.type_id
> equivalent so that the DB can use the flags_type_id_idx index.

that would be *very* complicated to do in a way which accommodates the extra features which 4.2 search has (that's the path my first patch took).

> This would hopefully avoid the full table scan I saw with some queries when using
> your patch.

i'm unable to generate flag queries which result in full table scans -- can you please provide the criteria you used which resulted in full table scans?


using a landfill_tip database dump (~18k bugs, 11 flagstypes), with a patch that removes the duplicate joins, searching *all* bugs for 'review+ approval?' takes about 0.6 seconds.  of course it's much quicker when applying other criteria (eg. open bugs only).  running a query on a bmo dump for *open* bugs with 'review+ approval?' takes about 0.2 seconds.  think this performance is acceptable.

Comment 39

[:glob ✱]

Attachment: patch v5

Comment 40

[Frédéric Buclin]

Comment on attachment 737380 [details] [diff] [review]
patch v5

As discussed on IRC, "is equal to" doesn't work if a bug has several flags.

 Flags is equal to approval+

won't catch bugs having approval+ and needinfo?

Also, Flags contains all of the strings approval ?

will also catch bugs which have another flag with ?, e.g. approval+ and needinfo?, though we expect this query to only catch approval?, approval4.4?, approval4.2?, etc..., which is exactly what this bug is about. GROUP_CONCAT() explains these problems. Maybe using CONCAT() only would work better, because it wouldn't mix data from different flags.

Comment 41

[:glob ✱]

Attachment: patch v6

Comment 42

[Frédéric Buclin]

Comment on attachment 738359 [details] [diff] [review]
patch v6

Works perfectly in my testing! Congratulations, glob! And thank you for all your time spent on this painful bug. :)

Comment 43

[Frédéric Buclin]

It's high time to release 4.4. \o/

Comment 44

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Search.pm
modified Bugzilla/Search/Clause.pm
modified Bugzilla/Search/Quicksearch.pm
Committed revision 8612.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
modified Bugzilla/Search.pm
modified Bugzilla/Search/Clause.pm
modified Bugzilla/Search/Quicksearch.pm
Committed revision 8545.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified Bugzilla/Search.pm
modified Bugzilla/Search/Clause.pm
modified Bugzilla/Search/Quicksearch.pm
Committed revision 8206.

Comment 45

[:glob ✱]

due to differences in search between 4.2 and 4.4, my patch there broke a few things.
i've backed it out:

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified Bugzilla/Search.pm
modified Bugzilla/Search/Clause.pm
modified Bugzilla/Search/Quicksearch.pm
Committed revision 8207.

and will work on a 4.2 specific patch.

i also need to disable the xt tests which this patch breaks to avoid burning those trees.

Comment 46

[:glob ✱]

Attachment: patch for 4.2

4.2 broke because my 4.4/trunk patch relied on changes made in bug 780820.

Comment 47

[Frédéric Buclin]

Comment on attachment 738921 [details] [diff] [review]
patch for 4.2

> sub build_subselect {
>-    my ($outer, $inner, $table, $cond) = @_;
>-    return "$outer IN (SELECT $inner FROM $table WHERE $cond)";
>+    my ($outer, $inner, $table, $cond, $negate) = @_;
>+    # Execute subselects immediately to avoid dependent subqueries, which are
>+    # large performance hits on MySql
>+    my $q = "SELECT DISTINCT $inner FROM $table WHERE $cond";
>+    my $dbh = Bugzilla->dbh;
>+    my $list = $dbh->selectcol_arrayref($q);
>+    return $negate ? "1=1" : "1=2" unless @$list;
>+    return $dbh->sql_in($outer, $list, $negate);
> }

Is this change required? This is a perf improvement which landed in 4.4 only, and is not really a bug fix.

Comment 48

[:glob ✱]

(In reply to Frédéric Buclin from comment #47)
> Comment on attachment 738921 [details] [diff] [review]
> Is this change required? This is a perf improvement which landed in 4.4
> only, and is not really a bug fix.

this patch has been tested and tuned performance-wise with that change included.  i also feel the performance gain here is worth bring to 4.2, and would consider performance regressions to be a bug.

Comment 49

[:glob ✱]

Attachment: patch for xt

this patch marks the impacted tests as broken.

i've also changed the failure output slightly, which helped me a lot in understanding the failures.  happy to remove this part of the patch if you prefer.

Comment 50

[Frédéric Buclin]

Comment on attachment 738921 [details] [diff] [review]
patch for 4.2

> sub build_subselect {

>+    return $dbh->sql_in($outer, $list, $negate);

sql_in() in 4.2 takes no $negate argument, and so queries involving (custom) multi-select fields are broken if negative operators are used. You need to backport changes made in Bugzilla::DB::sql_in() and Bugzilla::DB::Oracle::sql_in().

Otherwise works fine with flags and comments.

Comment 51

[:glob ✱]

Attachment: patch for 4.2 v2

Comment 52

[Jeff Hedlund]

This broke some of my whining report searches on 4.4, so I've reverted back to r8544 and my searches work.

I have a few searches for flags on bugs with "contains the string" and "does not contain the string", and after updating with r8545 some of them broke showing bugs that have the flags when searching for those that do not.

Comment 53

[:glob ✱]

(In reply to Jeff Hedlund from comment #52)
> I have a few searches for flags on bugs with "contains the string" and "does
> not contain the string", and after updating with r8545 some of them broke
> showing bugs that have the flags when searching for those that do not.

are you able to provide access to your system so i can investigate?
if not, can you please provide a more detailed description of the issue you encountered (steps to reproduce, expected outcome, actual outcome).

Comment 54

[Jeff Hedlund]

I was able to replicate on the Landfill bugzilla's, assuming the 4.4 and 4.2 are up to date (therefore 4.4 is 'broken' and 4.2 was backed out, therefore 'not broken').

A really simple query (and meaningless but shows the problem):

Advanced Search, clear the Resolution field, add "10" to the "Bugs Numbered" field. (bug 10 has no flags set).

Then in the Custom Search choose Flags, Does not contain the string, "blocker" (a flag that is not set on bug 10).

On the 4.4 landfill, there are zero bugs when bug 10 should show up.

On the 4.2 landfill, bug 10 shows up in the list.

Searches: 

4.4:
https://landfill.bugzilla.org/bugzilla-4.4-branch/buglist.cgi?bug_id=10&bug_id_type=anyexact&f1=flagtypes.name&order=Importance&o1=notsubstring&query_format=advanced&v1=blocker

4.2:
https://landfill.bugzilla.org/bugzilla-4.2-branch/buglist.cgi?bug_id=10&bug_id_type=anyexact&f1=flagtypes.name&order=Importance&o1=notsubstring&query_format=advanced&v1=blocker

Comment 55

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

this search now returns zarro bugs when it should not
Product: mozilla.org

Component: "Security Assurance","Security Assurance: Applications","Security Assurance: Incident","Security Assurance: Operations","Security Assurance: Review Request"

Status: New, Reopened, Assigned
Assignee: [e-mail] OR security-assurance
whiteboard: (does not contain) triage
whiteboard: (does not contain) mentor
flags: (does not contain) needinfo

Comment 56

[Daniel Veditz [:dveditz]]

I think bug 864133 describes what we're seeing. Not sure if it's a "regression" from this one on BMO, though, because this bug is still open -- did this land as part of the changes last night?

Comment 57

[:glob ✱]

(In reply to Daniel Veditz [:dveditz] from comment #56)
> I think bug 864133 describes what we're seeing. Not sure if it's a
> "regression" from this one on BMO, though, because this bug is still open --
> did this land as part of the changes last night?

as bmo's search is much closer to 4.4 than 4.2, so the 4.4r+'d patch landed on bmo yesterday.  i'll either try to fix negative flag lookups or get this backed out of bmo first thing next week (which is the earliest we can get changes pushed).

Comment 58

[:glob ✱]

what's going on here is 4.2 is doing "where there aren't any flags, or where the set flags do not contain X", while 4.4 is doing "where the set flags do not contain X".

Comment 59

[Frédéric Buclin]

Comment on attachment 740278 [details] [diff] [review]
patch for 4.2 v2

Why is this patch marked as obsolete? Has it been proven to be broken? And if yes, does it mean patches for 4.4 and trunk are broken too?

Comment 60

[Frédéric Buclin]

Comment on attachment 739443 [details] [diff] [review]
patch for xt

>=== modified file 'xt/lib/Bugzilla/Test/Search/FieldTest.pm'

>+sub debug_fail {

>+    return
>+        "   Value: '" . $self->translated_value . "'" . "\n" .

Nit: simply write "'\n".


>+        "Expected: [" . join(',', @expected) . "]\n" .
>+        " Results: [" . join(',', @results) . "]\n";
>+        trim($sql) . "\n";

I guess you meant to include trim($sql) in the returned string, in which case the previous line must end with . instead of ;


With your patch applied, I have several TODO tests which now pass:

  TODO passed:   2707, 2845, 6802, 6811, 6825, 6852, 6985
                13481, 57049, 57058, 68767, 68878, 68905
                72019

Don't you want to remove them from the broken list?


Anyway, r=LpSolit

Comment 61

[Frédéric Buclin]

Hum, I just realized that most of my saved searches no longer work, because they contain "flags is equal to review?" and with the new code, this only catches flags with no requestee. As most requests for review have a requestee, such queries now return no bugs or only bugs with no requestees. If I use "contains the string" instead of "is equal to", things are better, but they would also return flags such as superreview or doc-review.

Probably the logic must be changed again so that:

a) "Flags is equal to review?" only looks at the flag name + status, independently of the presence of a requestee.
b) "Flags is equal to review?[e-mail]" works as it does currently in the new code.

I think the right fix is to only join the "profiles as requestee" table and concatenate the requestee only if the requestee is detected in the value passed to _flagtypes_nonchanged(). This should be fixed as part of this bug. This part is easy.

About negative operators, this should be fixed as part of bug 864133. Now idea how hard it is to fix.

Comment 62

[:glob ✱]

(In reply to Frédéric Buclin from comment #59)
> Why is this patch marked as obsolete? Has it been proven to be broken? And
> if yes, does it mean patches for 4.4 and trunk are broken too?

yes, as per comment 54.

(In reply to Frédéric Buclin from comment #61)
> About negative operators, this should be fixed as part of bug 864133. Now
> idea how hard it is to fix.

that issue is actually a problem with searching multiple flags, not with negative flags.

Comment 63

[:glob ✱]

Attachment: patch v8

addresses the following issues:
- negated flag searches not working
- reverts requestee searching behavior
- fixes issues with searching for multiple flags (eg. "blocking+ and not approval+")

in order to address the multiple flag searching issue, i had to rethink the implementation, resulting in a switch from joins to subselects.

Comment 64

[Frédéric Buclin]

Comment on attachment 744037 [details] [diff] [review]
patch v8

This doesn't fix negative operators nor multiple flags.

If I write "flag:blocker+ -flag:approval+" in the QuickSearch box, all bugs having both blocker+ and approval+ flags are returned, because the SQL query remains the same as in bug 864133 comment 0.

If I write something similar in the Custom Search area, something similar is returned.

You cannot use EXISTS (SELECT 1 .... WHERE INSTR(CONCAT(...)) = 0) nor can you use
EXISTS (SELECT 1 .... WHERE CONCAT(...) != 'approval+'), because they will both catch other flags within the same bug.

You must write NOT EXISTS (SELECT 1 .... WHERE INSTR(CONCAT(...)) > 0) and
NOT EXISTS (SELECT 1 .... WHERE CONCAT(...) = 'approval+') instead.

Comment 65

[:glob ✱]

Attachment: patch v9

Comment 66

[Frédéric Buclin]

Comment on attachment 746261 [details] [diff] [review]
patch v9

Works fine. Thanks! :) r=LpSolit

Comment 67

[:glob ✱]

committed to trunk and 4.4, still working on the 4.2 backport:

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Search.pm
modified Bugzilla/Search/Condition.pm
modified Bugzilla/Search/Quicksearch.pm
Committed revision 8620.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
modified Bugzilla/Search.pm
modified Bugzilla/Search/Condition.pm
modified Bugzilla/Search/Quicksearch.pm
Committed revision 8553.

Comment 68

[:glob ✱]

Attachment: patch for 4.2 v3

Comment 69

[Frédéric Buclin]

Comment on attachment 749162 [details] [diff] [review]
patch for 4.2 v3

r=LpSolit

Comment 70

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified Bugzilla/DB.pm
modified Bugzilla/Search.pm
modified Bugzilla/DB/Oracle.pm
modified Bugzilla/Search/Clause.pm
modified Bugzilla/Search/Condition.pm
modified Bugzilla/Search/Quicksearch.pm
modified xt/lib/Bugzilla/Test/Search/Constants.pm
modified xt/lib/Bugzilla/Test/Search/FieldTest.pm
Committed revision 8214.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
modified xt/lib/Bugzilla/Test/Search/Constants.pm
modified xt/lib/Bugzilla/Test/Search/FieldTest.pm
Committed revision 8557.

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified xt/lib/Bugzilla/Test/Search/Constants.pm
modified xt/lib/Bugzilla/Test/Search/FieldTest.pm
Committed revision 8626.

Comment 71

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified xt/lib/Bugzilla/Test/Search/Constants.pm
Committed revision 8628.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
modified xt/lib/Bugzilla/Test/Search/Constants.pm
Committed revision 8559.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified xt/lib/Bugzilla/Test/Search/Constants.pm
Committed revision 8215.

Comment 72

[Daniel Veditz [:dveditz]]

I now have a problem the opposite of comment 61 -- I _want_ to find the flags with no requestee. My old queries have broken and I can't figure out a replacement.

The following query used to work to find bugs with the "sec-review?" flag which had no requestee (we need to make sure they were all assigned):

https://bugzilla.mozilla.org/buglist.cgi?o1=substring&v1=sec-review%3F&n3=1&f1=flagtypes.name&o3=isnotempty&query_format=advanced&f3=requestees.login_name

Note that's a "NOT is not empty", which I think we had to do when an earlier formulation broke -- possibly around the time Frédéric's queries broke. I've also tried things like 'NOT requestee contains "."' since all email addresses should have that.

This is a query we use in a weekly triage meeting so I don't know for sure it was broken by this morning's push (comment 70, push in bug 874759). It could have been broken previously and only coincidental that I noticed it today. But since it's the inverse of what you were trying to fix this bug seems likely :-)

Comment 73

[Frédéric Buclin]

It has never been possible to look for flags with no requestee. A semi-working workaround is to do:

Match ALL of the following separately:
Flags is equal to sec-review?
Flag Requestee doesn't contain the string @

The isempty patch hasn't been reviewed upstream yet. Maybe it's broken.

Comment 74

[Daniel Veditz [:dveditz]]

(In reply to Frédéric Buclin from comment #73)
> It has never been possible to look for flags with no requestee.

Except around the time of your comment 61? You complained about getting exactly the results I want :-)

> A semi-working workaround is to do:

Tried that (I didn't include all the alternatives I tried in comment 72), didn't work. Still get a zarro boogs query.

It seems that the "flags" field being matched isn't the same as the one the requestee is matched against. For example, I decided to try to do a two-query search -- first get all the bugs with sec-review? and then subtract the ones with mozilla.com requestees (that should be close enough for my purposes to manually sift through the remainder). However the query returns bugs I know have no sec-review? requestee--in one known case because it matches "mozilla.com" in a needinfo? flag.

The query _does_ say "match...separately" so I guess I can't entirely fault bugzilla for the results. But there seems to be no way to get the results I _do_ want which is to perform queries where the requestee and flag name terms refer to the same flag.

Can't do a match on the same field because the query page insists that flag and flag requestee are not the same field.

Comment 75

[Daniel Veditz [:dveditz]]

Quicksearch exhibits the same behavior

https://bugzil.la/sec-review%3Fnmaul%40mozilla

Returns bug 871669 (currently) which has a sec-review? flag with no requestee and a needinfo flag for nmaul

Comment 76

[Frédéric Buclin]

(In reply to Daniel Veditz [:dveditz] from comment #74)
> Except around the time of your comment 61? You complained about getting
> exactly the results I want :-)

Hehe, that was actually a bug in the committed patch. :)


> Tried that (I didn't include all the alternatives I tried in comment 72),
> didn't work. Still get a zarro boogs query.

WFM with the review flag.


> It seems that the "flags" field being matched isn't the same as the one the
> requestee is matched against.

Exactly. That's why I said "a semi-working workaround". What you want is bug 677757.

Comment 77

[Daniel Veditz [:dveditz]]

ah ha, a better workaround for me is to use request.cgi. Returns more data than I need, but by grouping on requestee the "none" bugs pop to the top. Close enough.

Comment 78

[Frédéric Buclin]

(In reply to Daniel Veditz [:dveditz] from comment #77)
> ah ha, a better workaround for me is to use request.cgi. Returns more data
> than I need, but by grouping on requestee the "none" bugs pop to the top.

Ah, if you are happy with request.cgi, simply type "-" in the requestee field. This means "no requestee":

 https://bugzilla.mozilla.org/request.cgi?action=queue&requestee=-&type=sec-review