Skip to content

repr: canonicalize float encodings in row packing - #37482

Open
ggevay wants to merge 3 commits into
MaterializeInc:mainfrom
ggevay:gabor/sql-452-literalconstraints-optimization-causes-wrong-results
Open

repr: canonicalize float encodings in row packing#37482
ggevay wants to merge 3 commits into
MaterializeInc:mainfrom
ggevay:gabor/sql-452-literalconstraints-optimization-causes-wrong-results

Conversation

@ggevay

@ggevay ggevay commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Stacked on top of #37489. The first (tiny) commit is from there.

Fixes SQL-452.

SQL float equality is semantic: -0.0 = 0.0 and NaN = NaN are true, and Datum equality agrees (via OrderedFloat). But packed rows are compared as raw bytes (Row equality, arrangement keys, index lookups), and -0.0 vs +0.0 and different NaN payloads have distinct bit patterns. Equal values therefore landed under distinct arrangement keys, giving wrong results for index lookups (LiteralConstraints), joins, GROUP BY, DISTINCT, DISTINCT ON, and UNION on float columns.

Fix this at the single choke point that writes float bytes: row packing now rewrites each float to the canonical representative of its equality class, -0.0 to +0.0 and every NaN to the quiet positive NaN, mirroring the existing Numeric canonicalization. All ingress paths (persist columnar decode, ProtoRow decode, source decoders, pgwire) go through the packer, so pre-existing persisted data is normalized on read.

A visible consequence is that floats never render as "-0" anymore. Row packing canonicalizes stored values, and text formatting renders -0.0 as "0" as well, so text output does not depend on whether a value crossed a packing boundary (a view and a materialized view of the same query render identically). Negative zero is thereby unobservable in Materialize. This deviates from PostgreSQL, which prints "-0".

Also correct the warning comment on the "=" operator table, which claimed BinaryFunc::Eq is byte equality. It is Datum equality, and packing must canonicalize any type whose Datum equality is coarser than bit equality.

Sign-of-zero-sensitive functions

A few scalar functions could observe the sign of a float zero, which would make their results depend on whether the input crossed a packing boundary (e.g. a view vs a materialized view of the same query). They now treat -0.0 exactly like +0.0:

  • cot(-0.0) returns +Infinity. This deviates from PostgreSQL, which returns -Infinity.
  • power(-0.0, 0.5) returns 0 and power(0, -0.0) returns 1 instead of erroring. This matches PostgreSQL, which uses strict comparisons rather than the sign bit. NaN inputs now propagate to NaN results regardless of the NaN's sign bit, also matching PostgreSQL.
  • ln/log/log10 (float and numeric) report the "not defined for zero" error for -0.0 rather than the "not defined for negative numbers" error, matching PostgreSQL's zero-first check order.

SourceData encoding stability

The source_proto_serialization_stability golden file is regenerated: old encodings containing non-canonical floats now decode to canonical rows, so re-encoding them produces different bytes. Consequence for existing shards: a record whose old and new encodings differ never consolidates away inside persist (an old-version addition and a new-version retraction of the same logical row both stay in the shard). Readers stay correct because they consolidate rows after decoding, where the two encodings are identical. The stability test's comment pointed at MINIMUM_CONSOLIDATED_VERSION as the gate for such changes, but that constant has been unused since codec-order consolidation was removed in f02f202. It is deleted in #37489, which this PR is stacked on (the first commit here is that PR's commit until it merges). Persist reviewers, please confirm this reasoning.

Compatibility notes

  • Kafka upsert sinks with a float in the key: a key containing -0.0 emitted before this change encodes different bytes than the same key emitted after it, so a post-upgrade update or deletion of such a row will not match the pre-upgrade key in the topic, leaving a phantom entry behind. Floats in upsert sink keys are a degenerate setup, but worth calling out.
  • Persist shards: a -0.0 row written before this change and its retraction written after it no longer byte-cancel during compaction. They still cancel at read time (every read decodes through the packer, which normalizes), so results are correct. The cost is a bounded amount of never-consolidated data in affected shards.
  • test/sqllogictest/cockroach/float.slt stays disabled: its correctness expectations now hold, but its display expectations assume CockroachDB's key/value separation (stored -0 preserved on output), which this design intentionally does not provide.

Testing

  • New platform checks (FloatCanonicalizationTable, FloatCanonicalizationPgCdc, FloatCanonicalizationUpsert) exercise the cross-version story: rows written by an older version with raw float bits must cancel against retractions written by a newer version, and must land in the same DISTINCT/GROUP BY/index groups. This covers tables, Postgres sources, and Kafka upsert sources (including a -0.0 vs +0.0 upsert key).

  • New unit test test_float_packing_canonicalizes in mz-repr covering signed zeros, negative/payload/signaling NaNs, for both float4 and float8.

  • New regression tests in test/sqllogictest/transform/literal_constraints.slt: indexed lookups across -0.0/+0.0/NaN in both directions, a no-index control, and an EXPLAIN showing = '0' OR = '-0' dedupes to a single lookup key.

  • New tests in test/sqllogictest/float.slt for the arrangement family: join, GROUP BY, DISTINCT, DISTINCT ON, UNION over mixed zeros and NaN payloads, the -0.0 rendering, and a view vs materialized view of the same query rendering identically. The existing '-0'::float::text expectations change from -0 to 0.

  • Nightly run: https://buildkite.com/materialize/nightly/builds/17039

