sql: audit production callers of rowenc.DatumToEncDatum

yuzefovich · yuzefovich · commit c0aab4862184 · 2025-10-06T17:15:45.000-07:00
This commit audits all callers of `rowenc.DatumToEncDatum` in order to
replace possibly unsafe calls to `DatumToEncDatumEx` which requires
explicit error handling (the former panics when the datum's resolved
type is not equivalent to the passed-in type). I decided to not remove
the former method altogether since it seems to be reasonable to use it
when we're construcing the datum directly in the call site (and also in
tests).

Release note: None
diff --git a/pkg/ccl/changefeedccl/avro/avro.go b/pkg/ccl/changefeedccl/avro/avro.go
@@ -995,7 +995,10 @@ func (r *DataRecord) rowFromNative(native interface{}) (rowenc.EncDatumRow, erro
 		if err != nil {
 			return nil, err
 		}
-		row[r.colIdxByFieldIdx[fieldIdx]] = rowenc.DatumToEncDatum(field.typ, decoded)
+		row[r.colIdxByFieldIdx[fieldIdx]], err = rowenc.DatumToEncDatumEx(field.typ, decoded)
+		if err != nil {
+			return nil, err
+		}
 	}
 	return row, nil
 }
diff --git a/pkg/sql/distsql_physical_planner.go b/pkg/sql/distsql_physical_planner.go
@@ -4478,7 +4478,10 @@ func (dsp *DistSQLPlanner) createValuesSpecFromTuples(
 			if err != nil {
 				return nil, err
 			}
-			encDatum := rowenc.DatumToEncDatum(resultTypes[colIdx], datum)
+			encDatum, err := rowenc.DatumToEncDatumEx(resultTypes[colIdx], datum)
+			if err != nil {
+				return nil, err
+			}
 			buf, err = encDatum.Encode(resultTypes[colIdx], &a, catenumpb.DatumEncoding_VALUE, buf)
 			if err != nil {
 				return nil, err
diff --git a/pkg/sql/distsql_plan_window.go b/pkg/sql/distsql_plan_window.go
@@ -180,7 +180,10 @@ func initBoundsFromAST(
 			spec.Start.OffsetType = execinfrapb.DatumInfo{Encoding: catenumpb.DatumEncoding_VALUE, Type: typ}
 			var buf []byte
 			var a tree.DatumAlloc
-			datum := rowenc.DatumToEncDatum(typ, dStartOffset)
+			datum, err := rowenc.DatumToEncDatumEx(typ, dStartOffset)
+			if err != nil {
+				return err
+			}
 			buf, err = datum.Encode(typ, &a, catenumpb.DatumEncoding_VALUE, buf)
 			if err != nil {
 				return err
@@ -226,7 +229,10 @@ func initBoundsFromAST(
 				spec.End.OffsetType = execinfrapb.DatumInfo{Encoding: catenumpb.DatumEncoding_VALUE, Type: typ}
 				var buf []byte
 				var a tree.DatumAlloc
-				datum := rowenc.DatumToEncDatum(typ, dEndOffset)
+				datum, err := rowenc.DatumToEncDatumEx(typ, dEndOffset)
+				if err != nil {
+					return err
+				}
 				buf, err = datum.Encode(typ, &a, catenumpb.DatumEncoding_VALUE, buf)
 				if err != nil {
 					return err
diff --git a/pkg/sql/execinfra/processorsbase.go b/pkg/sql/execinfra/processorsbase.go
@@ -295,7 +295,10 @@ func (h *ProcOutputHelper) ProcessRow(
 			if err != nil {
 				return nil, false, err
 			}
-			h.outputRow[i] = rowenc.DatumToEncDatum(h.OutputTypes[i], datum)
+			h.outputRow[i], err = rowenc.DatumToEncDatumEx(h.OutputTypes[i], datum)
+			if err != nil {
+				return nil, false, err
+			}
 		}
 	} else if h.outputCols != nil {
 		// Projection.
diff --git a/pkg/sql/plan_node_to_row_source.go b/pkg/sql/plan_node_to_row_source.go
@@ -199,7 +199,11 @@ func (p *planNodeToRowSource) Next() (rowenc.EncDatumRow, *execinfrapb.ProducerM
 
 		for i, datum := range p.node.Values() {
 			if datum != nil {
-				p.row[i] = rowenc.DatumToEncDatum(p.outputTypes[i], datum)
+				p.row[i], err = rowenc.DatumToEncDatumEx(p.outputTypes[i], datum)
+				if err != nil {
+					p.MoveToDraining(err)
+					return nil, p.DrainHelper()
+				}
 			}
 		}
 		// ProcessRow here is required to deal with projections, which won't be
diff --git a/pkg/sql/row/fetcher.go b/pkg/sql/row/fetcher.go
@@ -1131,7 +1131,10 @@ func (rf *Fetcher) processValueSingle(
 	if rf.args.TraceKV {
 		prettyValue = value.String()
 	}
-	table.row[idx] = rowenc.DatumToEncDatum(typ, value)
+	table.row[idx], err = rowenc.DatumToEncDatumEx(typ, value)
+	if err != nil {
+		return "", "", err
+	}
 	return prettyKey, prettyValue, nil
 }
 
diff --git a/pkg/sql/rowcontainer/disk_row_container_test.go b/pkg/sql/rowcontainer/disk_row_container_test.go
@@ -281,17 +281,21 @@ func TestDiskRowContainer(t *testing.T) {
 					}
 
 					// Check sorted order.
-					sortedRows.getEncRow(sortedRows.scratchEncRow, numKeysRead)
+					err = sortedRows.getEncRow(sortedRows.scratchEncRow, numKeysRead)
+					if err != nil {
+						t.Fatal(err)
+					}
 					if cmp, err := compareEncRows(
 						ctx, types, sortedRows.scratchEncRow, row, &evalCtx, d.datumAlloc, ordering,
 					); err != nil {
 						t.Fatal(err)
 					} else if cmp != 0 {
-						sortedRows.getEncRow(sortedRows.scratchEncRow, numKeysRead)
+						err = sortedRows.getEncRow(sortedRows.scratchEncRow, numKeysRead)
 						t.Fatalf(
-							"expected %s to be equal to %s",
+							"expected %s to be equal to %s (err=%v)",
 							row.String(types),
 							sortedRows.scratchEncRow.String(types),
+							err,
 						)
 					}
 					numKeysRead++
diff --git a/pkg/sql/rowcontainer/hash_row_container.go b/pkg/sql/rowcontainer/hash_row_container.go
@@ -362,8 +362,7 @@ func (i *hashMemRowBucketIterator) Next() {
 
 // EncRow implements the RowIterator interface.
 func (i *hashMemRowBucketIterator) EncRow() (rowenc.EncDatumRow, error) {
-	i.container.getEncRow(i.scratchEncRow, i.rowIdxs[i.curIdx])
-	return i.scratchEncRow, nil
+	return i.scratchEncRow, i.container.getEncRow(i.scratchEncRow, i.rowIdxs[i.curIdx])
 }
 
 // Row implements the RowIterator interface.
@@ -452,7 +451,7 @@ func (i *hashMemRowIterator) computeKey() error {
 	}
 
 	if valid {
-		i.container.getEncRow(i.scratchEncRow, i.curIdx)
+		err = i.container.getEncRow(i.scratchEncRow, i.curIdx)
 	} else {
 		if i.curIdx == 0 {
 			// There are no rows in the container, so the key corresponding to the
@@ -464,7 +463,10 @@ func (i *hashMemRowIterator) computeKey() error {
 		// will "simulate" the key corresponding to the non-existent row as the key
 		// to the last existing row plus one (plus one part is done below where we
 		// append the index of the row to curKey).
-		i.container.getEncRow(i.scratchEncRow, i.curIdx-1)
+		err = i.container.getEncRow(i.scratchEncRow, i.curIdx-1)
+	}
+	if err != nil {
+		return err
 	}
 
 	i.curKey = i.curKey[:0]
@@ -496,8 +498,7 @@ func (i *hashMemRowIterator) Next() {
 
 // EncRow implements the RowIterator interface.
 func (i *hashMemRowIterator) EncRow() (rowenc.EncDatumRow, error) {
-	i.container.getEncRow(i.scratchEncRow, i.curIdx)
-	return i.scratchEncRow, nil
+	return i.scratchEncRow, i.container.getEncRow(i.scratchEncRow, i.curIdx)
 }
 
 // Row implements the RowIterator interface.
diff --git a/pkg/sql/rowcontainer/numbered_row_container.go b/pkg/sql/rowcontainer/numbered_row_container.go
@@ -186,8 +186,7 @@ func (d *DiskBackedNumberedRowContainer) GetRow(
 			return nil, nil
 		}
 		mrc := d.rc.mrc
-		mrc.getEncRow(mrc.scratchEncRow, idx)
-		return mrc.scratchEncRow, nil
+		return mrc.scratchEncRow, mrc.getEncRow(mrc.scratchEncRow, idx)
 	}
 	return d.rowIter.getRow(ctx, idx, skip)
 }
diff --git a/pkg/sql/rowcontainer/row_container.go b/pkg/sql/rowcontainer/row_container.go
@@ -231,11 +231,16 @@ func (mc *MemRowContainer) Less(i, j int) bool {
 // getEncRow populates the given EncDatumRow with the values of the idx-th row.
 // The behavior is undefined if the given row is of a different width than the
 // rows stored in the container.
-func (mc *MemRowContainer) getEncRow(encRow rowenc.EncDatumRow, idx int) {
+func (mc *MemRowContainer) getEncRow(encRow rowenc.EncDatumRow, idx int) error {
 	datums := mc.At(idx)
 	for i, d := range datums {
-		encRow[i] = rowenc.DatumToEncDatum(mc.types[i], d)
+		var err error
+		encRow[i], err = rowenc.DatumToEncDatumEx(mc.types[i], d)
+		if err != nil {
+			return err
+		}
 	}
+	return nil
 }
 
 // AddRow adds a row to the container.
@@ -344,7 +349,11 @@ func (i *memRowIterator) Next() {
 func (i *memRowIterator) EncRow() (rowenc.EncDatumRow, error) {
 	datums := i.container.At(i.curIdx)
 	for colidx, d := range datums {
-		i.scratchEncRow[colidx] = rowenc.DatumToEncDatum(i.container.types[colidx], d)
+		var err error
+		i.scratchEncRow[colidx], err = rowenc.DatumToEncDatumEx(i.container.types[colidx], d)
+		if err != nil {
+			return nil, err
+		}
 	}
 	return i.scratchEncRow, nil
 }
@@ -377,8 +386,7 @@ func (mc *MemRowContainer) NewFinalIterator(ctx context.Context) RowIterator {
 
 // GetRow implements IndexedRowContainer.
 func (mc *MemRowContainer) GetRow(ctx context.Context, pos int) (eval.IndexedRow, error) {
-	mc.getEncRow(mc.scratchEncRow, pos)
-	return IndexedRow{Idx: pos, Row: mc.scratchEncRow}, nil
+	return IndexedRow{Idx: pos, Row: mc.scratchEncRow}, mc.getEncRow(mc.scratchEncRow, pos)
 }
 
 var _ RowIterator = &memRowFinalIterator{}
@@ -398,8 +406,7 @@ func (i *memRowFinalIterator) Next() {
 
 // EncRow implements the RowIterator interface.
 func (i *memRowFinalIterator) EncRow() (rowenc.EncDatumRow, error) {
-	i.container.getEncRow(i.scratchEncRow, 0)
-	return i.scratchEncRow, nil
+	return i.scratchEncRow, i.container.getEncRow(i.scratchEncRow, 0)
 }
 
 // Row implements the RowIterator interface.
@@ -942,7 +949,10 @@ func (f *DiskBackedIndexedRowContainer) GetRow(
 		}
 	}
 	mrc := f.DiskBackedRowContainer.mrc
-	mrc.getEncRow(mrc.scratchEncRow, pos)
+	err = mrc.getEncRow(mrc.scratchEncRow, pos)
+	if err != nil {
+		return nil, err
+	}
 	rowWithIdx = mrc.scratchEncRow
 	row, rowIdx := rowWithIdx[:len(rowWithIdx)-1], rowWithIdx[len(rowWithIdx)-1].Datum
 	if idx, ok := rowIdx.(*tree.DInt); ok {
diff --git a/pkg/sql/rowenc/encoded_datum.go b/pkg/sql/rowenc/encoded_datum.go
@@ -180,6 +180,9 @@ func EncDatumValueFromBufferWithOffsetsAndType(
 }
 
 // DatumToEncDatum initializes an EncDatum with the given Datum.
+//
+// NB: the method will panic if the datum's resolved type is not equivalent to
+// the passed-in type. Consider using safer DatumToEncDatumEx.
 func DatumToEncDatum(ctyp *types.T, d tree.Datum) EncDatum {
 	ed, err := DatumToEncDatumEx(ctyp, d)
 	if err != nil {
@@ -190,8 +193,6 @@ func DatumToEncDatum(ctyp *types.T, d tree.Datum) EncDatum {
 
 // DatumToEncDatumEx is the same as DatumToEncDatum that returns an error
 // instead of panicking under unexpected circumstances.
-// TODO(yuzefovich): we should probably get rid of DatumToEncDatum in favor of
-// this method altogether.
 func DatumToEncDatumEx(ctyp *types.T, d tree.Datum) (EncDatum, error) {
 	if d == nil {
 		return EncDatum{}, errors.AssertionFailedf("cannot convert nil datum to EncDatum")
diff --git a/pkg/sql/rowexec/aggregator.go b/pkg/sql/rowexec/aggregator.go
@@ -543,7 +543,11 @@ func (ag *aggregatorBase) getAggResults(
 			// We can't encode nil into an EncDatum, so we represent it with DNull.
 			result = tree.DNull
 		}
-		ag.row[i] = rowenc.DatumToEncDatum(ag.outputTypes[i], result)
+		ag.row[i], err = rowenc.DatumToEncDatumEx(ag.outputTypes[i], result)
+		if err != nil {
+			ag.MoveToDraining(err)
+			return aggStateUnknown, nil, nil
+		}
 	}
 
 	if outRow := ag.ProcessRowHelper(ag.row); outRow != nil {
diff --git a/pkg/sql/rowexec/windower.go b/pkg/sql/rowexec/windower.go
@@ -722,7 +722,10 @@ func (w *windower) populateNextOutputRow() (bool, error) {
 		copy(w.outputRow, inputRow[:len(w.inputTypes)])
 		for windowFnIdx, windowFn := range w.windowFns {
 			windowFnRes := w.windowValues[w.partitionIdx][windowFnIdx][rowIdx]
-			encWindowFnRes := rowenc.DatumToEncDatum(w.outputTypes[windowFn.outputColIdx], windowFnRes)
+			encWindowFnRes, err := rowenc.DatumToEncDatumEx(w.outputTypes[windowFn.outputColIdx], windowFnRes)
+			if err != nil {
+				return false, err
+			}
 			w.outputRow[windowFn.outputColIdx] = encWindowFnRes
 		}
 		w.rowsInBucketEmitted++
diff --git a/pkg/sql/span/span_builder.go b/pkg/sql/span/span_builder.go
@@ -437,7 +437,11 @@ func (s *Builder) SpansFromInvertedSpans(
 			scratchRows[i] = make(rowenc.EncDatumRow, keyLength+1)
 			for j := 0; j < keyLength; j++ {
 				val := span.StartKey().Value(j)
-				scratchRows[i][j] = rowenc.DatumToEncDatum(val.ResolvedType(), val)
+				var err error
+				scratchRows[i][j], err = rowenc.DatumToEncDatumEx(val.ResolvedType(), val)
+				if err != nil {
+					return nil, err
+				}
 			}
 		}
 	} else {
diff --git a/pkg/sql/stats/row_sampling.go b/pkg/sql/stats/row_sampling.go
@@ -258,7 +258,11 @@ func (sr *SampleReservoir) copyRow(
 			return err
 		}
 		beforeRowSize += int64(dst[i].Size())
-		sr.scratch[i] = rowenc.DatumToEncDatum(sr.colTypes[i], src[i].Datum)
+		var err error
+		sr.scratch[i], err = rowenc.DatumToEncDatumEx(sr.colTypes[i], src[i].Datum)
+		if err != nil {
+			return err
+		}
 		afterSize := sr.scratch[i].Size()
 
 		// If the datum is too large, truncate it.

Original file line number	Diff line number	Diff line change
`@@ -995,7 +995,10 @@ func (r *DataRecord) rowFromNative(native interface{}) (rowenc.EncDatumRow, erro`
`995`	`995`	`if err != nil {`
`996`	`996`	`return nil, err`
`997`	`997`	`}`
`998`		`- row[r.colIdxByFieldIdx[fieldIdx]] = rowenc.DatumToEncDatum(field.typ, decoded)`
	`998`	`+ row[r.colIdxByFieldIdx[fieldIdx]], err = rowenc.DatumToEncDatumEx(field.typ, decoded)`
	`999`	`+ if err != nil {`
	`1000`	`+ return nil, err`
	`1001`	`+ }`
`999`	`1002`	`}`
`1000`	`1003`	`return row, nil`
`1001`	`1004`	`}`
Original file line number	Diff line number	Diff line change
`@@ -295,7 +295,10 @@ func (h *ProcOutputHelper) ProcessRow(`
`295`	`295`	`if err != nil {`
`296`	`296`	`return nil, false, err`
`297`	`297`	`}`
`298`		`- h.outputRow[i] = rowenc.DatumToEncDatum(h.OutputTypes[i], datum)`
	`298`	`+ h.outputRow[i], err = rowenc.DatumToEncDatumEx(h.OutputTypes[i], datum)`
	`299`	`+ if err != nil {`
	`300`	`+ return nil, false, err`
	`301`	`+ }`
`299`	`302`	`}`
`300`	`303`	`} else if h.outputCols != nil {`
`301`	`304`	`// Projection.`
Original file line number	Diff line number	Diff line change
`@@ -199,7 +199,11 @@ func (p planNodeToRowSource) Next() (rowenc.EncDatumRow, execinfrapb.ProducerM`
`199`	`199`
`200`	`200`	`for i, datum := range p.node.Values() {`
`201`	`201`	`if datum != nil {`
`202`		`- p.row[i] = rowenc.DatumToEncDatum(p.outputTypes[i], datum)`
	`202`	`+ p.row[i], err = rowenc.DatumToEncDatumEx(p.outputTypes[i], datum)`
	`203`	`+ if err != nil {`
	`204`	`+ p.MoveToDraining(err)`
	`205`	`+ return nil, p.DrainHelper()`
	`206`	`+ }`
`203`	`207`	`}`
`204`	`208`	`}`
`205`	`209`	`// ProcessRow here is required to deal with projections, which won't be`
Original file line number	Diff line number	Diff line change
`@@ -1131,7 +1131,10 @@ func (rf *Fetcher) processValueSingle(`
`1131`	`1131`	`if rf.args.TraceKV {`
`1132`	`1132`	`prettyValue = value.String()`
`1133`	`1133`	`}`
`1134`		`- table.row[idx] = rowenc.DatumToEncDatum(typ, value)`
	`1134`	`+ table.row[idx], err = rowenc.DatumToEncDatumEx(typ, value)`
	`1135`	`+ if err != nil {`
	`1136`	`+ return "", "", err`
	`1137`	`+ }`
`1135`	`1138`	`return prettyKey, prettyValue, nil`
`1136`	`1139`	`}`
`1137`	`1140`
Original file line number	Diff line number	Diff line change
`@@ -186,8 +186,7 @@ func (d *DiskBackedNumberedRowContainer) GetRow(`
`186`	`186`	`return nil, nil`
`187`	`187`	`}`
`188`	`188`	`mrc := d.rc.mrc`
`189`		`- mrc.getEncRow(mrc.scratchEncRow, idx)`
`190`		`- return mrc.scratchEncRow, nil`
	`189`	`+ return mrc.scratchEncRow, mrc.getEncRow(mrc.scratchEncRow, idx)`
`191`	`190`	`}`
`192`	`191`	`return d.rowIter.getRow(ctx, idx, skip)`
`193`	`192`	`}`