Unexpected Result of a complex expression

[bajinsheng]

Describe the problem

The distributed plan gets the wrong result.

To Reproduce

create table t0(c0 text);
insert into t0 values('/a');
select * from t0 join (select 1) on not(-61.100 // 79 || c0 <= c0); -- {/a|1}, Expected: {}

If the query is executed by a distributed plan, we get a one-row result, which is unexpected, as not (-61.100 // 79 || c0 <= c0) is always false.

                          info                          
--------------------------------------------------------
 distribution: full
 vectorized: true
 
 • render
 │
 └── • filter
     │ filter: c0 < (-0 || c0)
     │
     └── • scan
           missing stats
           table: t0@t0_pkey
           spans: FULL SCAN

Expected behavior
The result should be empty.

Environment:

• CockroachDB version [7625d9f]
• Server OS: [Linux/Ubuntu]
• Client app [psql]

cockroach start --insecure --store=node1 --locality=region=r1,zone=r1a --listen-addr=0.0.0.0:26257 --http-addr=0.0.0.0:8080 --join=127.0.0.1:26257,127.0.0.1:26258,127.0.0.1:26259 --background > node1.log 2>&1
cockroach start --insecure --store=node2 --locality=region=r2,zone=r2a --listen-addr=0.0.0.0:26258 --http-addr=0.0.0.0:8081 --join=127.0.0.1:26257,127.0.0.1:26258,127.0.0.1:26259 --background > node2.log 2>&1
cockroach init --insecure --host=127.0.0.1:26257

Jira issue: CRDB-51513

[blathers-crl]

Hello, I am Blathers. I am here to help you get the issue triaged.

Hoot - a bug! Though bugs are the bane of my existence, rest assured the wretched thing will get the best of care here.

I have CC'd a few people who may be able to assist you:

• @cockroachdb/sql-queries (found keywords: vectorized,plan)

If we have not gotten back to your issue within a few business days, you can try the following:

• Join our community slack channel and ask on #cockroachdb.
• Try find someone from here if you know they worked closely on the area and CC them.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[mgartner]

We reduced this down to this logic test:

# File: pkg/sql/logictest/testdata/logic_test/__test
statement ok
CREATE TABLE t (i INT);

statement ok
INSERT INTO t VALUES(0);

query R
SELECT -1.0 // 2 FROM t
----
-0

This test fails fairly quickly under the fakedist configuration:

./dev test './pkg/sql/logictest/tests/fakedist' -f 'TestLogic_tmp' --stress

[yuzefovich]

This reproduction should fail every time (although it's an incorrect test - PG would always report 0 and not -0):

# LogicTest: 5node

statement ok
CREATE TABLE t (i INT);

statement ok
INSERT INTO t VALUES(0);

retry
statement ok
ALTER TABLE t EXPERIMENTAL_RELOCATE VALUES (ARRAY[2], 2)

statement ok
SET distsql = on;

query R
SELECT -1.0 // 2 FROM t
----
-0

Disabling DistSQL everywhere will make it easier to debug:

diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go
index ece0fada32a..61a62853a55 100644
--- a/pkg/sql/exec_util.go
+++ b/pkg/sql/exec_util.go
@@ -606,7 +606,7 @@ var DistSQLClusterExecMode = settings.RegisterEnumSetting(
        settings.ApplicationLevel,
        "sql.defaults.distsql",
        "default distributed SQL execution mode",
-       "auto",
+       "off",
        map[sessiondatapb.DistSQLExecMode]string{
                sessiondatapb.DistSQLOff:    "off",
                sessiondatapb.DistSQLAuto:   "auto",
diff --git a/pkg/sql/logictest/logictestbase/logictestbase.go b/pkg/sql/logictest/logictestbase/logictestbase.go
index a0b498a3d25..6771d4b12f3 100644
--- a/pkg/sql/logictest/logictestbase/logictestbase.go
+++ b/pkg/sql/logictest/logictestbase/logictestbase.go
@@ -339,7 +339,7 @@ var LogicTestConfigs = []TestClusterConfig{
        {
                Name:                "5node",
                NumNodes:            5,
-               OverrideDistSQLMode: "on",
+               OverrideDistSQLMode: "off",
                // Have to disable the default test tenant here as there are test run in
                // this mode which try to modify zone configurations and we're more
                // restrictive in the way we allow zone configs to be modified by

What happens here is that we correctly serialize negative zero as (-0):::DECIMAL in Expression.Expr, but then on the remote node we parse this expression in execexpr.helper.deserializeExpr which produces a regular zero, which is constant-evaluated in that method to DDecimal with a regular zero.

[yuzefovich]

Correction: we correctly parse the expression into tree.NumVal with negative=true. We lose that negation during type-checking when we resolve the constant to a datum due to this logic

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

Lines 395 to 401 in 48b25b6

	if !dd.IsZero() {
	// Negative zero does not exist for DECIMAL, in that case we
	// ignore the sign. Otherwise XOR the signs of the expr and the
	// decimal value contained in the expr, since the negative may
	// have been folded into the inner decimal.
	dd.Negative = dd.Negative != expr.negative
	}

.

[yuzefovich]

Yet another clarification: we lose the negative sign here

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

Lines 1053 to 1055 in 48b25b6

	if d.IsZero() && d.Negative {
	d.Negative = false
	}

which was added in. The logic I mentioned above hardens that to truly ensure that we never produce negative zero decimal (and was added in f4658b4).

The behavior overall matches PG14:

yuzefovich=# select '-0.0000'::DECIMAL;
 numeric 
---------
  0.0000
(1 row)

yuzefovich=# select '-0.0000'::FLOAT;
 float8 
--------
     -0
(1 row)

Not sure what to do about this though. Perhaps we want to allow preservation of the negative decimal zero for intermediate computation when parsing the serialized expression on the remote node (e.g. we could adjust SemaCtx we have there), but then we'll need to catch decimals before communicating them outside of the execution pipeline (which includes call sites around DistSQLReceiver / PG wire encoding and when writing datums to disk). In other words, we'd effectively need to lift this logic (for dropping the negation I pointed at above) to boundaries of the execution engine, which seems error-prone ...

[mgartner]

Another option is to audit all places where a new DDecimal can be created, e.g., built-ins, parsing, etc., and ensure that we always set Negative to false.