Redisearch: Nested and/or: Outer OR behaves as AND -> UNION/INTERSECT

[tw-bert]

I'm seeing weird behaviour with the scenario below.
I can work around it by splitting my client logic into 2 FT.SEARCH calls, but 1 call will be more efficient (plus, I'd like to know what's going on here).

redis-cli statements below, Redis 4.0.6, RediSearch 1.0.2.master.Dec20.2017.redis_index_csx_tw_fix (will upgrade to 1.0.3 soon __- released 1 hour ago -__, but should be nearly identical to 1.0.3)

FT.CREATE idxFoob STOPWORDS 0 SCHEMA iIDBrightnessSlots NUMERIC SORTABLE cSvBrinn TEXT NOSTEM iCollidingGroepno TAG SEPARATOR ";" cSvRamps TEXT NOSTEM cLockline TAG SEPARATOR ";"
FT.ADD idxFoob docFoob01 1.0 FIELDS iCollidingGroepno 7 cSvRamps _ cLockline _
FT.EXPLAIN idxFoob '((@cSvBrinn:30221)|(@iCollidingGroepno:{1|4|5|6|7|11|12|15|16|34|35|40|45|46|47|49|51|62|64}(@cSvRamps:\\_|@cSvRamps:01)@cLockline:{\\_|grinc}))' 'SORTBY' 'iIDBrightnessSlots' 'DESC' 'LIMIT' '0' '75'

gives:

@cSvBrinn:UNION {
  @cSvBrinn:UNION {
    @cSvBrinn:30221
    @cSvBrinn:+30221(expanded)
  }
  @cSvBrinn:INTERSECT {
    TAG:@iCollidingGroepno {
      1
      4
      5
      6
      7
      11
      12
      15
      16
      34
      35
      40
      45
      46
      47
      49
      51
      62
      64
    }
    @cSvRamps:UNION {
      @cSvRamps:UNION {
        @cSvRamps:_
        @cSvRamps:+_(expanded)
      }
      @cSvRamps:UNION {
        @cSvRamps:01
        @cSvRamps:+01(expanded)
      }
    }
    TAG:@cLockline {
      _
      grinc
    }
  }
}

So I would expect that when @cSvBrinn:30221 returns 0 documents, the search will continue.

Two searches (note that I only left component (@cSvBrinn:30221) out in the second search:

A-> FT.SEARCH idxFoob '((@cSvBrinn:30221)|(@iCollidingGroepno:{1|4|5|6|7|11|12|15|16|34|35|40|45|46|47|49|51|62|64}(@cSvRamps:\\_|@cSvRamps:01)@cLockline:{\\_|grinc}))' 'SORTBY' 'iIDBrightnessSlots' 'DESC' 'LIMIT' '0' '75'
B-> FT.SEARCH idxFoob '(                  (@iCollidingGroepno:{1|4|5|6|7|11|12|15|16|34|35|40|45|46|47|49|51|62|64}(@cSvRamps:\\_|@cSvRamps:01)@cLockline:{\\_|grinc}))' 'SORTBY' 'iIDBrightnessSlots' 'DESC' 'LIMIT' '0' '75'

Search B returns 1 doc.
Search A returns 0 docs, when it should return 1 doc.

[dvirsky]

Looks like some edge case when there is just one document, I'll investigate.

[tw-bert]

@dvirsky I simplified and anonymized a real scenario here, where around 5k records were involved. Hope you find something.

[dvirsky]

The problem is with the field selectors. You can notice that the second half of the query is @cSvBrinn:INTERSECT, it inherited the field selectors from the first part. I'm working on fixing this

[tw-bert]

@dvirsky Yes, looks like you are spot on. @cSvBrinn:INTERSECT should simply be INTERSECT, nicely shown at the top of FT.EXPLAIN -> B.

Small nag, unrelated: FT.EXPLAIN returns escaped newlines from redis-cli (I have to unescape the \n's to read the info). _edit:_ never mind, redis-cli most probably does the escaping

The way this problem can be spotted from a client is very nice indeed btw, you guys made an excellent choice to implement FT.EXPLAIN at an early stage. Problems like these can be a nightmare to debug otherwise. I missed it because of unfamiliarity with the syntax, glad you saw it.

[dvirsky]

Ok, after a lot of fiddling with how the filters are applied, I've managed to get to this which works;

UNION {
  @cSvBrinn:UNION {
    @cSvBrinn:30221
    @cSvBrinn:+30221(expanded)
  }
  INTERSECT {
    TAG:@iCollidingGroepno {
      1
      4
      5
      6
      7
      11
      12
      15
      16
      34
      35
      40
      45
      46
      47
      49
      51
      62
      64
    }
    @cSvRamps:UNION {
      @cSvRamps:UNION {
        @cSvRamps:_
        @cSvRamps:+_(expanded)
      }
      @cSvRamps:UNION {
        @cSvRamps:01
        @cSvRamps:+01(expanded)
      }
    }
    TAG:@cLockline {
      _
      grinc
    }
  }
}

Now the query parser tests fail and I need to go over them and see what other edge cases break, but I'll probably release this fix tomorrow.

[tw-bert]

@dvirsky I wouldn't call this an edge case though, but that's just terminology ;)

[dvirsky]

It only happens under specific conditions, not every union will have this. The problem now is that the fix affects more mainstream cases.

[tw-bert]

@dvirsky Ah, clear. Nested parsing can be hard to get right, good luck

[dvirsky]

The problem is my insistence on having the language be both structured and human friendly. Had I split it into two parsers, for general search and structured queries, we would have less edge cases. What's fucked now is the case @field:foo bar with no parentheses gets parsed as (@field:foo) (bar), but a human just inputting some text in a text box would expect it to be @field:(foo bar)

[tw-bert]

@dvirsky Clear, you didn't make it easy on yourself indeed. But I can see the added value. I might add that, as a human user, I quickly resorted to using parentheses because of undesired parsing results, and in general your human friendly parser is not very forgiving. To compare a Google search (I know this is not a fair comparison, but it's a good example), it won't return a parser error but will still try to guess what you might want. Notwithstanding that I hate that behaviour. Two parsers might still be an option, from code I can generate the outcome of FT.EXPLAIN quite easily. But that's a hard design decision.

