sql: permit forward indexes on arrays by jordanlewis · Pull Request #91762 · cockroachdb/cockroach

jordanlewis · 2022-11-11T18:59:36Z

This functionality has been unlocked since we added trigram indexes - the blocker before was the ability to support both forward and inverted indexes on a given type.

This commit simply permits forward indexes on arrays, where they were previously not supported at all.

Release note (sql change): enable forward indexes on arrays

cockroach-teamcity · 2022-11-11T18:59:45Z

This change is

jordanlewis · 2022-11-11T19:10:53Z

This needs to take into account enum values in partitioning columns: #17154 (comment)

ajwerner · 2022-11-11T19:16:38Z

One reasonable way to sidestep the array enums would be to disallow partitioning by arrays altogether. It's not clear to me that the partitioning code knows how to think about arrays. We'd want to add testing to cover both the simpler case of primitive arrays and then the more complex case of udts in arrays.

michae2

Cool!

Would be nice to have some test cases in pkg/sql/logictest/testdata/logic_test/distsql_stats creating stats on an indexed array column. Maybe a case where there is both a forward and an inverted index on the same array column.

Reviewable status: complete! 0 of 0 LGTMs obtained

jordanlewis · 2022-11-11T22:02:53Z

TFTR! Added a bunch of tests.

There's a bit of an annoying problem: the index recommendation engine doesn't properly handle types that are both inverted and forward indexable.

In particular, if a type is forward indexable, an inverted index will never be recommended even if it's possible and optimal to create one.

For now, I'm going to propose that we accept the regression (array inverted indexes will no longer be recommended). We already do not recommend trigram inverted indexes for this same reason.

I've opened #91777 to track this problem. It's a little bit annoying and will require some light refactoring of the index recommender, which currently uses data structures that do not permit adding the required extra bit (inverted or not) without changing the datatype (which is used in a bunch of different places).

jordanlewis · 2022-11-11T22:03:22Z

Also, I added a commit that disables partitioning tables and indexes by array columns to avoid the problem that @ajwerner was talking about.

PTAL.

If partitioning on array columns were supported, we'd need to make sure to check the partition columns for enum types when enums are edited - this is currently not implemented. It's not clear if users want to be able to partition tables that are indexed on arrays, so let's just not support it for the time being to avoid the trouble. Release note: None

michae2

Reviewed 3 of 5 files at r2, 8 of 8 files at r4, 7 of 7 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis)

pkg/sql/randgen/schema.go line 572 at r4 (raw file):

	// TODO(harding): Allow partitioning the primary index. This will require
	// massaging the syntax.
	if !isMultiRegion && !isPrimaryIndex && !partitioningNotSupported && len(prefix) > 0 && rng.Intn(10) == 0 {

Thank you for preemptively fixing randgen!

pkg/sql/logictest/testdata/logic_test/alter_table line 2674 at r5 (raw file):


statement ok
CREATE TABLE t1_non_indexable (n INT8);

nit: the name "t1_non_indexable" doesn't make sense any more 🙂 maybe "t1_indexable"?

pkg/sql/logictest/testdata/logic_test/array line 1461 at r5 (raw file):

CREATE INDEX i ON t(x DESC)

# Add "t@i" index annotation once #50659 is fixed.

nit: We should get rid of this todo (and probably do it, right? I guess we do this below, so maybe it's not necessary here).

pkg/sql/logictest/testdata/logic_test/array line 1469 at r5 (raw file):

{NULL}

# Add "t@i" index annotation once #50659 is fixed.

This one as well.

pkg/sql/logictest/testdata/logic_test/distsql_stats line 1946 at r5 (raw file):

ORDER BY statistics_name, column_names::STRING
----
NULL  {a}      10000  true

Should there be two rows for {a} here, one with the forward histogram and one with the inverted histogram?

Hmm, now that I'm trying this with strings it looks like we only get the forward histogram when both indexes exist. I remember in #84927 you fixed statistics_builder to handle coexistence of both types of histogram, so I assumed ANALYZE would create both, but maybe that's not the case?

Well, this doesn't seem like a blocker anyway. We can fix this in another PR.

jordanlewis

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

pkg/sql/randgen/schema.go line 572 at r4 (raw file):