sql: allow array builtins to operate on tuples

[rafiss]

fixes #70029

DistSQL does not support this because the DArray operations in eval.go
all result in arrays of AnyTuple, which cannot be decoded.

Release note (sql change): The array builtins (array_agg, array_cat,
array_position, etc) now operate on record types.

[cockroach-teamcity]

This change is 

[rafiss]

Not sure why, but it seems like the SELECT array_cat(ARRAY[ROW(1,2)], ARRAY[NULL::record]) logic test is still using the vectorized engine under the fakedist config

[yuzefovich]

Note that there are failures in fakedist-vec-off and fakedist-metadata configs which don't use the vectorized engine, so the problem is applicable to both execution engines. My quick take is that fakedist config and DistsqlBlocklist don't work together, and it is a test harness issue; if that's true, we should just create a separate logic test file that would only run with local* configs.

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

---

pkg/sql/sem/builtins/builtins.go, line 7060 at r2 (raw file):

		}
	}
	// Prevent usage in DistSQL/vectorized engine because they cannot handle

This doesn't prevent the usage of the vectorized engine, only of queries being distributed.

[rafiss]

The part I don't understand yet is why the other array_cat tests right above that one don't have the same issue?

[yuzefovich]

fakedist is non-deterministic, maybe those got lucky (i.e. the fakeSpanResolver happened to behave as if the cluster was a single node)?

[rafiss]

I figured it out -- the issue is that the array_* builtin would be evaluated during constant-folding, so checking if the builtin function is blocklisted would not work. I added another check for arrays with untyped tuples.

[yuzefovich]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @nvanbenschoten, and @rafiss)

---

pkg/sql/sem/builtins/builtins.go, line 7060 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

This doesn't prevent the usage of the vectorized engine, only of queries being distributed.

Is this needed given the new check?

[rafiss]

i also tried to avoid the array by avoiding creating an array with AnyTuple in https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/sem/tree/eval.go#L321-L327

and instead do

-        for _, e := range MustBeDArray(left).Array {
+        leftArray := MustBeDArray(left)
+        result = NewDArray(leftArray.ResolvedType().ArrayContents())
+        for _, e := range leftArray.Array {

but that gets messy because it still doesn't handle tuple(unknown, int) (and similar types) that well. and for array_cat, it's not clear whether to use the left array or right array's type.

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

---

pkg/sql/sem/builtins/builtins.go, line 7060 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Is this needed given the new check?

don't think so

[rafiss]

i also tried changing the return types of the builtins, and it didn't seem to matter

-			ReturnType: tree.FixedReturnType(types.MakeArray(typ)),
+			ReturnType: tree.IdentityReturnType(0),

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

[rafiss]

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

---

pkg/sql/sem/builtins/builtins.go, line 7060 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

don't think so

actually, i was wrong. it's still needed. i didn't look too far into why, but when i removed this check, then the SELECT array_cat(ARRAY[ROW(10,'fish')], ARRAY[(k,v)]) FROM kv; (the first array of tuple builtin test) test case failed

[yuzefovich]

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

---

pkg/sql/sem/builtins/builtins.go, line 7060 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

actually, i was wrong. it's still needed. i didn't look too far into why, but when i removed this check, then the SELECT array_cat(ARRAY[ROW(10,'fish')], ARRAY[(k,v)]) FROM kv; (the first array of tuple builtin test) test case failed

Ok, thanks.

nit: drop "vectorized engine" from the comment because DistsqlBlocklist doesn't affect it.

[rafiss]

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

---

pkg/sql/sem/builtins/builtins.go, line 7060 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Ok, thanks.

nit: drop "vectorized engine" from the comment because DistsqlBlocklist doesn't affect it.

done

[rafiss]

bors r=yuzefovich

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)