237107 - Boolean chart with attachment-specific request hides results without attachments

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect, P3)

Description

[Andreas Höfler]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7a) Gecko/20040106 MultiZilla/1.6.2.1d
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7a) Gecko/20040106 MultiZilla/1.6.2.1d

When using boolean charts query with something related to attachments (ie.
"attachment is patch" in the example url) as an OR-requirement, all other
requirements are enhanced implicitely to also have attachments on the bugs.

In the example search, I query for bugs which have something in the status
whiteboard OR an attachment, which is a patch.

Reproducible: Always
Steps to Reproduce:
1. Set up query with boolean charts to search for 
 a) something in the status_whiteboard 
 OR 
 b) an attachment, which is a patch
2. Query


Actual Results:  
- Bugs having attachments which are patches
- Bugs having something in the status_whiteboard AND PATCHES ADDED (not
neccessarily patches)

There are no bugs which just have something on the status_whiteboard but no
attachment added.

Expected Results:  
- Bugs having attachments which are patches
- Bugs having something in the status_whiteboard

When querying for something related to attachments with boolean charts, the
underlying SQL-select gets extended with the following line:
...
  AND bugs.bug_id = attachments_0.bug_id 
...

The actual boolean chart-query is two lines below:
...
  AND ((LOWER(bugs.status_whiteboard) REGEXP '.+') OR (attachments_0.ispatch =
'1')) 
...

So the need for attachments is implicitely added to all requirements, even if
they are added with OR.

Comment 1

[Dave Miller [:justdave]]

That should be a left join on the attachments table instead of an implicit inner
join...

I'd rather not screw with the search code on the 2.16 branch.  2.18 is possible
if we get a patch soon.

Comment 2

[Dave Miller [:justdave]]

Attachment: Patch v1

Comment 3

[Joel Peshkin]

Comment on attachment 149503 [details] [diff] [review]
Patch v1

The LEFT JOIN needs the bugs.bug_id = $table.bug_id moved to the ON clause.  It
is still in @wherepart

Comment 4

[Dave Miller [:justdave]]

Attachment: Patch v2

duh.

Comment 5

[Joel Peshkin]

Actually, it's worse than that.

Every condition we want on the attachment needs to be in the ON clause instead
of in @wherepart

Rather than creating a special case for each verb, we should find a way to call
the functions already defined.

Comment 6

[Joel Peshkin]

This is going to be a pain to fix....

What needs to happen is that the code ...
    foreach my $str (@supptables) {
        if (!$suppseen{$str}) {
            if ($str !~ /^(LEFT|INNER) JOIN/i) {
                $suppstring .= ",";
            }
            $suppstring .= " $str";
            $suppseen{$str} = 1;
        }
    }

needs to combine multiple LEFT JOIN statements into one that merges the
non-identical ON criteria.

Comment 7

[Joel Peshkin]

Once bug 245158 lands, this can be done, but ALL of the criteria need to move
from @wherepart to the ON section.  Then, @wherepart becomes just a
<table>.<field> IS NOT NULL.

This sets us up quite nicely to do negation in boolean charts as soon as we
figure out how to make the UI comprehensible because we can do the same thing
with IS NULL instead of IS NOT NULL and find bugs where, for example, there is
no matching CC record (as opposed to a CC record where a particular condition is
not met).

Comment 8

[Joel Peshkin]

The cleanest way to do this is to make sure that all of the one-to-many joined
tables are done with LEFT JOIN and keep the criteria in the ON clause.

I am not sure if that could have a performance impact (effecting only the cases
where the query includes comments, attachments, or CC).  If so, we could make it
switch between join types depending on whether there is an OR involved or not. 
However, I doubt that is worth the mess.  If we do that, we should just switch
between LEFT JOIN and INNER JOIN rather than using the comma.  We would have to
require mysql 3.23.17 or better so that we can do 
INNER JOIN table AS alias ON condition
and have the condition behave just as if it is in @wherepart

Comment 9

[Dave Miller [:justdave]]

(In reply to comment #8)
> We would have to require mysql 3.23.17 or better so that we can do 
> INNER JOIN table AS alias ON condition
> and have the condition behave just as if it is in @wherepart

We already require MySQL 3.23.41, so that's not a problem.

Comment 10

[Joel Peshkin]

I suspect that we won't have to make that change since it looks like mysql will
figure out when it can get away with changing the left join back to a regular
join anyway.  As soon as bug 245158 lands, you should be able to go ahead and
land this one.

Comment 11

[Max Kanat-Alexander]

So, that bug has landed. Any progress on this?

Comment 12

[Max Kanat-Alexander]

This bug has not been touched by its owner in over six months, even though it is
targeted to 2.20, for which the freeze is 10 days away. Unsetting the target
milestone, on the assumption that nobody is actually working on it or has any
plans to soon.

If you are the owner, and you plan to work on the bug, please give it a real
target milestone. If you are the owner, and you do *not* plan to work on it,
please reassign it to [e-mail] or a .bugs component owner. If you are
*anybody*, and you get this comment, and *you* plan to work on the bug, please
reassign it to yourself if you have the ability.

Comment 13

[Dave Miller [:justdave]]

Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.

Comment 14

[Frédéric Buclin]

Fixed in 4.2, but not in 4.0 and older. I don't know which patch fixed the problem exactly, but I suspect this is one of mkanat ones while refactoring Search.pm.