sql, distsql, opt: fixing serialization of empty array

[RaduBerinde]

DArray.AmbiguousFormat now returns true if the array is empty. This
fixes a serialization issue.

The optimizer code now creates a DArray instead of an Array
whenever possible (most importantly, for empty arrays).

Fixes #26087.

Release note (bug fix): Fixed an error caused by empty arrays in some
cases.

[cockroach-teamcity]

This change is 

[andy-kimball]

Review status: 0 of 5 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

pkg/sql/opt/exec/execbuilder/scalar_builder.go, line 293 at r1 (raw file):

func (b *Builder) buildArray(ctx *buildScalarCtx, ev memo.ExprView) (tree.TypedExpr, error) {
	if memo.HasOnlyConstChildren(ev) {

What if this is an array of const tuples? Does this need to recursively check if the entire expression tree is constant?

---

Comments from Reviewable

[RaduBerinde]

Review status: 0 of 5 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/sql/opt/exec/execbuilder/scalar_builder.go, line 293 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

What if this is an array of const tuples? Does this need to recursively check if the entire expression tree is constant?

Hm, good question. The same question applies for buildTuples (and potentially other uses of MatchesTupleOfConstants). I'll look at it and try to add that.

---

Comments from Reviewable

[RaduBerinde]

I moved the const DArray and DTuple construction to ExtractConstDatum, and made the check recursive, PTAL

[andy-kimball]

Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/sql/opt/memo/extract.go, line 32 at r2 (raw file):

//  - one that has a ConstValue tag, or
//  - a tuple or array where all children are constant values.
func ExtractConstDatum(ev ExprView) tree.Datum {

Can you review all the places that call this, and make sure this is the right method for them to be calling, now that it's so much more extensive? For example, logical_props_builder calls ExtractConstDatum just to check whether the constant is null or not - that should be changed. And factory.go calls it on every comparison, which will trigger a lot of object creation, if it's not one of the simple types. Another possibility is to split this method, or add a flag to avoid deep unpacking.

---

Comments from Reviewable

[RaduBerinde]

I added a commit relaxing one of the IsConstValue checks and we now create constraints on columns that are tuples.

---

Review status: 0 of 9 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/sql/opt/memo/extract.go, line 32 at r2 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

Can you review all the places that call this, and make sure this is the right method for them to be calling, now that it's so much more extensive? For example, logical_props_builder calls ExtractConstDatum just to check whether the constant is null or not - that should be changed. And factory.go calls it on every comparison, which will trigger a lot of object creation, if it's not one of the simple types. Another possibility is to split this method, or add a flag to avoid deep unpacking.

Hm, I looked at the paths using MatchesTupleOfConstants and they seemed reasonable. Any other uses of ExtractConstDatum can't call the function on tuples or arrays (or they would panic today). The one you mention in buildProjectProps is behind a IsConstValue check.

If we find that we generate the same objects many times, we can cache the datum in the tuple's private.

---

Comments from Reviewable

[andy-kimball]

---

Review status: 0 of 9 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/sql/opt/memo/extract.go, line 32 at r2 (raw file):

Previously, RaduBerinde wrote…

Hm, I looked at the paths using MatchesTupleOfConstants and they seemed reasonable. Any other uses of ExtractConstDatum can't call the function on tuples or arrays (or they would panic today). The one you mention in buildProjectProps is behind a IsConstValue check.

If we find that we generate the same objects many times, we can cache the datum in the tuple's private.

ok, sounds reasonable, i didn't catch that IsConstValue was filtering out the potentially expensive cases

---

Comments from Reviewable

[RaduBerinde]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)