sql: preserve decimal negative zero in serialized expressions

[Alignyx]

Summary

Preserve arithmetic-produced DECIMAL negative zero when serializing expressions for remote execution. This fixes the plan-dependent result reported in #148279 while retaining CockroachDB's existing local arithmetic and DECIMAL-to-STRING behavior.

This is a scoped consistency fix, not a complete resolution of the PostgreSQL-compatibility and zero-normalization questions in the issue discussion. The unresolved behavior and the shared formatter's external effects are described below for review.

Problem and root cause

The reported query is:

CREATE TABLE t0(c0 TEXT);
INSERT INTO t0 VALUES ('/a');
SELECT * FROM t0 JOIN (SELECT 1)
  ON NOT(-61.100 // 79 || c0 <= c0);

On the baseline, local execution returns no rows, but a plan executing the filter remotely returns one row. The cause is a mismatch between the representation produced by arithmetic and the representation accepted by ordinary SQL decimal literal resolution:

1. DECIMAL integer division can produce an APD finite zero with Negative = true. Numerically it compares equal to positive zero, but the existing DECIMAL-to-STRING conversion exposes the sign as '-0'.
2. Local expression execution can retain that DDecimal in Expression.LocalExpr. Remote expression construction instead formats it through FmtCheckEquivalence into Expression.Expr, previously as (-0):::DECIMAL.
3. On the receiver, execexpr.helper.deserializeExpr parses, type-checks, and pre-evaluates the expression. As clarified in the issue, parsing into NumVal retains the negative flag; ordinary decimal construction/type resolution canonicalizes zero. setDecimalString removes the negative sign from zero, and NumVal.ResolveAsType also avoids applying a negative sign to a zero decimal. The remote datum therefore becomes positive zero.
4. The filter consequently compares '-0/a' locally but '0/a' remotely. For the input '/a', that changes the predicate's truth value and whether the row is returned.

The invalid transition is the expression datum serialization/deserialization round trip under the current execution semantics. It is not a different division algorithm or a vectorized comparison defect. The literal normalization itself is deliberate; changing it globally would be a separate semantic decision.

Relevant paths are pkg/sql/physicalplan/expression.go, pkg/sql/execinfra/execexpr/expr.go, and pkg/sql/sem/tree/{datum,constant}.go. This agrees with the issue's final root-cause clarification. Historical commit f4658b45 also deliberately distinguishes preservation of FLOAT negative zero from DECIMAL literal normalization.

How the cause was isolated

The investigation used type- and representation-preserving metamorphic comparisons, followed by query-plan, actual execution, and source-path comparison. The baseline was 8812064a015d2faf99d3fc7e15880f94042954b0; the candidate is 27ec5128087eebed707527e1fe313759ef600fd5.

Three independent three-node baseline runs compared the following paths, then repeated the same observations on the candidate:

Query/path	Baseline row count	Candidate row count
Original expression, local execution	0	0
Original expression, remote execution	1	0
Explicit (-61.100 // 79)::STRING before concatenation, remote execution	0	0
Remote division using stored num DECIMAL = -61.100 and den INT = 79	0	0

The explicit cast is a useful control because the already-computed STRING '-0' survives the expression boundary. The column-based control performs the identical typed arithmetic after that boundary and produces the signed zero on the remote node. Row-oriented and vectorized execution both exhibited the original mismatch.

EXPLAIN (OPT, VERBOSE) showed the folded decimal expression versus the explicitly converted STRING expression. EXPLAIN ANALYZE verified that the compared filters actually executed on node n2 and checked their actual row counts; this was not inferred merely from SET distsql or from a distributed plan label. Source inspection then located the different LocalExpr and serialized-expression paths. Finally, a native regression through the real DeserializeExpr reproduced the sign loss independently of the SQL predicate.

Numeric rewrites such as adding zero or substituting a positive-zero literal were not accepted as equivalent oracles: DECIMAL-to-STRING observes a representation difference that numeric equality alone does not preserve.

These comparisons establish consistency with the existing local semantics, not that exposing DECIMAL negative zero is the desired long-term SQL contract. If all DECIMAL zeros were instead canonicalized before conversion to STRING, the original query would consistently return one row, not zero.

Fix

The production change is eleven lines in DDecimal.Format, restricted to finite negative zero under FmtParsableNumerics.

Instead of serializing a zero literal whose sign will be discarded, emit a fully parenthesized typed multiplication, for example:

(0.00:::DECIMAL * (-1):::DECIMAL)

A sign-cleared copy supplies the positive-zero spelling while retaining the exponent/scale. Constant evaluation after deserialization reconstructs the negative-zero datum using existing arithmetic. The original datum is not mutated. The outer parentheses and operand type annotations preserve expression grouping and DECIMAL typing.

No arithmetic implementation, ordinary SQL literal parsing, FLOAT handling, or ordinary scalar formatting is changed. No new SQL syntax or wire-protocol field is introduced; mixed-version execution has not been independently tested here.

Regression coverage and completed local validation

• TestDeserializeDecimalSignedZero fails on the baseline and passes on the candidate through actual expression deserialization. It uses CmpTotal for scalar representation fidelity, covers negative zero with scale and tested exponents -2000/+2000, positive zero, nonzero and nonfinite controls, nested ARRAY/TUPLE expressions, and non-mutation of the input. It also checks that ordinary scalar formatting remains unchanged.
• The five-node distsql_builtin/regression_148279 test relocates the data to node 2 and checks the original expression, explicit STRING conversion, and remote computation over identical typed operands. The unchanged regression fails on baseline production and passes on the candidate.
• The selected decimal, distsql_expr, and zero files passed their five selected local/fakedist configurations; distsql_builtin passed the selected 5node and 5node-disk configurations.
• Complete tree, parser, eval, normalize, execexpr, physicalplan, and //pkg/sql:sql_test targets passed. The last is the SQL package's 16-shard test target, not the entire repository or every package under pkg/sql/....
• Three candidate three-node replays corrected the original result while preserving the equivalent controls. A separate held-out matrix covered six signed-zero execution scenarios over five non-NULL strings plus a NULL control, with positive-zero, nonzero, Infinity, FLOAT-negative-zero, and literal-parsing controls.

These are completed local checks for the unchanged candidate, not a claim that upstream CI has already passed.

What this fixes, and what remains unresolved

Fixed: loss of the negative sign of finite DECIMAL zero in the tested expression round trips, and the resulting plan-dependent STRING-conversion/filter results. The replacement representation preserves the existing scale as well; scale loss was not the baseline defect. The new serialization also works for the tested nested expression containers.

Not fixed or decided:

• The PostgreSQL-facing zero behavior noted in the discussion. This patch does not make every final arithmetic DECIMAL zero display as positive zero, nor redefine DECIMAL-to-STRING conversion.
• The proposed normalization at execution/output/storage boundaries. Text PGwire still uses the ordinary decimal string representation, while binary numeric encoding selects a positive sign for zero. VALUE/composite storage still preserves negative zero. These are existing behaviors, not newly introduced boundary regressions.
• The alternative creator-wide DDecimal audit. Arithmetic and built-ins can still create signed zero; the two proposed normalization strategies have not been treated as an agreed design.
• Exhaustive coverage of PGwire text/binary protocols, DistSQLReceiver boundaries, stored-value round trips, spill, or legacy stored negative zeros. Passing expression-formatting tests does not substitute for those checks.

Additional shared-formatter effect requiring review: this change is not exclusively internal. DArray.Format switches exported array elements to parsable formatting, so DECIMAL arrays containing negative zero can acquire the multiplication spelling in EXPORT CSV and CSV changefeed output. Scalar EXPORT spelling stays unchanged. The ARRAY import path uses a full SQL expression parser/type-checker/evaluator, so the new spelling is not an identified import-syntax failure; however, actual EXPORT/IMPORT and changefeed compatibility tests for this case are not included. In particular, scalar text import still normalizes zero whereas the new array expression can reconstruct its sign.

I am seeking review of this bounded consistency fix and guidance on whether the broader semantics should be addressed here or separately. The existing commit includes an issue-closing trailer; please decide how remaining work should be tracked before merging rather than interpreting that trailer as a claim that all discussion points are resolved.

See also: #148279
Epic: none

Release note (bug fix): Fixed incorrect distributed query results when an arithmetic-produced DECIMAL negative zero was converted to a string. Serialized expressions now preserve its sign and scale, matching local execution.

[blathers-crl]

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

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

[Alignyx]

@yuzefovich @mgartner Could you advise whether you would like the remaining DECIMAL-negative-zero work to continue in this PR, or prefer this as a bounded local/remote consistency fix with a separately tracked semantic change?

The current patch deliberately preserves the existing intermediate result. It does not complete either the execution-boundary normalization direction or the creator-wide normalization direction. I do not yet have a fully audited and validated implementation of either; the following are proposed next steps, not completed fixes:

1. Agree on the observable contract first. Should negative zero be retained internally but normalized at selected boundaries, or should every SQL DECIMAL zero be canonical? This needs an explicit decision for ::STRING/implicit concatenation as well as final DECIMAL output. Normalizing only PGwire datums cannot change a STRING already produced inside a filter. With creator-wide canonicalization, the original query's consistent result would become one row rather than the zero rows asserted by this compatibility-preserving patch.
2. If the long-term choice is canonical SQL DECIMAL zero, a candidate implementation strategy is a common finite-zero normalization operation that clears only Negative, preserving coefficient/exponent and leaving FLOAT, nonzero and nonfinite values unchanged. Its callers would need an audit across scalar and vectorized arithmetic, built-ins, casts, aggregates, constant evaluation, and construction/decoding paths. It must not mutate shared datums. Existing stored negative zeros and mixed-version execution need an explicit compatibility policy; merely adding a helper to a few constructors is not sufficient.
3. If retaining intermediate negative zero is preferred, first enumerate the boundaries and conversion rules, then implement non-mutating normalization only at the agreed boundaries, including nested containers where applicable. This is not just a formatting-only fix: STRING-producing conversions and storage/legacy-value handling need their own decisions.
4. For the narrow serialization approach, a distinct internal serialization mode/path is worth evaluating to avoid changing DECIMAL-array EXPORT/CSV changefeed spelling. This needs a call-site audit rather than simply substituting an existing flag (FmtCheckEquivalence and FmtSerializable currently have identical flag sets), plus nested-expression and actual EXPORT/IMPORT coverage.

Whichever direction you prefer, the focused follow-up matrix would compare the reduced SELECT -1.0 // 2 FROM t across local/remote and row/vector execution, direct DECIMAL output versus STRING conversion, text/binary PGwire, write/read and spill boundaries, and DECIMAL-array EXPORT/IMPORT/changefeed behavior, with scale and FLOAT-negative-zero controls. These would supplement, not be inferred from, the existing expression round-trip tests.

Is the narrow fix an acceptable first step, and which normalization contract should guide any further work? Also, should the remaining work stay under #148279 or move to a follow-up issue? The current commit has an issue-closing trailer, so that tracking choice should be settled before merge.