Merge #150454 #152765 #153168 #153176 #153327

150454: tree: clean up DOidWrapper usage r=yuzefovich a=yuzefovich

This PR cleans up usage of `DOidWrapper` utility. Throughout the years we've accumulated some dead code as well as in many cases we had blind copy-paste of unnecessary code, and this PR gets rid of it. Namely, `DOidWrapper` utility is now used to support `DName` and `DRefCursor` (on top of `DString`) and `DCIText` (on top of `DCollatedString`), and any other usages are forbidden. This PR removes all those other usages.

Please see each commit for more details.

Epic: None


152765: sql: replace a few context.Background() with what we have in scope r=yuzefovich a=yuzefovich

**sql: remove no longer used StubTableStats function**

**sql: replace a few context.Background() with what we have in scope**

I was looking into a timeout where we seemingly didn't respect the
context cancellation and noticed a couple of places where we use
`context.Backround()` that were easy to fix.

Touches: #152417.
Epic: None


153168: colcontainer: harden closing of files in diskQueue r=yuzefovich a=yuzefovich

The AI review of 31c3419 pointed out that `io.Closer` interface is such that calling `Close` multiple times can result in an undefined behavior. Previously, we had multiple places where if we got an error on the first `Close` call of read or write file of the disk queue, we could try to close the same file again - this was the case since we nil-ed out the file pointer after the error short-circuiting logic. This commit hardens the disk queue around this error to guarantee that we always close the file exactly once. This also allows us to remove some of explicit nilling out logic which now became redundant.

It's not immediately clear to me what actual "undefined behavior" would be in case of double-`Close` (or if it's even possible for implementations that we use to return an error), so I didn't include a release note. Still, it seems prudent to follow the interface's contract.

Epic: None
Release note: None

153176: sql/parser: fix minor issue with recent LTREE additions r=yuzefovich a=yuzefovich

We recently added parser support for FIRST_CONTAINS and FIRST_CONTAINED_BY operators (`?`@>`` and `?<`@`` respectively). The AI review pointed out a minor bug in how we handle these operators in the scanner - we incorrectly advanced the position unconditionally before matching the following characters. Instead, we need to use `peekN` to look a few characters ahead to see whether we have a match, and this commit does that. Note that the impact of the bug is very minor - we simply would put the `^` character in the error in the wrong position.

Note that two newly-added tests have slightly different behavior with regards to the `^` character which seems to be due to peculiarities of the tokenizing / parsing that I didn't look closely into.

Additionally, this commit adds a newline character in an unrelated parser test file.

Epic: None
Release note: None

153327: importer: simplify MakeTestingSimpleTableDescriptor r=yuzefovich a=yuzefovich

We no longer need any FK handling in this function, which allows us to remove some dead code. Also update error messages to not reference no longer open issues.

Epic: None
Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>

Author: craig[bot]

pkg/backup/compaction_job.go

