colbuilder: optimize IS DISTINCT FROM NULL when null is casted

[yuzefovich]

We have an optimized operator for Is{Not}DistinctFrom operation which
we can plan currently only if the right side is a constant NULL. In some
cases the optimizer might create a cast expression on the right in order
to propagate the type of the null, and previously we would fallback to
the default comparison operator in such scenario. This is suboptimal,
and this commit fixes the issue by special casing the scenario of
casting NULL to some type.

Fixes: #63792.

Release note: None

[cockroach-teamcity]

This change is 

[yuzefovich]

I guess this pre-evaluation is similar to "constant folding" in the optimizer. I'm curious whether there are other cases in which we can know that a tree.TypedExpr evaluates to a constant so that we could pre-evaluate it.

[RaduBerinde]

Normally, the optimizer folds all casts. The exception is with Nulls, for which there is no way to pass them down directly with the correct type.

[yuzefovich]

Makes sense, thanks.

[michae2]

Reviewed 2 of 2 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @yuzefovich)

---

pkg/sql/colexec/colbuilder/execplan.go, line 2165 at r1 (raw file):

		// The projection result will be outputted to a new column which is
		// appended to the input batch.
		op, err = colexecproj.GetProjectionLConstOperator(

It looks like looks like colexecproj.GetProjectionLConstOperator calls colconv.GetDatumToPhysicalFn which panics if passed types.Unknown, so I'm not sure if it is a good idea to do this pre-evaluation for the left side. But this is theoretical, because I also couldn't figure out how to get the optimizer to emit a constant null on the left side of a binary operator 😄

[yuzefovich]

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

---

pkg/sql/colexec/colbuilder/execplan.go, line 2165 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

It looks like looks like colexecproj.GetProjectionLConstOperator calls colconv.GetDatumToPhysicalFn which panics if passed types.Unknown, so I'm not sure if it is a good idea to do this pre-evaluation for the left side. But this is theoretical, because I also couldn't figure out how to get the optimizer to emit a constant null on the left side of a binary operator 😄

In this case the null is well-typed after evaluating the cast expression (the whole idea of creating a cast expression like NULL:INT8 in the first place is to propagate the correct type), so we will see types.Int and not types.Unknown (the latter is only for "untyped" nulls).

[michae2]

Reviewed 1 of 1 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @yuzefovich)

---

pkg/sql/colexec/colbuilder/execplan.go, line 2165 at r1 (raw file):
Ah, I'm confused then, because when I was looking in the debugger it did not look well-typed. (This is right, not left, because like I said I can't figure out how to get the optimizer to emit a null on the left side.)

(dlv) print right
github.com/cockroachdb/cockroach/pkg/sql/sem/tree.TypedExpr(github.com/cockroachdb/cockroach/pkg/sql/sem/tree.dNull) {}

(dlv) call right.ResolvedType()
> github.com/cockroachdb/cockroach/pkg/sql/colexec/colbuilder.planProjectionExpr() ./pkg/sql/colexec/colbuilder/execplan.go:2145 (PC: 0x6f3ca86)
Warning: debugging optimized function
Values returned:
	~r0: *github.com/cockroachdb/cockroach/pkg/sql/types.T {
		InternalType: github.com/cockroachdb/cockroach/pkg/sql/types.InternalType {
			Family: UnknownFamily (13),
			Width: 0,
			Precision: 0,
			ArrayDimensions: []int32 len: 0, cap: 0, nil,
			Locale: *"",
			VisibleType: 0,
			ArrayElemType: *github.com/cockroachdb/cockroach/pkg/sql/types.Family nil,
			TupleContents: []*github.com/cockroachdb/cockroach/pkg/sql/types.T len: 0, cap: 0, nil,
			TupleLabels: []string len: 0, cap: 0, nil,
			Oid: T_unknown (705),
			ArrayContents: *github.com/cockroachdb/cockroach/pkg/sql/types.T nil,
			TimePrecisionIsSet: false,
			IntervalDurationField: *github.com/cockroachdb/cockroach/pkg/sql/types.IntervalDurationField nil,
			GeoMetadata: *github.com/cockroachdb/cockroach/pkg/sql/types.GeoMetadata nil,
			UDTMetadata: *github.com/cockroachdb/cockroach/pkg/sql/types.PersistentUserDefinedTypeMetadata nil,},
		TypeMeta: github.com/cockroachdb/cockroach/pkg/sql/types.UserDefinedTypeMetadata {
			Name: *github.com/cockroachdb/cockroach/pkg/sql/types.UserDefinedTypeName nil,
			Version: 0,
			EnumData: *github.com/cockroachdb/cockroach/pkg/sql/types.EnumMetadata nil,},}

  2140:		}
  2141:		right, err = preEvaluateConstCast(evalCtx, right)
  2142:		if err != nil {
  2143:			return nil, resultIdx, typs, err
  2144:		}
