sql: internal error: DECIMAL: could not evaluate

[maddyblue]

CREATE TABLE table1 ();

SELECT
        3.432390042674610877E+21 / 'Infinity'
        + (-8328160919613561056)
FROM
        table1;

github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/expr.go:78: processExpression()
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/expr.go:160: init()
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/processors.go:146: Init()
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/processors.go:844: InitWithEvalCtx()
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/processors.go:820: Init()
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/tablereader.go:112: newTableReader()
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/processors.go:1028: newProcessor()
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/flow.go:321: makeProcessor()
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/flow.go:396: setupProcessors()
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/flow.go:479: setup()
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/server.go:457: setupFlow()
github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/server.go:512: SetupLocalSyncFlow()
github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:237: setupFlows()
github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:326: Run()
github.com/cockroachdb/cockroach/pkg/sql/distsql_running.go:962: PlanAndRun()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:816: execWithDistSQLEngine()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:708: dispatchToExecutionEngine()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:417: execStmtInOpenState()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:98: execStmt()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1238: execCmd()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1167: run()
github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:444: ServeConn()
github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:584: func1()

[rafiss]

I've been looking into this. The problem seems to be coming from constant.go ResolveAsType() when it tries to deal with the following value:

8328160919613561056.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

It calls the following function, which can only deal with a precision of 2000 places, but the above string has 2019 trailing zeroes.

// SetString sets d to s. Any non-standard NaN values are converted to a
// normal NaN. Any negative zero is converted to positive.
func (d *DDecimal) SetString(s string) error {
	// Using HighPrecisionCtx here restricts the max and min exponents to 2000,
	// and the precision to 2000 places. Any rounding or other inexact conversion
	// will result in an error.
	_, res, err := HighPrecisionCtx.SetString(&d.Decimal, s)
	if res != 0 || err != nil {
		return makeParseError(s, types.Decimal, nil)
	}
        // ...
}

Next I'll try to figure out why the number ends up getting represented like that.

[rafiss]

I found that any decimal division with a divisor of Infinity will result in a value with an Exponent of etiny(). That comes from the quoSpecials() function in apd, which sets d.Exponent = c.etiny()

// etiny returns the smallest value an Exponent can contain.
func (c *Context) etiny() int32 {
	return c.MinExponent - int32(c.Precision) + 1
}

In this case, the constant folding uses eval.go, which uses the context DecimalCtx, which has a precision of 20 and a min exponent of -2000.

But later (as mentioned in the above comment), SetString() in datum.go uses HighPrecisionCtx, which cannot handle that exponent.

@mjibson I believe you have the most knowledge about how we should be using decimals and the various contexts. Do you know why HighPrecisionCtx is used in datum.go, and why DecimalCtx is used in eval.go? Does your comment here (#39540 (comment)) indicate that we should actually be using ExactCtx everywhere?

Also, as an aside, I am curious why we have implemented x / Inf to result in 0E-2019 instead of 0.

[maddyblue]

Do you know why HighPrecisionCtx is used in datum.go, and why DecimalCtx is used in eval.go?

The various decimal contexts were chosen by me to try to match the postgres behavior of the corresponding operation, but stuff dealing with precision and exponents near the edge has not been fully tested or made identical. We could probably change folding to use HighPrecisionCtx? Although I don't think that would affect this bug at all.

I'm not exactly clear on why or when SetString is getting called? Is it at the end of constant folding when converting to a typed datum?

We have a few options:

1. SetString shouldn't care about the number of digits in the decimal. It doesn't in postgres, which appears to accept a decimal of any length. Unclear what affect this will have on the rest of the decimal operations. We could try using ExactCtx in SetString?
2. Constant folding could call Reduce to remove trailing zeros. But I think we could still come up with things that SetString would error on.

Does your comment here (#39540 (comment)) indicate that we should actually be using ExactCtx everywhere?

No, it should be used only in the same places where it's used in normal execution. It's used in decimal - decimal subtraction:

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

Line 744 in f81ac2b

_, err := ExactCtx.Sub(&dd.Decimal, l, r)

I am curious why we have implemented x / Inf to result in 0E-2019 instead of 0.

This is to match the gda spec for divide. See the tests at https://github.com/cockroachdb/apd/blob/master/testdata/divide.decTest#L601. That is, none of here made this decision, we just followed the spec. Most decimal implementations around follow that spec (java, python, etc.).

[rafiss]

Thanks for the very helpful background and suggestions! I have a slightly better picture of what's going on.

First off, here is a simple repro:

	var d1, d2, d3 apd.Decimal
	var final, quo, sum apd.Decimal
	d1.SetString("1")
	d2.SetString("Infinity")
	d3.SetString("1")

        // DecimalCtx is used in eval.go division
	tree.DecimalCtx.Quo(&quo, &d1, &d2)
        // ExactCtx is used in eval.go addition
	tree.ExactCtx.Add(&sum, &quo, &d3)
        // HighPrecisionCtx is used in datum.go SetString
	_, res, err := tree.HighPrecisionCtx.SetString(&final, sum.String())
	if res != 0 || err != nil {
		panic("parse error")
	}

I'm not exactly clear on why or when SetString is getting called? Is it at the end of constant folding when converting to a typed datum?

SetString ends up getting called during the distsql engine's setup flow stage. It makes a distsql processor for each node in the SQL command. In this case it fails when handling the RenderExpr as it deals with the result of the constant-folding. It calls processExpression() in distsqlrun/expr.go and that is the thing that calls SetString with that really long decimal value as an argument.

As it turns out, any one of the following things ends up solving the error.

1. Using ExactCtx in datum.go SetString.
2. Using HighPrecisionCtx for eval.go addition.
3. Calling Reduce after constant folding. Though I'm not sure where exactly we would want to do this, one option is in pkg/sql/opt/norm/factory.go ConstructConstVal.

If we do options 1 or 2, then the SQL query in the original bug report produces a result that has 2019 decimal places. If we do option 3, then the result has no decimal places.

Based on your comment, it seems like maybe option 1 is the best approach and is desirable for other reasons. And maybe it would be safe to do 3 as well for good measure, though I am not sure if this would violate the spec that you referenced.

I'm guessing you already knew this, but it seems like as far as Postgres goes, it does not support Infinity for decimal types, so it's possible we have some flexibility around some of these decisions.