sql: out of range routingIndex is produced in inverted filterer

[angelazxu]

Describe the problem

While running some logic tests for inverted index support for fetch/contains expressions (#63048), the following query leads to an index out of range error:

SELECT j FROM f@i WHERE j->'a' @> '"b"' AND j->'a' <@ '["b", "c", "d", "e"]' ORDER BY k

This is due to the b.routingIndex in prepareAddIndexRow being set to -1, (line 521, inverted_expr_evaluator.go). This is a quick fix, however I'm not sure if this issue could be reproduced on the current master and the fix would need to be backported.

[angelazxu]

@sumeerbhola I'm not too familiar with how the execution engine works, do you think this would need to be backported / if this bug could happen in other queries?

[yuzefovich]

Angela and I debugged this a little, and we think that the problem might be here:

cockroach/pkg/sql/rowexec/inverted_filterer.go

Line 250 in e7f7244

if _, err = ifr.invertedEval.prepareAddIndexRow(enc, nil /* encFull */); err != nil {

prepareAddIndexRow as the first parameter returns a boolean indicating whether addIndexRow should be called, but in the link above we're ignoring it. I think this is very suspicious.

[yuzefovich]

I opened up #63196 that prototypes the fix as I understand it, but it is missing a regression test (I couldn't repro on master what Angela saw on her branch; possibly the issue is only present because of the improvements by Angela recently). I'd appreciate some help with coming up with the test.

[yuzefovich]

@angelazxu could you please find a reproduction of this problem on master (i.e. without the changes in your PR)? I believe the bug can be exposed pretty easily, but I don't have the relevant context. Quite possibly modifying some existing query/data in inverted_index logic test file should be sufficient.

[yuzefovich]

@sumeerbhola do you think this issue should be a GA-blocker? I believe the impact is that if the flow is vectorized, an internal error might be encountered, if the flow is row-based, then the node might crash; but I don't know the likelihood of hitting this problem. cc @rytaft

[yuzefovich]

The current hypothesis proposed by Becca in #63048 (review) is that invalid JSON encoding could have been the root cause for the panic, and it aligns with what Sumeer wrote in #63196 (review). So we'll treat this issue as a non-blocker for now.

[rytaft]

Looks like my hypothesis that this was due to invalid JSON was wrong. I also managed to find a simple reproduction that doesn't require any of @angelazxu's new stuff, indicating that this bug exists in prior releases:

CREATE TABLE f (
  j JSON,
  INVERTED INDEX i (j)
);

INSERT INTO f VALUES ('{"a": "b", "x": "y"}}');

SELECT j FROM f@i WHERE j @> '{"a": "c"}' AND (j @> '{"a": "b"}' OR j @> '{"a": "c"}');

I think this can be explained by the fact that there are more spans scanned from the inverted index than are needed by the inverted filterer, as shown here:

> EXPLAIN (opt) SELECT j FROM f@i WHERE j @> '{"a": "c"}' AND (j @> '{"a": "b"}' OR j @> '{"a": "c"}');
                                         info
--------------------------------------------------------------------------------------
  index-join f
   └── inverted-filter
        ├── inverted expression: /5
        │    ├── tight: true, unique: false
        │    └── union spans: ["7a\x00\x01\x12c\x00\x01", "7a\x00\x01\x12c\x00\x01"]
        └── scan f@i
             ├── inverted constraint: /5/1
             │    └── spans
             │         ├── ["7a\x00\x01\x12b\x00\x01", "7a\x00\x01\x12b\x00\x01"]
             │         └── ["7a\x00\x01\x12c\x00\x01", "7a\x00\x01\x12c\x00\x01"]
             └── flags: force-index=i

I'm guessing the fix should actually be in sql/inverted/expression.go to ensure that the extra span is removed from SpansToRead. I'll work on a fix.

[rytaft]

I'm going to add the GA-Blocker label since I think 21.1 is the first release that we are using invertedFilterer and invertedJoiner for JSON and Array expressions.

[blathers-crl]

Hi @rytaft, please add branch-* labels to identify which branch(es) this release-blocker affects.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}