204903 - Cannot get buglists with certain search criteria matching NULL alias, no qa_contact, or no cc members

Status: RESOLVED FIXED

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

Description

[Joel Peshkin]

when search for bugs with a null alias (or bugs with an alias not matching
something), it is currently impossible to match any bugs having no alias with
alias-based criteria.

All negative boolean queries (does not contain, does not match, etc...) should
match records that match the criteria OR records that are NULL.

Comment 1

[Joel Peshkin]

I've done some experiments and I think that the right thing to do here is simply
to change all the instances of.....

 ",notregexp" => sub {
             $term = "LOWER($ff) NOT REGEXP $q";
to
 ",notregexp" => sub {
             $term = "((LOWER($ff) NOT REGEXP $q) OR ($ff IS NULL))";

and do the same for notsubstring, notequals, nowordssubstr, and nowords

I expect that this will optimize right out on fields that can never be NULL.

I would like a bbaetz opinion on database portability and a gerv opinion on the
UI semantics.

Comment 2

[Bradley Baetz (:bbaetz)]

Well, this means that all of the outer joins which mysql optimises to be inner
joins will have to remain outer joins.

Why IS NULL? '' is not the same as NULL, and we pull off the '' stuff earlier,
during the parsing stages.

Comment 3

[Joel Peshkin]

Well, this is really keeping aliases from working well now.

Would the right solution be to just have aliases not matching some criteria
indicate "OR alias IS NULL"  or would we be better off using IFNULL(alias,'')
whenever someone tries to include alias in a boolean chart?

Comment 4

[Bradley Baetz (:bbaetz)]

This doesn't stop |alias='foo'| searches. What we should do, is not strip out
the blank entries when parsing, and then have an extra hash somewhere to define
whether empty means NULL (alias), '' (summary), or <none-of> (groups, cc).

Comment 5

[Joel Peshkin]

A very similar issue applies to searching on qa_contact when that field is null
in some bugs. This should either be fixed in search or in both editcomponents
and sanity check otherwise sites that enable qa_contact after they have been
running for awhile will have some strange behavior.

Comment 6

[Ali Ebrahim]

(In reply to comment #0)
> All negative boolean queries (does not contain, does not match, etc...) should
> match records that match the criteria OR records that are NULL.

Not only that, but we should also be able to perform searches like the
following: UNCO/NEW/ASSI/REOP Firefox Downloading bugs where QA is blank (just
as an example). Right now we can't do that.

This is a pretty major feature that is missing, and would be great if we could
get in for Bugzilla 2.18.

Comment 7

[Joel Peshkin]

Attachment: Non-landable patch that does the trick

This is the patch I used to add the capability to boolean charts

Comment 8

[Joel Peshkin]

Attachment: partial patch for initial preview

This patch handles aliases by makign them not null.

The rest of the queries, qa_contact, cc, commenters, changedby, etc... will all
need to undergo the same transformation as is shown here for the simple cc
case.

mysql4 should optimize it enough that it will not be a big cpu hit.

Comment 9

[Joel Peshkin]

Attachment: patch v2

This patch addresses the complaints in this bug.  The rest will be handled in a
few other bugs as there are over 20 other cases that are hard to see without
doing an OR or chart negation.

Comment 10

[Jouni Heikniemi]

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

>Index: Bugzilla/Search.pm
>===================================================================
>-             $f = "map_$f.login_name";
>+             $f = "COALESCE(map_$f.login_name,'')";

COALESCE is pretty MySQL specific; is there anything more generic we could use?
I'm fine with this if there is no practically better alternative.

>-            push(@supptables, "LEFT JOIN profiles map_cc_$chartseq ON cc_$chartseq.who = map_cc_$chartseq.userid");
>-            $f = "map_cc_$chartseq.login_name";
>+            $ff = $f = "map_cc_$chartseq.login_name";
>+            my $ref = $funcsbykey{",$t"};
>+            &$ref;
>+            push(@supptables, "LEFT JOIN profiles map_cc_$chartseq ON cc_$chartseq.who = map_cc_$chartseq.userid AND ($term)");
>+            $term = "$f IS NOT NULL";

Can you please 1) wrap the lines to max 80 chars and 2) explain how this code
works? Also, I wouldn't mind reproduction instructions...

Comment 11

[Joel Peshkin]

There are 3 distinct mechanisms here.

1) alias can be null.  NULL REGEXP('^$') is not true.  Changing alias to a
not-null type makes that tyep fo query true. All you need to do is attempt to
search for bugs that never had aliases set on them to see this one.