@ggevay ggevay added the A-CLUSTER Topics related to the CLUSTER layer label Jul 7, 2026
@ggevay
ggevay force-pushed the gabor/sql-452-literalconstraints-optimization-causes-wrong-results branch from 0f95e35 to a76d983 Compare July 7, 2026 10:59
@antiguru
antiguru self-requested a review July 7, 2026 11:08
@ggevay

ggevay commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

(I'm still investigating some test fails before bringing it out of draft.) Edit: fixed

MINIMUM_CONSOLIDATED_VERSION gated assumptions about codec-order
consolidation: parts written before it could not be assumed to be
consolidated or sorted according to the current definition. Its last
consumers were removed in f02f202 ("Remove codec-order
consolidation") when consolidation moved entirely to structured (Arrow)
ordering with per-run RunOrder metadata, leaving the constant
unreferenced.

Also update the comment on the SourceData serialization stability test,
which still instructed encoding changes to bump the constant.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SQL float equality is semantic: -0.0 = 0.0 and NaN = NaN are true, and
Datum equality agrees (via OrderedFloat). But packed rows are compared
as raw bytes (Row equality, arrangement keys, index lookups), and -0.0
vs +0.0 and different NaN payloads have distinct bit patterns. Equal
values therefore landed under distinct arrangement keys, giving wrong
results for index lookups (LiteralConstraints), joins, GROUP BY,
DISTINCT, DISTINCT ON, and UNION on float columns.

Fix this at the single choke point that writes float bytes: row packing
now rewrites each float to the canonical representative of its equality
class, -0.0 to +0.0 and every NaN to the quiet positive NaN, mirroring
the existing Numeric canonicalization. All ingress paths (persist
columnar decode, ProtoRow decode, source decoders, pgwire) go through
the packer, so pre-existing persisted data is normalized on read.

Text formatting renders -0.0 as "0" as well, so text output does not
depend on whether a value crossed a packing boundary (a view and a
materialized view of the same query render identically). Negative zero
is thereby unobservable in Materialize. This deviates from PostgreSQL,
which prints "-0".

Also correct the warning comment on the "=" operator table, which
claimed BinaryFunc::Eq is byte equality. It is Datum equality, and
packing must canonicalize any type whose Datum equality is coarser
than bit equality.

Fixes SQL-452.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ggevay
ggevay force-pushed the gabor/sql-452-literalconstraints-optimization-causes-wrong-results branch 2 times, most recently from 9833ce1 to 5c999cf Compare July 7, 2026 13:40
Make the remaining sign-of-zero-sensitive scalar functions treat -0.0
as +0.0, so that results do not depend on whether a value crossed a
packing boundary (e.g. a view vs a materialized view of the same
query): cot(-0.0) now returns +Infinity (PostgreSQL returns -Infinity
here), power no longer errors for a -0.0 base with a fractional
exponent or a -0.0 exponent with a zero base, and ln/log10 (float and
numeric) report the zero error rather than the negative error for
-0.0. The ln/log10/power changes also match PostgreSQL, which uses
zero-first checks and strict comparisons, and they let NaN inputs
propagate to NaN results regardless of the NaN's sign bit.

Regenerate the SourceData serialization stability snapshot: old
encodings containing non-canonical floats now decode to canonical rows,
so re-encoding them produces different bytes. Update the stale comment
that pointed at MINIMUM_CONSOLIDATED_VERSION, which has not been
consulted since codec-order consolidation was removed.

Add platform checks covering the cross-version story for tables,
Postgres sources, and Kafka upsert sources: rows written by an old
version with raw float bits must cancel against retractions written by
a new version, and must land in the same DISTINCT/GROUP BY/index
groups.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ggevay
ggevay force-pushed the gabor/sql-452-literalconstraints-optimization-causes-wrong-results branch from 5c999cf to 08a90d5 Compare July 7, 2026 14:59
@ggevay
ggevay marked this pull request as ready for review July 7, 2026 15:17
@ggevay
ggevay requested review from a team as code owners July 7, 2026 15:17
@ggevay

ggevay commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

This is ready for review!

(Nightly's failing Orchestratord tests are unrelated.)

@antiguru
antiguru requested review from DAlperin and martykulma July 7, 2026 15:27

@martykulma martykulma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like there might be some impact to sinks:

  • kafka upsert with float in the key
  • kafka sink w/debezium envelope

kafka compaction will leave stranded messages (floats in the key are already problematic, so I'm not sure how much worse this is just because floats are problematic). Anything that cares about definite outputs is going to see some impact.

Upsert iceberg sink isn't affected, it disallows floats in keys. For non-key cases (e.g. float in the value), there's customer visibility, but presumably no impact.

How are we communicating to users?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-CLUSTER Topics related to the CLUSTER layer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants