tree: change hint injection validation to take FmtFlags arg

Instead of passing around settings everywhere, let the outer code look
up the value of `sql.stats.statement_fingerprint.format_mask` and
instead pass FmtFlags into the hint injection code. This will simplify
the next PR.

Informs: #153633

Release note: None

Author: michae2

pkg/sql/sem/tree/inject_hints.go

@@ -8,7 +8,6 @@ package tree
 import (
 	"reflect"
 
-	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/errors"
 )
 
@@ -36,22 +35,13 @@ type HintInjectionDonor struct {
 	collapsed []bool
 }
 
-// validationFmtFlags returns the FmtFlags used to check that the donor
-// statement and the target statement match. These should be the same FmtFlags
-// used for statement fingerprints (including
-// sql.stats.statement_fingerprint.format_mask) with FmtHideHints added.
-func validationFmtFlags(sv *settings.Values) FmtFlags {
-	stmtFingerprintFmtMask := FmtHideConstants | FmtFlags(QueryFormattingForFingerprintsMask.Get(sv))
-	return stmtFingerprintFmtMask | FmtHideHints
-}
-
 // NewHintInjectionDonor creates a HintInjectionDonor from a parsed AST. The
 // parsed donor statement could be a regular SQL statement or a statement
 // fingerprint.
