coltypes: don't shallow copy decimals

[jordanlewis]

This commit fixes a correctness bug caused by illegal shallow copy of
decimals. To copy a decimal, you must use the .Set method on it, and its
memory must already be distinct from the source decimal.

To facilitate this and generally simplify things, the "safe" GET method
is removed and replaced by COPYVAL, which copies a scalar value to
another scalar value safely. There were fewer than 5 users of GET, so
this wasn't particularly disruptive.

Closes #39777.
Closes #39540.

Release note: None

[cockroach-teamcity]

This change is 

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @yuzefovich)

---

pkg/col/coltypes/types.go, line 201 at r1 (raw file):

tmp.Set(&%[1]s[%[2]s])
%[1]s[%[2]s].Set(&%[1]s[%[3]s])
%[1]s[%[3]s].Set(&%[1]s[%[2]s])`, target, i, j)

not sure if this is terribly readable -- you could consider using the inline templating idea that you have me for overloads.go

also, i think for the multiline ones it would be nicer to have some consistent indentation -- like:

return fmt.Sprintf(`
  var tmp apd.Decimal
  tmp.Set(&%[1]s[%[2]s])
  %[1]s[%[2]s].Set(&%[1]s[%[3]s])
`

---

pkg/col/coltypes/types.go, line 221 at r1 (raw file):

	case Decimal:
		return fmt.Sprintf(`
__tgt_slice := %[1]s[%[2]s:]

i guess it's fine that these variables share the outer scope? in overloads.go i had been introducing a new scope for all the cases where i defined a new variable, just to be safe, but maybe that's overkill?

[jordanlewis]

Okay, I redid this to be better with the templates.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @yuzefovich)

---

pkg/col/coltypes/types.go, line 201 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

not sure if this is terribly readable -- you could consider using the inline templating idea that you have me for overloads.go

also, i think for the multiline ones it would be nicer to have some consistent indentation -- like:

return fmt.Sprintf(`
  var tmp apd.Decimal
  tmp.Set(&%[1]s[%[2]s])
  %[1]s[%[2]s].Set(&%[1]s[%[3]s])
`

Done.

---

pkg/col/coltypes/types.go, line 221 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i guess it's fine that these variables share the outer scope? in overloads.go i had been introducing a new scope for all the cases where i defined a new variable, just to be safe, but maybe that's overkill?

Done.

[yuzefovich]

Reviewed 8 of 9 files at r1, 2 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @rafiss)

---

pkg/col/coltypes/types.go, line 196 at r2 (raw file):

// Swap is a function that should only be used in templates.
func (t T) Swap(target, i, j string) string {

I'm proposing to remove this function in the cleanup-vec PR.

---

pkg/sql/logictest/testdata/logic_test/vectorize, line 705 at r2 (raw file):

	tab_426214._bool ASC
----
1296 values hashing to cad02075a867c3c0564bf80fe665eed6

Huh, what is this output?

[jordanlewis]

Huh, what is this output?

You can have logic test compare the hash of the output to avoid having to write all of the rows, since in this case there are many.

[jordanlewis]

PTAL - I think this is ready to go.

[rafiss]

Reviewed 12 of 12 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @rafiss)

[jordanlewis]

Thanks for the reviews!

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)