opt: fold unary minus operations

[justinj]

This commit introduces a new rule, FoldUnaryMinus, which transforms
constant expressions of the form (UnaryMinus x) to -x.

There were some cases around this which would result in us not correctly
recognizing expressions like -1:::FLOAT as constant, causing us to
perform a full table scan instead of a point lookup.

Release note: None

[cockroach-teamcity]

This change is 

[andy-kimball]

Review status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/opt/norm/custom_funcs.go, line 766 at r1 (raw file):

// NegateNumeric applies a unary minus to a numeric value.
func (c *CustomFuncs) NegateNumeric(input memo.GroupID) memo.GroupID {

I don't think we should duplicate this logic. We should be using the logic in eval.go, for example in UnaryOps[UnaryMinus].

---

pkg/sql/opt/norm/custom_funcs.go, line 777 at r1 (raw file):

		id = c.f.InternDatum(tree.NewDFloat(-*t))
	case *tree.DInt:
		id = c.f.InternDatum(tree.NewDInt(-*t))

What if the existing value is -MIN_INT, so there's no way to negate it without overflow?

I think you need: CanFoldUnaryMinus and FoldUnaryMinus helper functions. We'll want function pairs like this for all the constant folding cases (Plus, Minus, Multiple, etc.). Our other option is to define an Error operator that will trigger a run-time error. That would cut down on needing lots of Can functions, since we could generate one of those when we detect overflow.

@radu, what do you think the best approach would be?

---

Comments from Reviewable

[justinj]

Review status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/opt/norm/custom_funcs.go, line 766 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

I don't think we should duplicate this logic. We should be using the logic in eval.go, for example in UnaryOps[UnaryMinus].

Pulled it out into functions.

---

pkg/sql/opt/norm/custom_funcs.go, line 777 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

What if the existing value is -MIN_INT, so there's no way to negate it without overflow?

I think you need: CanFoldUnaryMinus and FoldUnaryMinus helper functions. We'll want function pairs like this for all the constant folding cases (Plus, Minus, Multiple, etc.). Our other option is to define an Error operator that will trigger a run-time error. That would cut down on needing lots of Can functions, since we could generate one of those when we detect overflow.

@radu, what do you think the best approach would be?

Done, I did the CanFoldUnaryMinus approach for now since there are CCL tests which require this change before turning on the optimizer by default, we can revisit the Error approach afterwards, if need be.

---

Comments from Reviewable

[rytaft]

---

Reviewed 1 of 4 files at r1, 7 of 7 files at r2.
Review status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/sql/opt/norm/testdata/rules/combo, line 138 at r2 (raw file):

  +           └── a.k = xy.x [type=bool, outer=(1,6), constraints=(/1: (/NULL - ]; /6: (/NULL - ])]
================================================================================
PruneJoinRightCols

Did you run make generate PKG=./pkg/sql/opt...? I think these diffs are here because pkg/sql/opt/rule_name_string.go needs to be updated

---

pkg/sql/sem/tree/eval.go, line 67 at r2 (raw file):

const SecondsInDay = 24 * 60 * 60

// Negate returns the negative of a non-math.MinInt64 DInt.

Maybe say "should not be called with math.MinInt64, since that will cause integer overflow" or something like that...

---

Comments from Reviewable

[justinj]

As discussed on Slack I removed Negate and introduced a function to evaluate a unary op applied to a constant.

---

Review status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/sql/sem/tree/eval.go, line 67 at r2 (raw file):

Previously, rytaft wrote…

Maybe say "should not be called with math.MinInt64, since that will cause integer overflow" or something like that...

Ended up removing this

---

pkg/sql/opt/norm/testdata/rules/combo, line 138 at r2 (raw file):

Previously, rytaft wrote…

Did you run make generate PKG=./pkg/sql/opt...? I think these diffs are here because pkg/sql/opt/rule_name_string.go needs to be updated

Done.

---

Comments from Reviewable

[rytaft]

Reviewed 5 of 5 files at r3.
Review status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/sql/opt/memo/typing.go, line 353 at r3 (raw file):

// a datum referring to the evaluated result. If an appropriate overload is not
// found, EvalUnaryOp returns an error.
func EvalUnaryOp(evalCtx *tree.EvalContext, op opt.Operator, ev ExprView) (tree.Datum, error) {

I'm not convinced this is the right place for this function. Why not just put it in norm/custom_funcs.go where it's called?

---

Comments from Reviewable

[andy-kimball]

Review status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/sql/opt/memo/typing.go, line 353 at r3 (raw file):

Previously, rytaft wrote…

I'm not convinced this is the right place for this function. Why not just put it in norm/custom_funcs.go where it's called?

This function doesn't really fit perfectly in either place. We shouldn't be searching for overloads in norm (that logic needs to be in a single place). But we shouldn't be evaluating functions in typing.go. Probably we should have a FindUnaryOverload function in typing.go, and then the EvalUnaryOp function in norm. But I don't think we should do that right now, so I'm OK with the function living in either place until we can factor it better.

---

Comments from Reviewable

[rytaft]

Review status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/sql/opt/memo/typing.go, line 353 at r3 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

This function doesn't really fit perfectly in either place. We shouldn't be searching for overloads in norm (that logic needs to be in a single place). But we shouldn't be evaluating functions in typing.go. Probably we should have a FindUnaryOverload function in typing.go, and then the EvalUnaryOp function in norm. But I don't think we should do that right now, so I'm OK with the function living in either place until we can factor it better.

ok sounds good

---

Comments from Reviewable

[justinj]

TFTRs, going to rebase+merge this

[justinj]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)