[dvirsky]

I'm actually leaning towards making the search language simpler, and making the structured language just standard SQL WHERE expressions. Not implementing all of SQL, but the WHERE part is not that big.

[tw-bert]

Interesting, but quite a lot of work still. Personally, I like the current syntax and hope that you can iron out these edge cases.

[tw-bert]

You might also switch to more strict parsing if you detect any parentheses in the query string.

[dvirsky]

@tw-bert this was merged to master. I've verified that this specific case works now, and made some other changes to the parser to simplify the rules and hopefully make it more correct and less ambiguous.

It's merged to master, please try it. Also try your other complex queries as they might be affected. Although if anything we're less prone to such errors now.

[tw-bert]

@dvirsky Brilliant, I'm off from work but I'll test tm asap.

[tw-bert]

@dvirsky Tested, the patch works fine. All other queries we use still work, and return the exact same FT.EXPLAIN as before the patch.

One thing I noticed when making a mistake in my query (wrong index):
It's possible to get @NULL in FT.EXPLAIN output, when a field mapping could not be made.
FT.SEARCH does not fail in this scenario (but returns 0 results on INTERSECT, which is correct).

This might well be by design, but I thought I'd mention it anyway.

UNION {
  @NULL:UNION {
    @NULL:30221
    @NULL:+30221(expanded)
  }
  INTERSECT {
    TAG:@iFoo{
      1
      4
      5
    }
    @NULL:UNION {
      @NULL:UNION {
        @NULL:_
        @NULL:+_(expanded)
      }
      @NULL:UNION {
        @NULL:01
        @NULL:+01(expanded)
      }
    }
    TAG:@cFoo {
      _
      foo
    }
  }
}

I hope you can release a 1.0.4 with this patch included soon.

[dvirsky]

The NULL thing is by design, it just means "unknown field". Thanks for
verifying, I'll release it on Sunday.

On Fri, Jan 5, 2018, 10:06 AM tw-bert [e-mail] wrote:

@dvirsky https://github.com/dvirsky Tested, the patch works fine. All
other queries we use still work, and return the exact same FT.EXPLAIN as
before the patch.

One thing I noticed when making a mistake in my query (wrong index):
It's possible to get @NULL in FT.EXPLAIN output, when a field mapping
could not be made.
FT.SEARCH does not fail in this scenario (but returns 0 results on
INTERSECT, which is correct).

This might well be by design, but I though I'd mention it anyway.

UNION {
@NULL:UNION {
@NULL:30221
@NULL:+30221(expanded)
}
INTERSECT {
TAG:@iFoo{
1
4
5
}
@NULL:UNION {
@NULL:UNION {
@NULL:_
@NULL:+_(expanded)
}
@NULL:UNION {
@NULL:01
@NULL:+01(expanded)
}
}
TAG:@cFoo {
_
foo
}
}
}

I hope you can release a 1.0.4 with this patch included soon.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/RedisLabsModules/RediSearch/issues/253#issuecomment-355495507,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAetujyvBFZszTatZ3CF3COwHzxrB_dmks5tHdgNgaJpZM4RRxog
.