Skip to content

sql: prevent unsafe comparison constant unification - #172989

Draft
shivamshaw23 wants to merge 1 commit into
cockroachdb:masterfrom
shivamshaw23:codex/fix-172978
Draft

sql: prevent unsafe comparison constant unification#172989
shivamshaw23 wants to merge 1 commit into
cockroachdb:masterfrom
shivamshaw23:codex/fix-172978

Conversation

@shivamshaw23

Copy link
Copy Markdown
Contributor

Summary

Fixes #172978.

UnifyComparison could replace a mixed-type comparison with one against a converted constant even when the comparison's implicit cast collapses distinct column values. This caused INT8 values above 2^53 to be missed when compared to a FLOAT8 constant. It also performed stable casts without consulting FoldingControl, allowing session-time-zone-dependent TIMESTAMP to TIMESTAMPTZ conversions to be baked into prepared plans.

This change only permits the rewrite when the variable-to-original conversion is injective, and consults cast volatility through FoldingControl before evaluating either cast. It adds logic tests for scalar and ANY comparisons and for prepared statements across a time-zone change.

Validation

  • git diff --check
  • Full Bazel logic-test generation was not run locally: WSL fails to start because its Docker disk path is missing.
  • Direct go test ./pkg/sql/opt/norm is not supported in this checkout without Bazel-generated protobuf packages.

@blathers-crl

blathers-crl Bot commented Jul 31, 2026

Copy link
Copy Markdown

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

Before a member of our team reviews your PR, I have some potential action items for you:

  • Please ensure your git commit message contains a release note.
  • When CI has completed, please ensure no errors have appeared.

I have added a few people who may be able to assist in reviewing:

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

@blathers-crl blathers-crl Bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels Jul 31, 2026
@blathers-crl
blathers-crl Bot requested a review from yuzefovich July 31, 2026 10:00
@shivamshaw23

Copy link
Copy Markdown
Contributor Author

Review focus: please validate the intentionally conservative isInjectiveConversion allowlist and whether any additional type pairs are provably injective. The key invariant is that rewriting variable <op> constant must preserve the comparison overload's implicit cast for every possible variable value, not merely round-trip the selected constant.

The regression covers both paths from #172978 (scalar/ANY INT8-vs-FLOAT8 and prepared TIMESTAMP/TIMESTAMPTZ after a time-zone change). Local full test execution is blocked by the machine's unavailable WSL/Docker disk, as described in the PR body.

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

Labels

O-community Originated from the community X-blathers-triaged blathers was able to find an owner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sql/opt: constant type-unification in comparisons yields incorrect results (large INT vs FLOAT; TZ-dependent casts in prepared plans)

1 participant