-func NewHintInjectionDonor(ast Statement, sv *settings.Values) (*HintInjectionDonor, error) {
+func NewHintInjectionDonor(ast Statement, fingerprintFlags FmtFlags) (*HintInjectionDonor, error) {
 	hd := &HintInjectionDonor{
 		ast:           ast,
-		validationSQL: AsStringWithFlags(ast, validationFmtFlags(sv)),
+		validationSQL: FormatStatementHideConstants(ast, fingerprintFlags, FmtHideHints),
 		collapsed:     []bool{false},
 	}
 	WalkStmt(hd, ast)
@@ -318,8 +308,8 @@ func (v *hintInjectionVisitor) VisitStatementPost(expr Statement) Statement {
 
 // Validate checks that the target statement exactly matches the donor (except
 // for hints).
-func (hd *HintInjectionDonor) Validate(stmt Statement, sv *settings.Values) error {
-	sql := AsStringWithFlags(stmt, validationFmtFlags(sv))
+func (hd *HintInjectionDonor) Validate(stmt Statement, fingerprintFlags FmtFlags) error {
+	sql := FormatStatementHideConstants(stmt, fingerprintFlags, FmtHideHints)
 	if sql != hd.validationSQL {
 		return errors.Newf(
 			"statement does not match hint donor statement: %v vs %v", sql, hd.validationSQL,

pkg/sql/sem/tree/inject_hints_test.go

@@ -27,12 +27,13 @@ func TestInjectHints(t *testing.T) {
 	defer log.Scope(t).Close(t)
 
 	testCases := []struct {
-		name          string
-		originalSQL   string
-		donorSQL      string
-		expectedSQL   string
-		expectChanged bool
-		expectError   bool
+		name              string
+		originalSQL       string
+		donorSQL          string
+		expectedSQL       string
+		expectChanged     bool
+		expectError       bool
+		legacyFingerprint bool
 	}{
 		{
 			name:          "inject index hint on simple table",
@@ -248,19 +249,55 @@ func TestInjectHints(t *testing.T) {
 			expectedSQL:   "SELECT * FROM (VALUES (5, 6), (7, 8)) AS v (c, d), a@{NO_FULL_SCAN} WHERE b = c",
 			expectChanged: true,
 		},
+		{
+			name:              "tuple with legacy collapsed list",
+			originalSQL:       "SELECT (4, 5, 6) FROM a JOIN b ON c = d",
+			donorSQL:          "SELECT (_, _, __more1_10__) FROM a INNER HASH JOIN b ON c = d",
+			expectedSQL:       "SELECT (4, 5, 6) FROM a INNER HASH JOIN b ON c = d",
+			expectChanged:     true,
+			legacyFingerprint: true,
+		},
+		{
+			name:              "array with legacy collapsed list",
+			originalSQL:       "SELECT ARRAY[4, 5, 6] FROM a JOIN b ON c = d",
+			donorSQL:          "SELECT ARRAY[_, _, __more1_10__] FROM a@{NO_FULL_SCAN} INNER HASH JOIN b@b_d_idx ON c = d",
+			expectedSQL:       "SELECT ARRAY[4, 5, 6] FROM a@{NO_FULL_SCAN} INNER HASH JOIN b@b_d_idx ON c = d",
+			expectChanged:     true,
+			legacyFingerprint: true,
+		},
+		{
+			name:              "values with legacy collapsed lists",
+			originalSQL:       "SELECT * FROM (VALUES (5, 6), (7, 8)) AS v (c, d), a WHERE b = c",
+			donorSQL:          "SELECT * FROM (VALUES (_, _), (__more1_10__)) AS v (c, d), a@{NO_FULL_SCAN} WHERE b = c",
+			expectedSQL:       "SELECT * FROM (VALUES (5, 6), (7, 8)) AS v (c, d), a@{NO_FULL_SCAN} WHERE b = c",
+			expectChanged:     true,
+			legacyFingerprint: true,
+		},
 		{
 			name:          "inner join",
 			originalSQL:   "SELECT * FROM a INNER JOIN b ON true",
 			donorSQL:      "SELECT * FROM a INNER JOIN b ON true",
 			expectedSQL:   "SELECT * FROM a INNER JOIN b ON true",
 			expectChanged: false,
 		},
+		{
+			name:          "inner join in donor",
+			originalSQL:   "SELECT * FROM a JOIN b ON true",
+			donorSQL:      "SELECT * FROM a INNER JOIN b ON true",
+			expectedSQL:   "SELECT * FROM a INNER JOIN b ON true",
+			expectChanged: true,
+		},
 	}
 
 	st := cluster.MakeTestingClusterSettings()
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
+			var fingerprintFlags tree.FmtFlags
+			if !tc.legacyFingerprint {
+				fingerprintFlags = tree.FmtFlags(tree.QueryFormattingForFingerprintsMask.Get(&st.SV))
+			}
+
 			// Parse original statement.
 			originalStmt, err := parser.ParseOne(tc.originalSQL)
 			require.NoError(t, err)
@@ -270,15 +307,15 @@ func TestInjectHints(t *testing.T) {
 			require.NoError(t, err)
 
 			// Create hint injection donor.
-			donor, err := tree.NewHintInjectionDonor(donorStmt.AST, &st.SV)
+			donor, err := tree.NewHintInjectionDonor(donorStmt.AST, fingerprintFlags)
 			require.NoError(t, err)
 
 			// Validate.
 			if tc.expectError {
-				require.Error(t, donor.Validate(originalStmt.AST, &st.SV))
+				require.Error(t, donor.Validate(originalStmt.AST, fingerprintFlags))
 				return
 			} else {
-				require.NoError(t, donor.Validate(originalStmt.AST, &st.SV))
+				require.NoError(t, donor.Validate(originalStmt.AST, fingerprintFlags))
 			}
 
 			// Inject hints.
@@ -347,15 +384,21 @@ func TestRandomInjectHints(t *testing.T) {
 		t.Log(randSQL + ";")
 
 		// Generate the statement fingerprint for the random statement.
+		var fingerprintFlags tree.FmtFlags
+		if rng.Intn(10) < 9 {
+			// 90% chance of using the current default statement fingerprint
+			// formatting, 10% chance of using the legacy statement fingerprint
+			// formatting.
+			fingerprintFlags = tree.FmtFlags(tree.QueryFormattingForFingerprintsMask.Get(&st.SV))
+		}
 		randStmt, err := parser.ParseOne(randSQL)
 		require.NoError(t, err)
-		fingerprintFlags := tree.FmtHideConstants | tree.FmtCollapseLists | tree.FmtConstantsAsUnderscores
-		randFingerprint := tree.AsStringWithFlags(randStmt.AST, fingerprintFlags)
+		randFingerprint := tree.FormatStatementHideConstants(randStmt.AST, fingerprintFlags)
 
 		// Parse random fingerprint to make the donor AST.
 		donorStmt, err := parser.ParseOne(randFingerprint)
 		require.NoError(t, err)
-		donor, err := tree.NewHintInjectionDonor(donorStmt.AST, &st.SV)
+		donor, err := tree.NewHintInjectionDonor(donorStmt.AST, fingerprintFlags)
 		require.NoError(t, err)
 		donorSQL := tree.AsString(donorStmt.AST)
 
@@ -366,15 +409,15 @@ func TestRandomInjectHints(t *testing.T) {
 		require.NoError(t, err)
 
 		// Validate.
-		require.NoError(t, donor.Validate(targetStmt.AST, &st.SV))
+		require.NoError(t, donor.Validate(targetStmt.AST, fingerprintFlags))
 
 		// Inject hints.
 		result, changed, err := donor.InjectHints(targetStmt.AST)
 		require.NoError(t, err)
 
 		// Check that the rewritten statement matches the donor statement when
 		// formatted as a statement fingerprint.
-		resultFingerprint := tree.AsStringWithFlags(result, fingerprintFlags)
+		resultFingerprint := tree.FormatStatementHideConstants(result, fingerprintFlags)
 		require.Equal(t, donorSQL, resultFingerprint, "resulting SQL mismatch")
 		if !changed {
 			require.Same(t, targetStmt.AST, result, "resulting statement was changed")