2) If a bug does not have a qa contact, the query ...
http://landfill.bugzilla.org/bugzilla-tip/query.cgi?query_format=advanced&short_desc_type=allwordssubstr&short_desc=&long_desc_type=allwordssubstr&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&keywords=&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&emailassigned_to1=1&emailtype1=substring&email1=&emailassigned_to2=1&emailreporter2=1&emailcc2=1&emailtype2=substring&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&field0-0-0=short_desc&type0-0-0=notregexp&value0-0-0=food&field0-1-0=qa_contact&type0-1-0=notregexp&value0-1-0=.
should return the same results as ...
http://landfill.bugzilla.org/bugzilla-tip/query.cgi?query_format=advanced&short_desc_type=allwordssubstr&short_desc=&long_desc_type=allwordssubstr&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&keywords=&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&emailassigned_to1=1&emailtype1=substring&email1=&emailassigned_to2=1&emailreporter2=1&emailcc2=1&emailtype2=substring&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&field0-0-0=short_desc&type0-0-0=substring&value0-0-0=food
  But it does not

The third example will be a bit trickier.

Comment 12

[Joel Peshkin]

The third example...


http://landfill.bugzilla.org/bugzilla-tip/query.cgi?query_format=advanced&short_desc_type=allwordssubstr&short_desc=&long_desc_type=allwordssubstr&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&keywords=&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&emailassigned_to1=1&emailtype1=substring&email1=&emailassigned_to2=1&emailreporter2=1&emailcc2=1&emailtype2=substring&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&field0-0-0=short_desc&type0-0-0=substring&value0-0-0=food
returns 12 bugs

http://landfill.bugzilla.org/bugzilla-tip/query.cgi?query_format=advanced&short_desc_type=allwordssubstr&short_desc=&long_desc_type=allwordssubstr&long_desc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&status_whiteboard_type=allwordssubstr&status_whiteboard=&keywords_type=allwords&keywords=&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&emailassigned_to1=1&emailtype1=substring&email1=&emailassigned_to2=1&emailreporter2=1&emailcc2=1&emailtype2=substring&email2=&bugidtype=include&bug_id=&votes=&chfieldfrom=&chfieldto=Now&chfieldvalue=&field0-0-0=short_desc&type0-0-0=substring&value0-0-0=food&negate1=1&field1-0-0=cc&type1-0-0=regexp&value1-0-0=fdfghfdrrte&field1-1-0=short_desc&type1-1-0=substring&value1-1-0=food
returns only 7 bugs

even though the added crteria is supposed to be a criteria that can never be met.

Comment 13

[Joel Peshkin]

*** Bug 252439 has been marked as a duplicate of this bug. ***

Comment 14

[Jeroen Ruigrok van der Werven]

In response to #10:

PostgreSQL also has COALESCE, Oracle PLSQL also knows a coalesce function, 
Sybase also has it, as does DB2, MS SQL Server, and I'll leave it at that.

Comment 15

[Jeroen Ruigrok van der Werven]

Patch v2 looks alright in my eyes, but cannot test anything right now due to 
moving house (yet again).

Comment 16

[Jouni Heikniemi]

