internal/rangekey: fix invariant check with onlySets=false

[itsbilal]

In the invariants build, with onlySets set to false (i.e. in ScanInternal), we would see RangeKeyDels in the coalescer that would sort to the end of the span.Keys when in BySuffixAsc order. This is expected and should not trip up the invariant panic, but it was.

[cockroach-teamcity]

This change is 

[itsbilal]

Found with #3044 .

[RaduBerinde]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @aadityasondhi and @itsbilal)

---

internal/rangekey/coalesce.go line 216 at r1 (raw file):

			}
			// Don't do a comparison with RangeKeyDeletes, as those will always sort
			// to the end and have no prefix.

s/prefix/suffix?

[itsbilal]

TFTR!

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @aadityasondhi)

---

internal/rangekey/coalesce.go line 216 at r1 (raw file):

Previously, RaduBerinde wrote…

s/prefix/suffix?

Done. Oops!

[jbowens]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @aadityasondhi and @itsbilal)

---

internal/rangekey/coalesce.go line 218 at r1 (raw file):

			// to the end and have no prefix.
			if i > 0 && ((a.Keys[i].Kind() != base.InternalKeyKindRangeKeyDelete && ui.comparer.Compare(a.Keys[i].Suffix, a.Keys[i-1].Suffix) < 0) ||
				(b.Keys[i].Kind() != base.InternalKeyKindRangeKeyDelete && ui.comparer.Compare(b.Keys[i].Suffix, b.Keys[i-1].Suffix) < 0)) {

Shouldn't RangeKeyDeletes have already been removed by UserIteratorConfig.Transform?

[itsbilal]

internal/rangekey/coalesce.go line 218 at r1 (raw file):

Previously, jbowens (Jackson Owens) wrote…

Shouldn't RangeKeyDeletes have already been removed by UserIteratorConfig.Transform?

Not if onlySets=false. See the above invariant conditional.

Merging for now. Let me know if something else doesn't make sense.

[jbowens]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion

---

internal/rangekey/coalesce.go line 218 at r1 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

Not if onlySets=false. See the above invariant conditional.

I see — can we update the comment at the beginning of ShouldDefragment and above it, and the comment above UserIteratorConfig.Transform?

I believe there's a bug with !onlySets: This function relies on the absence of RangeKeyUnsets and RangeKeyDeletes below (hence the original invariant above): If you have a span a-b:{(#2,RANGEKEYSET,@5)} and a span b-c:{(#2,RANGEKEYUNSET,@5)}, these two spans cannot be defragmented without losing information. If we defragment in the forward direction, we'll replace the second span's unset with a set. We should add a unit test for this case too.

Instead of using UserIteratorConfig.ShouldDefragment during scan internal, I think we should use a custom DefragmentMethod that includes a key kind comparison.

[itsbilal]

internal/rangekey/coalesce.go line 218 at r1 (raw file):

Previously, jbowens (Jackson Owens) wrote…

I see — can we update the comment at the beginning of ShouldDefragment and above it, and the comment above UserIteratorConfig.Transform?

I believe there's a bug with !onlySets: This function relies on the absence of RangeKeyUnsets and RangeKeyDeletes below (hence the original invariant above): If you have a span a-b:{(#2,RANGEKEYSET,@5)} and a span b-c:{(#2,RANGEKEYUNSET,@5)}, these two spans cannot be defragmented without losing information. If we defragment in the forward direction, we'll replace the second span's unset with a set. We should add a unit test for this case too.

Instead of using UserIteratorConfig.ShouldDefragment during scan internal, I think we should use a custom DefragmentMethod that includes a key kind comparison.

Good catch, this is problematic even though we haven't run into it yet. Filed #3058 so we get to it asap.