=>2145:		allocator := colmem.NewAllocator(ctx, acc, factory)
  2146:		resultIdx = -1
  2147:		// There are 3 cases. Either the left is constant, the right is constant,
  2148:		// or neither are constant.
  2149:		if lConstArg, lConst := left.(tree.Datum); lConst {
  2150:			// Case one: The left is constant.

But I see the TypedExpr wrapper on there... hmm. Am I reading the debugger output wrong?

[yuzefovich]

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

---

pkg/sql/colexec/colbuilder/execplan.go, line 2165 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Ah, I'm confused then, because when I was looking in the debugger it did not look well-typed. (This is right, not left, because like I said I can't figure out how to get the optimizer to emit a null on the left side.)

(dlv) print right
github.com/cockroachdb/cockroach/pkg/sql/sem/tree.TypedExpr(github.com/cockroachdb/cockroach/pkg/sql/sem/tree.dNull) {}

(dlv) call right.ResolvedType()
> github.com/cockroachdb/cockroach/pkg/sql/colexec/colbuilder.planProjectionExpr() ./pkg/sql/colexec/colbuilder/execplan.go:2145 (PC: 0x6f3ca86)
Warning: debugging optimized function
Values returned:
	~r0: *github.com/cockroachdb/cockroach/pkg/sql/types.T {
		InternalType: github.com/cockroachdb/cockroach/pkg/sql/types.InternalType {
			Family: UnknownFamily (13),
			Width: 0,
			Precision: 0,
			ArrayDimensions: []int32 len: 0, cap: 0, nil,
			Locale: *"",
			VisibleType: 0,
			ArrayElemType: *github.com/cockroachdb/cockroach/pkg/sql/types.Family nil,
			TupleContents: []*github.com/cockroachdb/cockroach/pkg/sql/types.T len: 0, cap: 0, nil,
			TupleLabels: []string len: 0, cap: 0, nil,
			Oid: T_unknown (705),
			ArrayContents: *github.com/cockroachdb/cockroach/pkg/sql/types.T nil,
			TimePrecisionIsSet: false,
			IntervalDurationField: *github.com/cockroachdb/cockroach/pkg/sql/types.IntervalDurationField nil,
			GeoMetadata: *github.com/cockroachdb/cockroach/pkg/sql/types.GeoMetadata nil,
			UDTMetadata: *github.com/cockroachdb/cockroach/pkg/sql/types.PersistentUserDefinedTypeMetadata nil,},
		TypeMeta: github.com/cockroachdb/cockroach/pkg/sql/types.UserDefinedTypeMetadata {
			Name: *github.com/cockroachdb/cockroach/pkg/sql/types.UserDefinedTypeName nil,
			Version: 0,
			EnumData: *github.com/cockroachdb/cockroach/pkg/sql/types.EnumMetadata nil,},}

  2140:		}
  2141:		right, err = preEvaluateConstCast(evalCtx, right)
  2142:		if err != nil {
  2143:			return nil, resultIdx, typs, err
  2144:		}
=>2145:		allocator := colmem.NewAllocator(ctx, acc, factory)
  2146:		resultIdx = -1
  2147:		// There are 3 cases. Either the left is constant, the right is constant,
  2148:		// or neither are constant.
  2149:		if lConstArg, lConst := left.(tree.Datum); lConst {
  2150:			// Case one: The left is constant.

But I see the TypedExpr wrapper on there... hmm. Am I reading the debugger output wrong?

No, you're totally right, nice catch:

cockroach/pkg/sql/sem/tree/eval.go

Lines 3849 to 3851 in d93d9d6

	if d == DNull {
	return d, nil
	}

I'm not yet sure whether this PR needs an adjustment, but I'll need to think a bit more about it.

[yuzefovich]

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

---

pkg/sql/colexec/colbuilder/execplan.go, line 2165 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

No, you're totally right, nice catch:

cockroach/pkg/sql/sem/tree/eval.go

Lines 3849 to 3851 in d93d9d6

	if d == DNull {
	return d, nil
	}

I'm not yet sure whether this PR needs an adjustment, but I'll need to think a bit more about it.

I decided to lean on the safer side and introduce some special case behavior only for Is{Not}DistinctFrom operations.

[michae2]

Reviewed 1 of 1 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis)

[yuzefovich]

TFTR!

bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)