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

-- commits, line 8 at r1:
nit: s/Decodedd/Decoded/.

pkg/sql/sem/tree/datum.go, line 1626 at r1 (raw file):

// Format implements the NodeFormatter interface.
func (d *DEncodedKey) Format(ctx *FmtCtx) {
	f := ctx.flags

nit: it seems like it'd be good to extract a helper to be shared by two Bytes datums.

pkg/sql/sem/tree/datum_alloc.go, line 151 at r1 (raw file):

}

// NewDEncodedKey allocates a DBytes.

nit: s/DBytes/DEncodedKey/.

Great idea!

To prevent poor row count estimations I think we'll need to include the new type you've created here:

Otherwise, if a query filters by a value in the middle of a histogram bucket, the estimated row count will be the bucket's num_range divided by two.

Does the histo_col_type field need to change in table statistics? Or is it fine to leave that as BYTES for inverted index columns.

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

pkg/sql/types/types.go, line 468 at r1 (raw file):

	// EncodedKey is a special type used internally for inverted index keys, which
	// do not fully encode an object.
	EncodedKey = &T{

nit: Unless you think it's likely this will be used for other purposes in the future, it might make sense to make the name of the type and family more specific to inverted index keys.

The statistics creation and props code still uses DBytes (props.makeSpanFromInvertedSpan), I don't think it's worth changing that. I will add some more comments around the code though.

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

pkg/sql/types/types.go, line 468 at r1 (raw file):

I think I'd like to keep it, it's a sound concept of its own, maybe there will be some use case for passing around encoded keys. I could imagine potential usecases around composite types like collated strings.

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

pkg/sql/sem/tree/datum.go, line 1626 at r1 (raw file):

I just "rerouted" to the DBytes implementation.

The statistics creation and props code still uses DBytes (props.makeSpanFromInvertedSpan), I don't think it's worth changing that. I will add some more comments around the code though.

SGTM. Thanks!

Reviewed 4 of 5 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner, @RaduBerinde, @rytaft, and @yuzefovich)

pkg/sql/sem/tree/datum.go, line 1564 at r2 (raw file):

}

// DEncodedKey is the bytes Datum. The underlying type is a string because we want

nit: DEncodedKey is not the bytes datum. It'd be helpful to explain the contrast between the type and DBytes here.

pkg/sql/types/types.go, line 468 at r1 (raw file):

Sounds good to me. In that case, I think the comment should be more general and explain the behavior of the type (e.g. "EncodedKey is similar to DBytes but has a different behavior for encoding and decoding ...") before mentioning the inverted index key use-case.

TFTRs!

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

pkg/sql/types/types.go, line 468 at r1 (raw file):

Done, PTAL

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

pkg/sql/types/types.go, line 468 at r1 (raw file):

Looks great thanks!