sql: add support for scalars on the right of @> in index selection

[m-schneider]

Closes: #21783

Release note: None

[cockroach-teamcity]

This change is 

[RaduBerinde]

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.

---

pkg/sql/opt/index_constraints.go, line 806 at r1 (raw file):

// makeInvertedIndexSpansForExpr is analogous to makeSpansForExpr, but it is
// used for inverted indexes.
func (c *indexConstraintCtx) makeInvertedIndexSpansForExpr(offset int,

why do we need offset? it's always 0 for an inverted index

---

pkg/sql/opt/index_constraints.go, line 824 at r1 (raw file):

		switch rd.Type() {
		case json.ArrayJSONType, json.ObjectJSONType:
			break

[nit] move thereturn at the end here

---

Comments from Reviewable

[rytaft]

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks broke.

---

pkg/sql/opt_index_selection.go, line 537 at r1 (raw file):

func (ss *spanSorter) Less(i, j int) bool {
	// Compare start keys.
	return ss.spans[i].Key.Compare(ss.spans[j].EndKey) < 0

The comment says compare start keys, but it looks like you're comparing Key to EndKey...

---

pkg/sql/opt/index_constraints.go, line 827 at r1 (raw file):

		default:
			spans := make(LogicalSpans, 2)
			datum := rhs.private.(tree.Datum)

Isn't this the same as rightDatum above?

---

Comments from Reviewable

[m-schneider]

Please ignore the encoding change commit. This change needs to be on top of that commit in order to sort correctly.

---

Review status: 0 of 8 files reviewed at latest revision, 4 unresolved discussions.

---

pkg/sql/opt/index_constraints.go, line 806 at r1 (raw file):

Previously, RaduBerinde wrote…

why do we need offset? it's always 0 for an inverted index

Got rid of it.

---

pkg/sql/opt/index_constraints.go, line 824 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] move thereturn at the end here

Done.

---

pkg/sql/opt/index_constraints.go, line 827 at r1 (raw file):

Previously, rytaft wrote…

Isn't this the same as rightDatum above?

Good point!

---

pkg/sql/opt_index_selection.go, line 537 at r1 (raw file):

Previously, rytaft wrote…

The comment says compare start keys, but it looks like you're comparing Key to EndKey...

I got rid of the whole thing. After changing the inverted key encoding, everything sorts correctly on the logical span level!

---

Comments from Reviewable

[rytaft]

Reviewed 7 of 9 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

---

pkg/util/encoding/encoding.go, line 504 at r3 (raw file):

}

// encodeBytesAscendingWithTerminator encodes the []byte value using an escape-based

Fix comment to match function name

---

pkg/util/encoding/encoding.go, line 637 at r3 (raw file):

func prettyPrintInvertedIndexKey(b []byte) (string, []byte, error) {
	outBytes := ""
	tempB := b[1:]

Can you add a comment about why you're skipping the first byte?

---

pkg/util/encoding/encoding.go, line 788 at r3 (raw file):

}

// EncodeJSONAscending encodes a JSON Type. The encodes bytes are appended to the

encodes -> encoded

---

Comments from Reviewable

[RaduBerinde]

---

Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

---

pkg/sql/opt/index_constraints.go, line 822 at r3 (raw file):

			return LogicalSpans{c.makeEqSpan(0 /* offset */, rhs.private.(tree.Datum))}, true, true
		default:
			// If we find a scalar on the right side of the  @> operator it means that we need to find

[nit] extra space before @>

---

pkg/sql/opt/index_constraints.go, line 826 at r3 (raw file):

			// two logical spans, one for the original scalar and one for arrays containing the scalar.
			// This is valid because in JSON something can either be an array or scalar so the spans are
			// guaranteed not to overlap.

I would explain "overlap" more clearly. The spans themselves clearly don't overlap, but what we're saying here is that there can't be duplicate results (PKs) when we scan both.

---

pkg/sql/opt/index_constraints.go, line 828 at r3 (raw file):

			// guaranteed not to overlap.
			spans := make(LogicalSpans, 2)
			if !c.verifyType(0, rightDatum.ResolvedType()) {

This doesn't hurt but I don't think we need this check. We already know rightDatum is DJSON or we would have crashed above. If you keep it, add /* offset */ after 0.

---

Comments from Reviewable

[m-schneider]

Thanks!

---

Review status: 5 of 8 files reviewed at latest revision, 6 unresolved discussions.

---

pkg/sql/opt/index_constraints.go, line 822 at r3 (raw file):

Previously, RaduBerinde wrote…

[nit] extra space before @>

Done.

---

pkg/sql/opt/index_constraints.go, line 826 at r3 (raw file):

Previously, RaduBerinde wrote…

I would explain "overlap" more clearly. The spans themselves clearly don't overlap, but what we're saying here is that there can't be duplicate results (PKs) when we scan both.

Done.

---

pkg/sql/opt/index_constraints.go, line 828 at r3 (raw file):

/* offset */
That makes sense!

---

pkg/util/encoding/encoding.go, line 504 at r3 (raw file):

Previously, rytaft wrote…

Fix comment to match function name

Thanks for the comments, I'll address them on the original PR. It's at https://reviewable.io/reviews/cockroachdb/cockroach/21889 if you want to follow!

---

pkg/util/encoding/encoding.go, line 637 at r3 (raw file):

Previously, rytaft wrote…

Can you add a comment about why you're skipping the first byte?

Done.

---

pkg/util/encoding/encoding.go, line 788 at r3 (raw file):

Previously, rytaft wrote…

encodes -> encoded

Done.

---

Comments from Reviewable

[rytaft]

---

Reviewed 1 of 3 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

---

Comments from Reviewable