Merge #152466

152466: tree: harden DArray with NULL elements r=yuzefovich a=yuzefovich

We've had `tree.DArray.HasNulls` and `tree.DArray.HasNonNulls` flags for many years, and they are used as a short-cut in some scenarios (initial use case was for pgwire serialization). However, we haven't had any validation of those flags, and given that we expose `DArray.Array` field, it's possible to construct a datum that doesn't have these flags set correctly (they do get set when using `DArray.Append`).

I did a quick audit of existing spots and found a few where we didn't set the flags, but none seem concerning. To avoid bugs like this from slipping in, this commit changes the API to expose Get and Set methods, and then on every Get for the flag we now do the validation in test-only builds (the validation includes verifying NULL-related flags as well as the length of the array not exceeding MaxInt32).

We could've taken this one step further and hidden `DArray.Array` altogether in favor of explicit Get and Set methods for elements, but I decided it's not worth the effort. I also decide to not force usage of `Append` method in a few places where we're appending to the underlying array in order to not have to deal with an explicit error checking.

Additionally, this commit does some minor cleanup like pulling out a NULL element check outside of the loop and adding missing periods.

Epic: None
Release note: None

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

Author: craig[bot]

pkg/sql/execinfra/execexpr/expr_test.go

@@ -96,11 +96,9 @@ func TestDeserializeExpressionConstantEval(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	expected := &tree.DArray{
-		ParamTyp:    types.Int,
-		Array:       tree.Datums{tree.NewDInt(1), tree.NewDInt(2)},
-		HasNonNulls: true,
-	}
+	expected := tree.NewDArrayFromDatums(
+		types.Int, tree.Datums{tree.NewDInt(1), tree.NewDInt(2)},
+	)
 	if !reflect.DeepEqual(expr, expected) {
 		t.Errorf("invalid expr '%v', expected '%v'", expr, expected)
 	}

pkg/sql/opt/memo/constraint_builder.go

@@ -223,7 +223,7 @@ func (cb *constraintsBuilder) buildSingleColumnConstraintConst(
 
 	case opt.ContainsOp:
 		if arr, ok := datum.(*tree.DArray); ok {
-			if arr.HasNulls {
+			if arr.HasNulls() {
 				return contradiction, true
 			}
 		}

pkg/sql/opt/memo/extract.go

@@ -81,17 +81,11 @@ func ExtractConstDatum(e opt.Expr) tree.Datum {
 
 	case *ArrayExpr:
 		elementType := t.Typ.ArrayContents()
-		a := tree.NewDArray(elementType)
-		a.Array = make(tree.Datums, len(t.Elems))
-		for i := range a.Array {
-			a.Array[i] = ExtractConstDatum(t.Elems[i])
-			if a.Array[i] == tree.DNull {
-				a.HasNulls = true
-			} else {
-				a.HasNonNulls = true
-			}
+		elements := make(tree.Datums, len(t.Elems))
+		for i := range elements {
+			elements[i] = ExtractConstDatum(t.Elems[i])
 		}
-		return a
+		return tree.NewDArrayFromDatums(elementType, elements)
 	}
 	panic(errors.AssertionFailedf("non-const expression: %+v", e))
 }

pkg/sql/opt/memo/interner_test.go

@@ -68,20 +68,13 @@ func TestInterner(t *testing.T) {
 		Typ: tupleTyp3,
 	}
 
-	arr1 := tree.NewDArray(tupTyp1)
-	arr1.Array = tree.Datums{tup1, tup2}
-	arr2 := tree.NewDArray(tupTyp2)
-	arr2.Array = tree.Datums{tup2, tup1}
-	arr3 := tree.NewDArray(tupTyp3)
-	arr3.Array = tree.Datums{tup2, tup3}
-	arr4 := tree.NewDArray(types.Int)
-	arr4.Array = tree.Datums{tree.DNull}
-	arr5 := tree.NewDArray(types.String)
-	arr5.Array = tree.Datums{tree.DNull}
-	arr6 := tree.NewDArray(types.Int)
-	arr6.Array = tree.Datums{}
-	arr7 := tree.NewDArray(types.String)
-	arr7.Array = tree.Datums{}
+	arr1 := tree.NewDArrayFromDatums(tupTyp1, tree.Datums{tup1, tup2})
+	arr2 := tree.NewDArrayFromDatums(tupTyp2, tree.Datums{tup2, tup1})
+	arr3 := tree.NewDArrayFromDatums(tupTyp3, tree.Datums{tup2, tup3})
+	arr4 := tree.NewDArrayFromDatums(types.Int, tree.Datums{tree.DNull})
+	arr5 := tree.NewDArrayFromDatums(types.String, tree.Datums{tree.DNull})
+	arr6 := tree.NewDArrayFromDatums(types.Int, tree.Datums{})
+	arr7 := tree.NewDArrayFromDatums(types.String, tree.Datums{})
 
 	dec1, _ := tree.ParseDDecimal("1.0")
 	dec2, _ := tree.ParseDDecimal("1.0")

pkg/sql/opt/norm/fold_constants_funcs.go

@@ -182,17 +182,11 @@ func (c *CustomFuncs) IsListOfConstants(elems memo.ScalarListExpr) bool {
 // array as a Const datum with type TArray.
 func (c *CustomFuncs) FoldArray(elems memo.ScalarListExpr, typ *types.T) opt.ScalarExpr {
 	elemType := typ.ArrayContents()
-	a := tree.NewDArray(elemType)
-	a.Array = make(tree.Datums, len(elems))
-	for i := range a.Array {
-		a.Array[i] = memo.ExtractConstDatum(elems[i])
-		if a.Array[i] == tree.DNull {
-			a.HasNulls = true
-		} else {
-			a.HasNonNulls = true
-		}
+	elements := make(tree.Datums, len(elems))
+	for i := range elements {
+		elements[i] = memo.ExtractConstDatum(elems[i])
 	}
-	return c.f.ConstructConst(a, typ)
+	return c.f.ConstructConst(tree.NewDArrayFromDatums(elemType, elements), typ)
 }
 
 // IsConstValueOrGroupOfConstValues returns true if the input is a constant,

pkg/sql/opt/props/histogram_test.go

@@ -1178,10 +1178,9 @@ func BenchmarkHistogram(b *testing.B) {
 		case types.StringFamily:
 			return tree.NewDString(strconv.Itoa(i * 2))
 		case types.ArrayFamily:
-			arr := tree.NewDArray(t.ArrayContents())
-			arr.Array = make(tree.Datums, 1)
-			arr.HasNonNulls = true
-			arr.Array[0] = makeDatum(t.ArrayContents(), i)
+			arr := tree.NewDArrayFromDatums(
+				t.ArrayContents(), tree.Datums{makeDatum(t.ArrayContents(), i)},
+			)
 			return arr
 		}
 		panic(errors.AssertionFailedf("unsupported type"))

pkg/sql/pg_catalog.go

@@ -2523,11 +2523,14 @@ https://www.postgresql.org/docs/9.6/view-pg-prepared-statements.html`,
 			paramNames := make([]string, len(placeholderTypes))
 
 			for i, placeholderType := range placeholderTypes {
+				// TODO(yuzefovich): we're including INT8 into array containing
+				// REGTYPE. Figure it out.
 				paramTypes.Array[i] = tree.NewDOidWithTypeAndName(
 					placeholderType.Oid(),
 					placeholderType,
 					placeholderType.SQLStandardName(),
 				)
+				paramTypes.SetHasNonNulls(true /* hasNonNulls */)
 				paramNames[i] = placeholderType.Name()
 			}
 
@@ -2624,7 +2627,7 @@ func addPgProcBuiltinRow(name string, addRow func(...tree.Datum) error) error {
 		}
 
 		getVariadicStringArray := func() tree.Datum {
-			return &tree.DArray{ParamTyp: types.String, Array: tree.Datums{proArgModeVariadic}}
+			return tree.NewDArrayFromDatums(types.String, tree.Datums{proArgModeVariadic})
 		}
 
 		var argmodes tree.Datum

pkg/sql/pgwire/types.go

@@ -843,7 +843,7 @@ func writeBinaryDatumNotNull(
 		}
 		b.putInt32(ndims)
 		hasNulls := 0
-		if v.HasNulls {
+		if v.HasNulls() {
 			hasNulls = 1
 		}
 		oid := v.ParamTyp.Oid()

pkg/sql/rowenc/index_encoding.go

@@ -900,7 +900,7 @@ func encodeContainingArrayInvertedIndexSpans(
 		return invertedExpr, nil
 	}
 
-	if val.HasNulls {
+	if val.HasNulls() {
 		// If there are any nulls, return empty spans. This is needed to ensure
 		// that `SELECT ARRAY[NULL, 2] @> ARRAY[NULL, 2]` is false.
 		return &inverted.SpanExpression{Tight: true, Unique: true}, nil
@@ -981,7 +981,7 @@ func encodeOverlapsArrayInvertedIndexSpans(
 	// we cannot generate an inverted expression.
 
 	// TODO: This should be a contradiction which is treated as a no-op.
-	if val.Array.Len() == 0 || !val.HasNonNulls {
+	if val.Array.Len() == 0 || !val.HasNonNulls() {
 		return inverted.NonInvertedColExpression{}, nil
 	}
 

pkg/sql/rowenc/index_encoding_test.go

@@ -371,34 +371,22 @@ func TestInvertedIndexKey(t *testing.T) {
 		expectedKeysExcludingEmptyArray int
 	}{
 		{
-			value: &tree.DArray{
-				ParamTyp: types.Int,
-				Array:    tree.Datums{},
-			},
+			value:                           tree.NewDArrayFromDatums(types.Int, tree.Datums{}),
 			expectedKeys:                    1,
 			expectedKeysExcludingEmptyArray: 0,
 		},
 		{
-			value: &tree.DArray{
-				ParamTyp: types.Int,
-				Array:    tree.Datums{tree.NewDInt(1)},
-			},
+			value:                           tree.NewDArrayFromDatums(types.Int, tree.Datums{tree.NewDInt(1)}),
 			expectedKeys:                    1,
 			expectedKeysExcludingEmptyArray: 1,
 		},
 		{
-			value: &tree.DArray{
-				ParamTyp: types.Int,
-				Array:    tree.Datums{tree.NewDString("foo")},
-			},
+			value:                           tree.NewDArrayFromDatums(types.String, tree.Datums{tree.NewDString("foo")}),
 			expectedKeys:                    1,
 			expectedKeysExcludingEmptyArray: 1,
 		},
 		{
-			value: &tree.DArray{
-				ParamTyp: types.Int,
-				Array:    tree.Datums{tree.NewDInt(1), tree.NewDInt(2), tree.NewDInt(1)},
-			},
+			value: tree.NewDArrayFromDatums(types.Int, tree.Datums{tree.NewDInt(1), tree.NewDInt(2), tree.NewDInt(1)}),
 			// The keys should be deduplicated.
 			expectedKeys:                    2,
 			expectedKeysExcludingEmptyArray: 2,
@@ -985,8 +973,8 @@ func TestEncodeOverlapsArrayInvertedIndexSpans(t *testing.T) {
 
 		rightArr, _ := right.(*tree.DArray)
 		// An inverted expression can only be generated if the value array is
-		// non-empty or contains atleast one non-NULL element.
-		ok := rightArr.Len() > 0 && rightArr.HasNonNulls
+		// non-empty or contains at least one non-NULL element.
+		ok := rightArr.Len() > 0 && rightArr.HasNonNulls()
 		// A unique span expression can be guaranteed when the input is of
 		// the form:
 		// Array A && Array containing one or more entries of same non-null