@@ -161,11 +161,11 @@ func maybeStartCompactionJob(
 		if err != nil {
 			return err
 		}
-		idDatum, ok := tree.AsDInt(datums[0])
+		idDatum, ok := datums[0].(*tree.DInt)
 		if !ok {
 			return errors.Newf("expected job ID: unexpected result type %T", datums[0])
 		}
-		jobID = jobspb.JobID(idDatum)
+		jobID = jobspb.JobID(*idDatum)
 
 		scheduledJob := jobs.ScheduledJobTxn(txn)
 		backupSchedule, args, err := getScheduledBackupExecutionArgsFromSchedule(

pkg/backup/schedule_exec.go

@@ -544,12 +544,12 @@ func getBackupFnTelemetry(
 			return jobspb.BackupDetails{}, errors.New("expected job ID as first column of result")
 		}
 
-		jobID, ok := tree.AsDInt(jobIDDatum)
+		jobID, ok := jobIDDatum.(*tree.DInt)
 		if !ok {
 			return jobspb.BackupDetails{}, errors.New("expected job ID as first column of result")
 		}
 
-		job, err := registry.LoadJobWithTxn(ctx, jobspb.JobID(jobID), txn)
+		job, err := registry.LoadJobWithTxn(ctx, jobspb.JobID(*jobID), txn)
 		if err != nil {
 			return jobspb.BackupDetails{}, errors.Wrap(err, "failed to load dry-run backup job")
 		}

pkg/ccl/changefeedccl/avro/avro.go

@@ -450,7 +450,11 @@ func typeToSchema(typ *types.T) (*SchemaField, error) {
 		setNullable(
 			SchemaTypeString,
 			func(d tree.Datum, _ interface{}) (interface{}, error) {
-				return string(*d.(*tree.DString)), nil
+				s, ok := tree.AsDString(d)
+				if !ok {
+					return nil, errors.Newf("expected string type, got %T", d)
+				}
+				return string(s), nil
 			},
 			func(x interface{}) (tree.Datum, error) {
 				return tree.NewDString(x.(string)), nil
@@ -460,7 +464,11 @@ func typeToSchema(typ *types.T) (*SchemaField, error) {
 		setNullable(
 			SchemaTypeString,
 			func(d tree.Datum, _ interface{}) (interface{}, error) {
-				return d.(*tree.DCollatedString).Contents, nil
+				cs, ok := tree.AsDCollatedString(d)
+				if !ok {
+					return nil, errors.Newf("expected collated string type, got %T", d)
+				}
+				return cs.Contents, nil
 			},
 			func(x interface{}) (tree.Datum, error) {
 				return tree.NewDCollatedString(x.(string), typ.Locale(), &tree.CollationEnvironment{})

pkg/ccl/changefeedccl/avro/avro_test.go

@@ -72,7 +72,8 @@ func parseTableDesc(createTableStmt string) (catalog.TableDescriptor, error) {
 	tableID := descpb.ID(bootstrap.TestingUserDescID(1))
 	semaCtx := makeTestSemaCtx()
 	mutDesc, err := importer.MakeTestingSimpleTableDescriptor(
-		ctx, &semaCtx, st, createTable, parentID, keys.PublicSchemaID, tableID, importer.NoFKs, timeutil.Now().UnixNano())
+		ctx, &semaCtx, st, createTable, parentID, keys.PublicSchemaID, tableID, timeutil.Now().UnixNano(),
+	)
 	if err != nil {
 		return nil, err
 	}

pkg/ccl/changefeedccl/encoder_json_test.go

@@ -342,7 +342,8 @@ func parseTableDesc(createTableStmt string) (catalog.TableDescriptor, error) {
 	tableID := descpb.ID(bootstrap.TestingUserDescID(1))
 	semaCtx := makeTestSemaCtx()
 	mutDesc, err := importer.MakeTestingSimpleTableDescriptor(
-		ctx, &semaCtx, st, createTable, parentID, keys.PublicSchemaID, tableID, importer.NoFKs, timeutil.Now().UnixNano())
+		ctx, &semaCtx, st, createTable, parentID, keys.PublicSchemaID, tableID, timeutil.Now().UnixNano(),
+	)
 	if err != nil {
 		return nil, err
 	}

pkg/ccl/changefeedccl/encoder_test.go

@@ -1284,7 +1284,7 @@ func TestJsonRountrip(t *testing.T) {
 			// In this case, we can just compare strings.
 			if isFloatOrDecimal(test.datum.ResolvedType()) {
 				require.Equal(t, d.String(), j.String())
-			} else if dArr, ok := tree.AsDArray(test.datum); ok && isFloatOrDecimal(dArr.ParamTyp) {
+			} else if dArr, ok := test.datum.(*tree.DArray); ok && isFloatOrDecimal(dArr.ParamTyp) {
 				require.Equal(t, d.String(), j.String())
 			} else {
 				cmp, err := d.Compare(j)

pkg/ccl/changefeedccl/event_processing.go

@@ -825,7 +825,7 @@ func readOneJSONValue(row cdcevent.Row, colName string) (*tree.DJSON, error) {
 		if d == tree.DNull {
 			return nil
 		}
-		if valJSON, ok = tree.AsDJSON(d); !ok {
+		if valJSON, ok = d.(*tree.DJSON); !ok {
 			return errors.Newf("expected a JSON object, got %s", d.ResolvedType())
 		}
 		return nil

pkg/ccl/changefeedccl/kcjsonschema/kafka_connect_json_schema_test.go

@@ -183,7 +183,8 @@ func parseTableDesc(createTableStmt string) (catalog.TableDescriptor, error) {
 	tableID := descpb.ID(bootstrap.TestingUserDescID(1))
 	semaCtx := makeTestSemaCtx()
 	mutDesc, err := importer.MakeTestingSimpleTableDescriptor(
-		ctx, &semaCtx, st, createTable, parentID, keys.PublicSchemaID, tableID, importer.NoFKs, timeutil.Now().UnixNano())
+		ctx, &semaCtx, st, createTable, parentID, keys.PublicSchemaID, tableID, timeutil.Now().UnixNano(),
+	)
 	if err != nil {
 		return nil, err
 	}

pkg/ccl/partitionccl/zone_test.go

@@ -358,7 +358,8 @@ func (pt partitioningTest) parse() (parsed parsedPartitioningTest, _ error) {
 		st := cluster.MakeTestingClusterSettings()
 		parentID, tableID := descpb.ID(bootstrap.TestingUserDescID(0)), descpb.ID(bootstrap.TestingUserDescID(1))
 		mutDesc, err := importer.MakeTestingSimpleTableDescriptor(
-			ctx, &semaCtx, st, createTable, parentID, keys.PublicSchemaID, tableID, importer.NoFKs, timeutil.Now().UnixNano())
+			ctx, &semaCtx, st, createTable, parentID, keys.PublicSchemaID, tableID, timeutil.Now().UnixNano(),
+		)
 		if err != nil {
 			return parsed, err
 		}

pkg/cmd/smith/main.go

@@ -236,7 +236,8 @@ func parseSchemaDefinition(schemaPath string) (opts []sqlsmith.SmitherOption, _
 		case *tree.CreateTable:
 			tableID := descpb.ID(int(parentID) + i + 1)
 			desc, err := importer.MakeTestingSimpleTableDescriptor(
-				context.Background(), &semaCtx, st, t, parentID, keys.PublicSchemaID, tableID, importer.NoFKs, wall)
+				context.Background(), &semaCtx, st, t, parentID, keys.PublicSchemaID, tableID, wall,
+			)
 			if err != nil {
 				return nil, errors.Wrapf(err, "failed to create table descriptor for statement %s", t)
 			}