sql: store partial index predicate in index descriptor

[mgartner]

With this commit, serialized partial index predicates are now stored on
index descriptors. Predicates are dequalified so that database and table
names are not included in column references.

Release note: None

[cockroach-teamcity]

This change is 

[jordanlewis]

Nice, this looks good. I hope you don't mind the drop in reviews, but I'm just quite familiar with what's going on so it's pretty low cost for me.

I think that this patch will be broken in the presence of column renames. You need to change the code in pkg/sql/rename_column.go to iterate over the Predicates and not just the check constraints and computed column expressions. You should also add test cases for this, and also maybe some test cases for rename table as you allude to in one of your comments.

[mgartner]

Hold off on reviewing this until #49657 is sorted out. A rebase will be required if that gets merged.

[mgartner]

This is required for the SHOW CREATE TABLE... statements I added. Without it the build fails with relation "system.zones" does not exist. Why?

[RaduBerinde]

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

---

pkg/sql/logictest/testdata/logic_test/partial_index, line 1 at r3 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

This is required for the SHOW CREATE TABLE... statements I added. Without it the build fails with relation "system.zones" does not exist. Why?

Maybe @asubiotto knows. It looks like it's blacklisted in a lot of other tests so this is probably fine

---

pkg/sql/sqlbase/structured.proto, line 470 at r3 (raw file):

  optional geo.geoindex.Config geo_config = 22 [(gogoproto.nullable) = false];

  // If Predicate is not the empty string, describes a predicate expression for

[nit] It would be nice if it would be more clear that this is only set for partial indexes, and is actually the only way to tell if an index is partial. Maybe "Predicate, when it is not empty, indicates that this is a partial index and contains the expression for the partial index condition."

---

pkg/sql/sqlbase/structured.proto, line 471 at r3 (raw file):

  // If Predicate is not the empty string, describes a predicate expression for
  // a partial index.

[nit] maybe add a TODO referencing #49766. (once that is addressed, we'd want to update this comment to say how the predicate expression refers to table columns)

---

pkg/sql/sqlbase/structured.proto, line 472 at r3 (raw file):

  // If Predicate is not the empty string, describes a predicate expression for
  // a partial index.
  optional string predicate = 23 [(gogoproto.nullable) = false];

[nit] consider partial_index_predicate (your call, I realize it's a mouthful)

[mgartner]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @asubiotto, @jordanlewis, and @RaduBerinde)

---

pkg/sql/sqlbase/structured.proto, line 470 at r3 (raw file):

Previously, RaduBerinde wrote…

[nit] It would be nice if it would be more clear that this is only set for partial indexes, and is actually the only way to tell if an index is partial. Maybe "Predicate, when it is not empty, indicates that this is a partial index and contains the expression for the partial index condition."

Done.

---

pkg/sql/sqlbase/structured.proto, line 471 at r3 (raw file):

Previously, RaduBerinde wrote…

[nit] maybe add a TODO referencing #49766. (once that is addressed, we'd want to update this comment to say how the predicate expression refers to table columns)

Done.

---

pkg/sql/sqlbase/structured.proto, line 472 at r3 (raw file):

Previously, RaduBerinde wrote…

[nit] consider partial_index_predicate (your call, I realize it's a mouthful)

I felt that "index" was redundant given that this is within IndexDescriptor. Would partial_predicate be better than predicate?

[RaduBerinde]

---

pkg/sql/sqlbase/structured.proto, line 472 at r3 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I felt that "index" was redundant given that this is within IndexDescriptor. Would partial_predicate be better than predicate?

Nah, that's more confusing (sounds like the predicate is partial). Better to keep it as is.

[asubiotto]

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

---

pkg/sql/logictest/testdata/logic_test/partial_index, line 1 at r3 (raw file):

Previously, RaduBerinde wrote…

Maybe @asubiotto knows. It looks like it's blacklisted in a lot of other tests so this is probably fine

Because system.zones only exists on the system tenant: #49445. #49784 should make it so that we can remove this blacklist for a lot of tests that depend on accessing zone configs. Blacklisting is fine.

[mgartner]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jordanlewis)

---

pkg/sql/logictest/testdata/logic_test/partial_index, line 1 at r3 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Because system.zones only exists on the system tenant: #49445. #49784 should make it so that we can remove this blacklist for a lot of tests that depend on accessing zone configs. Blacklisting is fine.

Thanks for the explanation!

---

pkg/sql/sqlbase/structured.proto, line 472 at r3 (raw file):

Previously, RaduBerinde wrote…

Nah, that's more confusing (sounds like the predicate is partial). Better to keep it as is.

Ok. I'll keep an eye on this, and if it is confusing us again before the release, I can rename it.

[mgartner]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)