(In reply to comment #14)
> PostgreSQL also has COALESCE, Oracle PLSQL also knows a coalesce function, 
> Sybase also has it, as does DB2, MS SQL Server, and I'll leave it at that.

Sorry, I stand corrected if that's the way it is. I didn't really check, was
just writing out of my memory. Obviously a bad habit. 

Joel, can you still explain how the quoted section of cc code in comment 10 works?

Comment 17

[Joel Peshkin]

> 
> >-            push(@supptables, "LEFT JOIN profiles map_cc_$chartseq ON
cc_$chartseq.who = map_cc_$chartseq.userid");
> >-            $f = "map_cc_$chartseq.login_name";
> >+            $ff = $f = "map_cc_$chartseq.login_name";
> >+            my $ref = $funcsbykey{",$t"};
> >+            &$ref;
> >+            push(@supptables, "LEFT JOIN profiles map_cc_$chartseq ON
cc_$chartseq.who = map_cc_$chartseq.userid AND ($term)");
> >+            $term = "$f IS NOT NULL";
> 
> Can you please 1) wrap the lines to max 80 chars and 2) explain how this code
> works? Also, I wouldn't mind reproduction instructions...
> 

If I want to write a logical equation which is a combination of comparisons,
like "short_desc contains food", "priority in (p1,p2)", "cc contains justdave",
etc..tied together with boolean operators,AND, OR, etc..

(short_desc contains something) OR (priority in (p1,p2)
looks something like..
SELECT columns FROM bugs WHERE (the first thing) OR (the second thing)
and everyone is happy

The problem is, when that second thing is a join that can join in zero or more
records.
if we do this the way the code is today, we would get
SELECT columns FROM bugs,cc LEFT JOIN profiles ON profiles.id = cc.userid WHERE
bugs.id = cc.bug_id AND ( (the frst thing) OR (the second thing) )

If there were no CCs in the first place, that would make the row not match no
matter how many ORs are invokved.  It also keeps the results out of reach of
negation and uses the dreaded comma that worsens db compatibility.

This patch calls the same funcsbykey function to work out the conditional
operator, but takes the result and places it int he ON clause of a left join
rather than in the where clause and sticks the fieldname IS NOT NULL in the
WHERE clause.  That means, int his example, we get

SELECT columns FROM bugs LEFT JOIN cc ON bugs.id = cc.bug_id LEFT JOIN profiles
ON profiles.id = cc.userid ON (the first thing) WHERE ( (profiles.login_name IS
NOT NULL) OR (the second thing) )

This means that the function genrators all continue to work as they always have.
 If we decided to add a SOUNDEX match, it would just plain work.  All logic that
manipulates the boolean logic at the end also just plain works. Chart negation
(or row negation if we decided to add it) can operate on the wherepart without
worrying about what is the product of a join and what is a regular field from
the row.

mysql4 wil optmize the LEFT JOINs back into INNER JOINs if it finds that the IS
NOT NULL is effectively ANDed into the results.

 If other databases are found to be unable to optimize this, we could add logic
to know where it is safe to replace "LEFT" with "INNER".  That will be much
simpler once the code has become based on this new syntax.  We could even move
the function results to the where part under those same conditions.

Comment 18

[Jouni Heikniemi]

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

>Index: Bugzilla/Search.pm
>===================================================================

>-            push(@supptables, "LEFT JOIN profiles map_cc_$chartseq ON cc_$chartseq.who = map_cc_$chartseq.userid");
>-            $f = "map_cc_$chartseq.login_name";
>+            $ff = $f = "map_cc_$chartseq.login_name";
>+            my $ref = $funcsbykey{",$t"};
>+            &$ref;
>+            push(@supptables, "LEFT JOIN profiles map_cc_$chartseq ON cc_$chartseq.who = map_cc_$chartseq.userid AND ($term)");
>+            $term = "$f IS NOT NULL";

This is voodoo. No, make it Voodoo. But ok. Put () around the whole ON
condition for clarity and wrap the extra long push line.

>Index: checksetup.pl
>===================================================================
>+# 2004-07-22 - Keep alias non-null so that boolean queries can still work
>+ChangeFieldType('bugs', 'alias', 'varchar(20) not null');

When running checksetup, I'm getting "DBD::mysql::db do failed: Duplicate entry
'' for key 2 at checksetup.pl line 2340."; you need to replace null values with
'' first. You might also want to set |default ''| (both here and the initial
schema) to avoid any surprises elsewhere in the codebase. Also note that
process_bug refers to nullity of aliases by settings alias=NULL -- that will
fail after this patch.

Comment 19

[Joel Peshkin]

bleah!  now I remember why alias allowed nulls... otherwise we have to have the
code enforce unique.

I think the right way is to jsut change the query to tolerate nulls... I'll revise.

Comment 20

[Joel Peshkin]

Attachment: v3

Jouni is correct.  We can't let the field be nullable.

This lets search find the alias columns even when null.

Comment 21

[Joel Peshkin]

Attachment: patch - fix line wrap

Comment 22

[Jouni Heikniemi]

Comment on attachment 154319 [details] [diff] [review]
patch - fix line wrap

>Index: Bugzilla/Search.pm
>===================================================================
>+             $ff = "IF(bugs.alias IS NOT NULL, bugs.alias, '')";

As discussed on IRC, make this use COALESCE.

r=jouni

Comment 23

[Joel Peshkin]

*** Bug 191894 has been marked as a duplicate of this bug. ***

Comment 24

[Joel Peshkin]

checked in with suggested change

Comment 25

[Tyler]

(In reply to comment #24)
> checked in with suggested change

This suggests that the last patch in this bug is not what actually got applied
to cvs. Can the final patch be added here for those of us who add features to
released releases via patch can have the correct functionality?

Comment 26

[Dave Miller [:justdave]]

Attachment: final patch from cvs

This is the final patch that was checked into cvs, generated with:
cvs -q diff -u -r1.62 -r1.63 Bugzilla/Search.pm

(had to use bonsai to dig up the revision numbers for this patch, cuz no one
pasted them in here ;)