opt: inverted-index accelerate filters of the form j = ... and j IN (...) #101178

Shivs11 · 2023-04-10T22:27:17Z

An inverted index can be used to accelerate queries, using the = and the IN operator,
with filters having the fetch value operator present;
eg: j->0 = '{"b": "c"}' and j->0 IN ('1', '2')

where j is a JSON column.
This has been done completed here: #96471 and #94666

This PR aims to add support to generate inverted spans for queries not involving the
fetch val operator and having the = operator or the IN operator. This was done by
creating JSON objects from the JSON values present on the right side of the equality or
the IN expression. Moreover, in this case, no "keys" were extracted from the left hand
side of the operators while creating these JSON objects which were useful for creating the
inverted spans.

Epic: CRDB-3301

Fixes: #96658

Release note (performance improvement): The optimizer now plans
inverted index scans for queries that do not use the JSON fetch value operator (->) alongside
the IN and the = operators. eg: json_col = '{"b": "c"}' OR json_col IN ('"a"', '1')

cockroach-teamcity · 2023-04-10T22:27:29Z

This change is

Shivs11 · 2023-04-11T14:11:22Z

bors r+

rytaft · 2023-04-11T14:51:12Z

bors r-

rytaft · 2023-04-11T14:51:29Z

This hasn't been reviewed yet. I think maybe you meant to bors the other PR?

Shivs11 · 2023-04-11T15:03:27Z

This hasn't been reviewed yet. I think maybe you meant to bors the other PR?

Ah my apologies. Changes have been made.

rytaft

Nice work!

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @Shivs11)

-- commits line 7 at r1:
I don't understand this last sentence

pkg/sql/opt/invertedidx/json_array.go line 591 at r1 (raw file):

	// on which an inverted index is constructed. Other entries inside of this
	// index, such as JSON arrays and objects, may consist of the element 1 present
	// within them. Since the encodings inside the index do not contain the position

nit: "may consist of the element 1 present within them" sounds a bit convoluted. I'd change this to "may contain the element 1 along with other elements" or something like that.

pkg/sql/logictest/testdata/logic_test/inverted_index line 829 at r1 (raw file):


query T
SELECT j FROM f@i WHERE j = '{"a": "a"}' ORDER BY k

this is a duplicate of the one above it

Shivs11

Thank you for the review @rytaft !

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner and @rytaft)

-- commits line 7 at r1:

Previously, rytaft (Rebecca Taft) wrote…

I don't understand this last sentence

Replaced this with the intended PR numbers.

pkg/sql/opt/invertedidx/json_array.go line 591 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: "may consist of the element 1 present within them" sounds a bit convoluted. I'd change this to "may contain the element 1 along with other elements" or something like that.

I agree. Done.

pkg/sql/logictest/testdata/logic_test/inverted_index line 829 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

this is a duplicate of the one above it

Removed.

mgartner

Reviewed 1 of 5 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @rytaft and @Shivs11)

-- commits line 7 at r1:

Previously, Shivs11 (Shivam) wrote…

Replaced this with the intended PR numbers.

Did you push your latest changes? The last sentence looks incomplete: "This has been completed here:"

-- commits line 2 at r2:
nit: Should "and" be "or" here?

-- commits line 13 at r2:
I don't understand this last sentence. Are you suggesting that keys from the LHS were useful in creating inverted spans previously, but no longer are useful? Or simply that there are not keys on the LHS?

-- commits line 21 at r2:
It's odd to write this with the negative perspective: "queries that do not use...". I think you can word it something like "for queries of this form..." to be less confusing.

pkg/sql/opt/invertedidx/json_array.go line 437 at r2 (raw file):

		fetch = false
	default:
		return nil

nit: I think you should return inverted.NonInvertedColExpression here to match the comment for the function and the rest of the function.

pkg/sql/opt/invertedidx/json_array.go line 562 at r2 (raw file):

}

// extractJSONFetchValEqCondition extracts an InvertedExpression representing an

nit: incorrect function name

pkg/sql/opt/invertedidx/json_array.go line 582 at r2 (raw file):

	// For Equals expressions, we will generate the inverted expression for the
	// single object built from the keys and val.
	invertedExpr := getInvertedExprForJSONOrArrayIndexForContaining(ctx, evalCtx, val)

See my comment in the tests - I'm skeptical that we can use getInvertedExprForJSONOrArrayIndexForContaining in this case.

pkg/sql/opt/xform/testdata/rules/select line 2808 at r2 (raw file):

      │         │    └── spans
      │         │         ├── ["7\x00\x01\x00", "7\x00\x01\x00"]
      │         │         └── ["7\x00\x03\x00\x01\x00", "7\x00\x03\x00\x01\x00"]

We should only have a single span here, right? AFAIK there's only one possible JSON path for the JSON value 1. I think it's because we generating spans with getInvertedExprForJSONOrArrayIndexForContaining. "Containment" is different than "equality", and I believe it generates two spans to handle the case when the JSON value is an array containing the value 1.

pkg/sql/opt/invertedidx/json_array_test.go line 1089 at r2 (raw file):

			ok:               true,
			tight:            false,
			unique:           false,

Ideally, this should be unique=true, correct? Is it possible for the generated spans to scan two entries with the same PK? I don't think it should be, but maybe it's due to the use of "containment" spans not "equality" spans (see my other comments).

Shivs11

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner and @rytaft)

-- commits line 7 at r1: