opt: return struct from index rec library instead of formatted string

[mgartner]

opt: simplify the IndexRecommendationSet type

Release justification: This is a low-risk change to existing
functionality.

Release note: None

opt: rename some index recommendation functions

Some of the methods of indexrec.IndexRecommendationSet have been
renamed to be less redundant.

Release justification: This is a very minor change.

Release note: None

opt: return struct from index rec library instead of formatted string

Previously, the indexrec package returned a formatted string as the
final recommendation result. The string was formatted specificaly for
the output of EXPLAIN. This had limited utility. To create index
recommendations in the statement_statistics table, we were parsing the
string output.

This commit updates the indexrec package to return a slice of structs
rather than a slice of strings that are formatted for EXPLAIN output.
The recommendations can now be consumed in different contexts without
having to parse a string.

Fixes #85955

Release justification: This is a minor change to new functionality.

Release note: None

opt: simplify filenames in indexrec

Release justification: This is a non-code change.

Release note: None

[cockroach-teamcity]

This change is 

[maryliag]

Thank you for working on this! Awesome improvement! Just a minor change you will need to do

Reviewed 13 of 14 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @mgartner, and @wenyihu6)

---

pkg/sql/idxrecommendations/idx_recommendations.go line 51 at r1 (raw file):

	recommendations := make([]string, len(recs))
	for i := range recs {
		recType := "index creation"

you can remove the "index" and have just "creation" and just "replacement" below, to match the description of what this function does. Also your tests will probably fail with the extra word that are not expected on the final result
(on the UI we're also not expecting to see the "index" word)

[mgartner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @maryliag, and @wenyihu6)

---

pkg/sql/idxrecommendations/idx_recommendations.go line 51 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

you can remove the "index" and have just "creation" and just "replacement" below, to match the description of what this function does. Also your tests will probably fail with the extra word that are not expected on the final result
(on the UI we're also not expecting to see the "index" word)

Thanks for catching this! Fixed.

[maryliag]

Changes related to how is being used/saved for statement statistics

Reviewed 15 of 15 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball and @wenyihu6)

[wenyihu6]

It's possible I'm biased; do you think it's worth changing this struct to an interface? And we can create three types of structs that implement this interface.

type Rec interface {
    sqlString() // return "CREATE INDEX ..."
    recType() // return "index creation"
}

type CreateIndex struct {
     SQL string
     retType string
}

type IndexReplacement struct {
     SQL string
     retType string
}

type AlterIndexVisible struct {
     SQL string
     retType string
}

When you constructed the struct Rec, you know the type of index recommendation, what sql string you want to output already. Every time when you use Rec instances, instead of using if statements to check, and printing out different things, having an interface that returns what string to print out for different index rec type seems to be cleaner. We are only introducing three types of index rec now (create index, replacement, alter index visibility). But it's possible that we want to introduce more types of index rec in the future, and the if statement can become really long. This would make these two parts of the code easier.

cockroach/pkg/sql/idxrecommendations/idx_recommendations.go

Lines 50 to 54 in 8f6923b

	for i := range recs {
	recType := "creation"
	if recs[i].Replacement {
	recType = "replacement"
	}

cockroach/pkg/sql/explain_plan.go

Lines 145 to 152 in 8f6923b

	plural := ""
	recType := "index creation"
	if recs[i].Replacement {
	recType = "index replacement"
	plural = "s"
	}
	rows = append(rows, fmt.Sprintf("%d. type: %s", i+1, recType))
	rows = append(rows, fmt.Sprintf(" SQL command%s: %s", plural, recs[i].SQL))

Also, we are adding these printing statements in pkg/sql/explain_plan.go and pkg/sql/idxrecommendations/idx_recommendations.go which are outside of the pkg/sql/opt/indexrec folder. It's easy to forget updating them when someone is updating indexrec.

If we don't want to introduce an interface, adding struct methods inside index rec folder that does if statement check and return is also sufficient.

type Rec struct {
	// SQL is the statement(s) to run that will apply the recommendation.
	SQL string
	// Replacement is true if SQL replaces an existing index, i.e., it contains
	// both a CREATE INDEX and DROP INDEX statement.
	Replacement bool
}

func (rec Rec) retType() string {
 if Replacement{ return "index replacement"}
else {return "index creation"}
}

func (rec Rec) sqlStmt() string {
 if Replacement{ return "SQL commands: ...."}
else ...
}

[mgartner]

In Go, interfaces are primarily used to describe a set of actions or verbs that a type can perform, not a thing itself. As opposed to classes in object-oriented languages, they are generally not used to create hierarchies or groups of types, like in the example you provided. While we don't explicitly mention avoiding this pattern in our coding guidelines, I (and I think most others) would consider it non-idiomatic Go.

Instead of abstract type hierarchies, we should think about the concrete representation of the data (e.g., the recommendations) that we are working with. The different types of recommendations are all physically (their in-memory structure) very similar - they are all SQL strings. An interface doesn't provide any benefit from what I can tell. And there are downsides to using an interface including a possible heap allocation (an interface is a pointer to data and a type; in many cases the data will be allocated on the heap) (edit: although the SQL string requires an allocation regardless), obfuscating the nature of the data we are working with, and more lines of code.

Your concern with many if statements for generating rec type strings is more idiomatically handled by defining a String method for the enum representing the type of the recommendation. For example:

// Rec represents an index recommendation in the form of a SQL statement(s)
// that can be executed to apply the recommendation.
type Rec struct {
	// SQL is the statement(s) to run that will apply the recommendation.
	SQL string
	// Type is the type of the recommendation.
	Type Type
}

type Type uint8

const (
	TypeCreate Type = iota
	TypeReplace
	TypeAlter
)

func (t Type) String() string {
	switch t {
	case TypeCreate:
		return "index creation"
	case TypeReplace:
		return "index replacement"
	case TypeAlter:
		return "index alter"
	default:
		return "invalid"
	}
}

func formatRec(r Rec) string {
	// No if's necessary!
	return fmt.Sprintf("SQL: %s, Type: %s", r.SQL, r.Type.String())
}

If we need to add a new type of recommendation, it's as simple as:

 const (
 	TypeCreate Type = iota
 	TypeReplace
 	TypeAlter
+	TypeMyNewType
 )
 
 func (t Type) String() string {
 	switch t {
 	case TypeCreate:
 		return "index creation"
 	case TypeReplace:
 		return "index replacement"
 	case TypeAlter:
 		return "index alter"
+	case TypeMyNewType:
+		return "a new type"
 	default:
 		return "invalid"
 	}
 }

[wenyihu6]

I see. That makes sense. Thanks for explaining to me!

[mgartner]

If we don't want to introduce an interface, adding struct methods inside index rec folder that does if statement check and return is also sufficient.

That's a possibility - but I'd argue that if the formatting of a Rec is context-dependent (which it appears to be), then the implementation of Rec shouldn't be concerned with providing methods for all the different ways it can be formatted. For example, if the index rec type formatting changes depending on context, then my String suggestion won't work, but I don't think the conclusion is that we should define multiple XXXString() methods. We can define helpers to format the type in the context that the Rec is used.

Edit: I feel less strongly about this than the interface, though, and I've gone against this suggestion myself in the past, for example:

cockroach/pkg/sql/sem/volatility/volatility.go

Lines 64 to 112 in a6e88c3

	// String returns the byte representation of Volatility as a string.
	func (v V) String() string {
	switch v {
	case Leakproof:
	return "leakproof"
	case Immutable:
	return "immutable"
	case Stable:
	return "stable"
	case Volatile:
	return "volatile"
	default:
	return "invalid"
	}
	}
	// TitleString returns the byte representation of Volatility as a title-cased
	// string.
	func (v V) TitleString() string {
	switch v {
	case Leakproof:
	return "Leakproof"
	case Immutable:
	return "Immutable"
	case Stable:
	return "Stable"
	case Volatile:
	return "Volatile"
	default:
	return "Invalid"
	}
	}
	// ToPostgres returns the postgres "provolatile" string ("i" or "s" or "v") and
	// the "proleakproof" flag.
	func (v V) ToPostgres() (provolatile string, proleakproof bool) {
	switch v {
	case Leakproof:
	return "i", true
	case Immutable:
	return "i", false
	case Stable:
	return "s", false
	case Volatile:
	return "v", false
	default:
	panic(errors.AssertionFailedf("invalid volatility %s", v))
	}
	}

[wenyihu6]

pkg/sql/opt/testutils/opttester/opt_tester.go line 2209 at r3 (raw file):

	md = optExpr.(memo.RelExpr).Memo().Metadata()
	indexRecommendations := indexrec.FindRecs(optExpr, md)
	result := indexRecommendations.Output()

--- a/pkg/sql/opt/testutils/opttester/opt_tester.go
+++ b/pkg/sql/opt/testutils/opttester/opt_tester.go
@@ -2206,12 +2206,23 @@ func (ot *OptTester) IndexRecommendations() (string, error) {
        }
        md = optExpr.(memo.RelExpr).Memo().Metadata()
        indexRecommendations := indexrec.FindRecs(optExpr, md)
-       result := indexRecommendations.Output()
+       var rows []string
+       rows = append(rows, fmt.Sprintf("index recommendations: %d", len(indexRecommendations)))
+       for i := range indexRecommendations {
+               plural := ""
+               recType := "index creation"
+               if indexRecommendations[i].Replacement {
+                       recType = "index replacement"
+                       plural = "s"
+               }
+               rows = append(rows, fmt.Sprintf("%d. type: %s", i+1, recType))
+               rows = append(rows, fmt.Sprintf("   SQL command%s: %s", plural, indexRecommendations[i].SQL))
+       }
 
-       if result == nil {
+       if indexRecommendations == nil {
                return fmt.Sprintf("No index recommendations.\n--\nOptimal Plan.\n%s", ot.FormatExpr(optExpr)), nil
        }
-       return fmt.Sprintf("%s\n--\nOptimal Plan.\n%s", strings.Join(result, "\n"), ot.FormatExpr(optExpr)), nil
+       return fmt.Sprintf("%s\n--\nOptimal Plan.\n%s", strings.Join(rows, "\n"), ot.FormatExpr(optExpr)), nil
 }

I think you need something like this here as well.

[mgartner]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball, @maryliag, and @wenyihu6)

---

pkg/sql/opt/testutils/opttester/opt_tester.go line 2209 at r3 (raw file):

Previously, wenyihu6 (Wenyi Hu) wrote…

--- a/pkg/sql/opt/testutils/opttester/opt_tester.go
+++ b/pkg/sql/opt/testutils/opttester/opt_tester.go
@@ -2206,12 +2206,23 @@ func (ot *OptTester) IndexRecommendations() (string, error) {
        }
        md = optExpr.(memo.RelExpr).Memo().Metadata()
        indexRecommendations := indexrec.FindRecs(optExpr, md)
-       result := indexRecommendations.Output()
+       var rows []string
+       rows = append(rows, fmt.Sprintf("index recommendations: %d", len(indexRecommendations)))
+       for i := range indexRecommendations {
+               plural := ""
+               recType := "index creation"
+               if indexRecommendations[i].Replacement {
+                       recType = "index replacement"
+                       plural = "s"
+               }
+               rows = append(rows, fmt.Sprintf("%d. type: %s", i+1, recType))
+               rows = append(rows, fmt.Sprintf("   SQL command%s: %s", plural, indexRecommendations[i].SQL))
+       }
 
-       if result == nil {
+       if indexRecommendations == nil {
                return fmt.Sprintf("No index recommendations.\n--\nOptimal Plan.\n%s", ot.FormatExpr(optExpr)), nil
        }
-       return fmt.Sprintf("%s\n--\nOptimal Plan.\n%s", strings.Join(result, "\n"), ot.FormatExpr(optExpr)), nil
+       return fmt.Sprintf("%s\n--\nOptimal Plan.\n%s", strings.Join(rows, "\n"), ot.FormatExpr(optExpr)), nil
 }

I think you need something like this here as well.

Nice catch, thanks!

Fixed. I also added another commit that simplifies the output of the tests - they don't need to mimic the EXPLAIN output anymore.

[wenyihu6]

Sorry for taking so long! The rest looks good to me!

Reviewed 8 of 15 files at r3, 3 of 6 files at r5, 3 of 3 files at r6, 1 of 15 files at r7, 8 of 10 files at r9, 4 of 6 files at r10, 3 of 3 files at r11, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball)

[mgartner]

TFTRs!

The failure caused by #86838, which is unrelated.

bors r+

[craig]

This PR was included in a batch that timed out, it will be automatically retried

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)