memory: index: various inconsistencies and potential bugs

[paralin]

According to the interface definition for IndexAlterableTable:

// DropIndex removes an index from this table, if it exists.
// Returns an error if the removal failed or the index does not exist.
func (t *Table) DropIndex(ctx *sql.Context, indexName string) error

However the DropIndex in memory/ does not return an error if the index does not exist:

func (t *Table) DropIndex(ctx *sql.Context, indexName string) error {
	for name := range t.indexes {
		if name == indexName {
			delete(t.indexes, name)
		}
	}
	return nil
}

Which should be updated, the interface, or the memory/ impl?

[paralin]

In RenameIndex, the function is defined in the interface as:

// RenameIndex renames an existing index to another name that is not already taken by another index on this table.
func (t *Table) RenameIndex(ctx *sql.Context, fromIndexName string, toIndexName string) error

This implies that the function should return an error if toIndexName already exists, but the implementation in memory/ does not:

// RenameIndex implements sql.IndexAlterableTable
func (t *Table) RenameIndex(ctx *sql.Context, fromIndexName string, toIndexName string) error {
	if fromIndexName == toIndexName {
		return nil
	}
	if idx, ok := t.indexes[fromIndexName]; ok {
		delete(t.indexes, fromIndexName)
		t.indexes[toIndexName] = idx
	}
	return nil
}

[paralin]

In createIndex in the memory/ package, the DB ID is always set to "" (at line 1426) - why, shouldn't it be set to the db name?

[paralin]

In the table index, createIndex is passed a list of columns, []sql.IndexColumn and an expression is built to select each column.

Is it necessary for the underlying storage driver to store the full expression? Couldn't this be simplified to store just the column names?

This is a more unimportant question than the others, more of a performance optimization (skipping parsing the expressions)

[paralin]

In the table index, the errIfDuplicateEntryExist function iterates over all rows of all partitions which seems like it's definitely not intended to be the behavior that a "production grade" implementation would use. Is it really necessary to access all rows in the table for this?

In that same function it checks if the values of those columns have any nulls, and skips the entire row if so. This seems like a mistake: shouldn't the non-nil values be checked for uniqueness rather than skipping the entire row?

[paralin]

I guess most of this can be chalked up to #1347 - that the in-memory indexes are currently pretty much unimplemented.

[timsehn]

Thanks for using go-mysql-server (GMS as we call it).

I think you are correct that most of the issues you are seeing with drop and rename index are that the backend implementations are responsible for implementing these behaviors and the in memory index does not. We don't use the raw in memory GMS index implementation in Dolt so we've never prioritized a more robust implementation. This is definitely a place where we'd love a hero open source contribution :-)