partitionccl: make CreatePartitioning idempotent for implicit columns

[otan]

Previously, calling CreatePartitioning twice on an implicitly
partitioned index would un-implicitly partition that index. This commit
changes CreatePartitioning to not un-implicitly partition the index in
such a case.

Release note: None

[cockroach-teamcity]

This change is 

[arulajmani]

Thanks for making this change!

Couple notes:

• Can we add some tests in partitioning_implicit that test this stuff directly as well
• Can we cleanup some of the complexity in alter_primary_key around altering to/from REGIONAL BY ROW tables? I'm thinking around here:

  cockroach/pkg/sql/alter_primary_key.go

  Line 450 in 5744288

  // Detect partitioning if we are newly adding a PARTITION BY ALL statement.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)

---

pkg/ccl/partitionccl/partition.go, line 394 at r1 (raw file):

			)
		}
		for i, oldColID := range oldImplicitColumnIDs {

Just confirming, we don't need to check for the direction of implicit columns right? There's now way that could change?

[otan]

Can we add some tests in partitioning_implicit that test this stuff directly as well

What tests would do this? We block everywhere you try to change an implicitly partitioned column when using ALTER INDEX, and that's already tested.

Can we cleanup some of the complexity in alter_primary_key around altering to/from REGIONAL BY ROW tables? I'm thinking around here:

No, because in that case we are changing the implicitly partitioned columns to something else. But we are rewriting the index, so it's okay.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)

---

pkg/ccl/partitionccl/partition.go, line 394 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Just confirming, we don't need to check for the direction of implicit columns right? There's now way that could change?

Nope -- all implicitly partitioned column is ascending

[arulajmani]

We block everywhere you try to change an implicitly partitioned column when using ALTER INDEX

I see, I guess I was under the impression that this would no-op and we could test that. I don't think we need to do that in this patch, I'm fine with this as is.

No, because in that case we are changing the implicitly partitioned columns to something else. But we are rewriting the index, so it's okay.
I don't know this code quite well, but the comment says

		// column descriptor in front of each index, and calling CreatePartitioning again
		// will make these indexes explicit.

The "CreatePartitioning again will make indexes explicit" no longer applies, right? Can't we clean this up?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)

[otan]

The "CreatePartitioning again will make indexes explicit" no longer applies, right? Can't we clean this up?

I see, yes, but that is now needed regardless for the REGIONAL BY ROW -> REGIONAL BY ROW case.

[otan]

bors r=arulajmani

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)