mmaprototype: add more tests for err in TestNormalizedVoterAllRelatio…

[wenyihu6]

Epic: CRDB-55052
Release note: none

---

mmaprototype: add more tests for err in TestNormalizedVoterAllRelationships

---

mmaprototype: improve comment on relationship

---

mma: additional case for non-intersecting conjunctions

A=[+region=a, +zone=a1], B=[+region=a, +zone=a2] is now classified as
non-intersecting. This fixes an existing todo.

---

mmaprototype: fix conjNonIntersecting

Previously, we started returning conjNonIntersecting when we encounter
same type, same key, different values. However, the fix only returned
early if the differing value is encountered from cc. This commit changed
it so that we also check and return early for b.

---

mmaprototype: add test cases for conjNonIntersecting

[cockroach-teamcity]

This change is 

[wenyihu6]

@sumeerbhola I ended up cherry-picking your commit and added more tests as part of the work. So I will close #158380 in favor of this one if it looks good to you.

[tbg]

Good! I got confused by the less function, maybe this can be clarified a bit more. I think once this comment leads me a bit more through the "why" and "what" instead of just the how of this main (i,j) loop, the comments will be great.

@tbg reviewed 1 of 1 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola and @wenyihu6)

---

-- commits line 14 at r4:
Surprised there isn't a test update here?

edit: ah, next commit adds it.

---

pkg/kv/kvserver/allocator/mmaprototype/testdata/TestNormalizedVoterAllRelationships line 224 at r1 (raw file):

 voter-constraints:
   +region=us-west:2
normalization error: constraint replicas add up to more than configured replicas

so this is a hard error (nil normalized constraints returned) whereas the previous test case does return constraints but also an error?

---

pkg/kv/kvserver/allocator/mmaprototype/constraint.go line 229 at r2 (raw file):

	inBoth := 0

	// Example: cc = [A, C, E] and b = [B, D, E]:

This all sounds "simple" until I think about what it means and then it gets confusing (to me).
What does A < B mean? We're incrementing extraInCC, so it must mean "A is not present in B". Does this in turn mean "A is not stricter than B"? And is that then really only saying that they are not related at all when we consider the sets of things satisfying each one respectively (poset order)?

A < B really suggests "A is stricter than B" to me. I might be on the wrong track. Looking at the impl of less leaves me wondering what it does:

cockroach/pkg/kv/kvserver/allocator/mmaprototype/constraint.go

Lines 115 to 126 in 5d36212

	func (ic internedConstraint) less(b internedConstraint) bool {
	if ic.typ != b.typ {
	return ic.typ < b.typ
	}
	if ic.key != b.key {
	return ic.key < b.key
	}
	if ic.value != b.value {
	return ic.value < b.value
	}
	return false
	}

It seems that +anything < -anyotherthing, for example. This is definitely not a poset ordering - it's more or less an arbitrary total order. So its precise semantics probably don't matter. All it should really care about is how many elements are equal. But then the loop does act on the "sign" of less, so it must matter at least a little bit. Need to think about it more, just pointing out something that can maybe be explained here to lead readers down the right track.