While digging into #1400940: "IN" condition missing to create a filter query I discovered what appears to be a logic error in BackendTest:searchSuccess1(). The NOT IN test expects 4 results for NOT IN using 'grape' and 'apple', but looking at the sample data:

    $this->entities[1] = $entity_test_storage->create(array(
      'name' => 'foo bar baz foobaz föö smile' . $smiley,
      'body' => 'test test case Case casE',
      'type' => 'item',
      'keywords' => array('Orange', 'orange', 'örange', 'Orange', $smiley),
      'category' => 'item_category',
    ));
    $this->entities[1]->save();
    $this->entities[2] = $entity_test_storage->create(array(
      'name' => 'foo test foobuz',
      'body' => 'bar test casE',
      'type' => 'item',
      'keywords' => array('orange', 'apple', 'grape'),
      'category' => 'item_category',
    ));
    $this->entities[2]->save();
    $this->entities[3] = $entity_test_storage->create(array(
      'name' => 'bar',
      'body' => 'test foobar Case',
      'type' => 'item',
    ));
    $this->entities[3]->save();
    $this->entities[4] = $entity_test_storage->create(array(
      'name' => 'foo baz',
      'body' => 'test test test',
      'type' => 'article',
      'keywords' => array('apple', 'strawberry', 'grape'),
      'category' => 'article_category',
      'width' => '1.0',
    ));
    $this->entities[4]->save();
    $this->entities[5] = $entity_test_storage->create(array(
      'name' => 'bar baz',
      'body' => 'foo',
      'type' => 'article',
      'keywords' => array('orange', 'strawberry', 'grape', 'banana'),
      'category' => 'article_category',
      'width' => '2.0',
    ));
    $this->entities[5]->save();

Only entities 1 and 3 have neither grape nor apple in their keywords.

Since this test passes for the database backend, I think there is a bug in that backend's NOT IN query logic too.

CommentFileSizeAuthor
#9 2697201-9--NOT_IN_multivalued--simple_solution.patch2.3 KBdrunken monkey
#9 2697201-9--NOT_IN_multivalued.patch6.62 KBdrunken monkey
#9 2697201-9--NOT_IN_multivalued--interdiff.txt5.55 KBdrunken monkey
#9 2697201-9--NOT_IN_multivalued--simple_solution--interdiff.txt1.23 KBdrunken monkey
#3 2697201-03.patch1.07 KBjhedstrom

Comments

jhedstrom created an issue. See original summary.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

Confirmed there's an issue with how the database backend handles NOT IN queries:

SELECT DISTINCT t.item_id AS item_id, :score AS score
FROM 
{search_api_db_database_search_index} t
LEFT OUTER JOIN {search_api_db_database_search_index_keywords} t_2 ON t.item_id = t_2.item_id
WHERE ( (t_2.value NOT IN  (:db_condition_placeholder_0, :db_condition_placeholder_1)) )

Since each keyword is stored in a separate row:

+-------------------------+------------+
| item_id                 | value      |
+-------------------------+------------+
| entity:entity_test/1:en | Orange     |
| entity:entity_test/1:en | ὠ1         |
| entity:entity_test/2:en | apple      |
| entity:entity_test/2:en | grape      |
| entity:entity_test/2:en | orange     |
| entity:entity_test/4:en | apple      |
| entity:entity_test/4:en | grape      |
| entity:entity_test/4:en | strawberry |
| entity:entity_test/5:en | banana     |
| entity:entity_test/5:en | grape      |
| entity:entity_test/5:en | orange     |
| entity:entity_test/5:en | strawberry |
+-------------------------+------------+

This query still returns results for entities that do have the values it is supposed to exclude.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Active » Needs review
StatusFileSize
new 2697201-03.patch1.07 KB

This fixes just the test (which will fail for the db backend).

Status: Needs review » Needs work

The last submitted patch, 3: 2697201-03.patch, failed testing.

The last submitted patch, 3: 2697201-03.patch, failed testing.

stborchert’s picture

stborchert
he/him
Primary language German
commented

I created the test and also stumbled across this (at first sight) wrong behavior.
But if you look at the query, the resulting 4 items are totally correct. Using LEFT OUTER JOIN in combination with NOT IN will result in strange results if NULL values are involved (there are several post about this and how to avoid this, i.e. by using NOT EXISTS).
Unfortunately this will require a completely different query for the NOT IN operator without using the join. I'm not sure, this is possible at all.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

Not including the NULL (entity 3) may or may not work, but surely the other entities that have either of the values should not be included in the result set? Solr is returning entities 1 and 3 using a NOT IN query. I may have some time today to look into how Views handles this (which IIRC would exclude the entities).

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

This is how Views does a NOT IN query:

LEFT JOIN {node__field_tags} node__field_tags ON node_field_data.nid = node__field_tags.entity_id AND (node__field_tags.field_tags_target_id = '2' OR node__field_tags.field_tags_target_id = '6')
WHERE (( (node__field_tags.field_tags_target_id IS NULL) )

It does a join on the conditions (eg, the ones to exclude), and then filters by making sure the returned values have no rows in the joined table.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs work » Needs review
StatusFileSize
new 2697201-9--NOT_IN_multivalued--simple_solution--interdiff.txt1.23 KB
new 2697201-9--NOT_IN_multivalued--interdiff.txt5.55 KB
new 2697201-9--NOT_IN_multivalued.patch6.62 KB
new 2697201-9--NOT_IN_multivalued--simple_solution.patch2.3 KB

Thanks for reporting this issue!
I first had to think about this a bit, it seems to me like both interpretations could, on a technical level, be justified, so it's disputable whether our current code isn't already correct.

However, in the end I have to agree with you: your interpretation is pretty surely what most users would expect, too, so we should definitely fix this.

However, we already have this working correctly for normal <> conditions, so all we have to do is treat NOT IN the same way – see my "simple_solution" patch above.

It does a join on the conditions (eg, the ones to exclude), and then filters by making sure the returned values have no rows in the joined table.

Huh, that seems like a clever solution, avoiding the (in my estimate) more expensive nested query we're currently using.
So, I've used this opportunity to rewrite that part of the createDbCondition() method entirely to, hopefully, make more sense, and also perform better in this scenario. (Seems we previously, in addition to the nested query, also did a JOIN we then weren't using, for negative conditions on multi-valued fields.)

Please test/review!

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs review » Reviewed & tested by the community

Thanks! I've tested both approaches and each works. I haven't had time to run any sort of analysis on the performance implications, but since the non-simple solution is more in keeping with how Views does this, I'd say that approach should be used.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Fixed

OK, then let's use that. Thanks a lot for reviewing – and for writing the test, of course!
Committed.

  • drunken monkey committed 97c5168 on 8.x-1.x 
    Issue #2697201 by drunken monkey, jhedstrom: Fixed DB backend handling...
drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Reviewed & tested by the community » Fixed

Correcting issue status, see #2698635: Issue statuses changing by themselves?.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.