Skip to content

[CALCITE-7716] BETWEEN/range predicates on UUID literals give wrong results because RexSimplify orders bounds using java.util.UUID#compareTo (signed comparison) - #5199

Open
GoncaloCoutoDosSantos wants to merge 1 commit into
apache:mainfrom
GoncaloCoutoDosSantos:CAL-7716

Conversation

@GoncaloCoutoDosSantos

Copy link
Copy Markdown

Jira Link

CALCITE-7716

Changes Proposed

SQL orders UUID values as unsigned 128-bit integers, but java.util.UUID#compareTo
compares the two 64-bit halves as signed longs. Since a UUID was used directly as
the Comparable value of a UUID RexLiteral, every range predicate inherited that
signed ordering.

Any bound whose most significant bit is set therefore sorts below one whose is not, so
RexSimplify sees an inverted (empty) range and folds the predicate away:

SELECT UUID '8ba7b810-9dad-11d1-80b4-00c04fd430c8'                                                                                                                                                             
  BETWEEN UUID '00000000-0000-0000-0000-000000000000'                                                                                                                                                          
      AND UUID 'ffffffff-ffff-ffff-ffff-ffffffffffff';                                                                                                                                                         

Returns FALSE before this change, TRUE after. ffffffff-… has all bits set, so its
high half is -1 as a signed long and it compares as less than 00000000-…, making the
range empty. The same applies to <, <=, > and >=; IN, NOT IN and
IS [NOT] DISTINCT FROM were unaffected because they compare by equality only.

The fix

Introduce org.apache.calcite.util.UuidValue, a small wrapper around java.util.UUID
that implements Comparable using Long.compareUnsigned on each half, and use it as
both the RexLiteral value and the runtime representation of SQL UUID:

  • RexLiteralvalueMatchesType and the UUID assertion now expect UuidValue.
    getValueAs(UUID.class) is still supported and unwraps, so existing callers keep working.
  • RexBuildermakeUuidLiteral wraps into a UuidValue; a UuidValue overload is
    added and makeLiteral accepts either representation.
  • JavaTypeFactoryImpl — SQL UUID now maps to UuidValue.class rather than UUID.class.
  • SqlFunctionsuuidToString, uuidToBinary and binaryToUuid operate on
    UuidValue, plus new lt/le/gt/ge overloads so runtime comparison uses the same
    unsigned ordering as planning-time simplification.
  • BuiltInMethod, RelJson, VariantNonNull — updated to the new type.

UuidValue.fromString (wired to BuiltInMethod.UUID_FROM_STRING, used by the runtime
CAST(VARCHAR AS UUID) path) delegates to the existing lenient SqlFunctions.stringToUuid
rather than java.util.UUID.fromString, so all the spellings Calcite already accepts —
optional hyphen group separators, surrounding braces — keep working, matching PostgreSQL.

Escape hatch

The new system property calcite.uuid.unsigned.comparison (CalciteSystemProperty.UUID_UNSIGNED_COMPARISON,
default true) reverts to the old UUID#compareTo ordering for anyone depending on the
previous behaviour.

Tests

SqlOperatorTest.testUuidBetween covers BETWEEN/NOT BETWEEN, < and > across the
minimum, a mid-range and the maximum UUID, and asserts that IN, NOT IN and
IS [NOT] DISTINCT FROM are independent of the ordering. Expected values are derived from
UUID_UNSIGNED_COMPARISON so the test is correct under either setting.

./gradlew build passes, including the full test suite.

…esults because RexSimplify orders bounds using java.util.UUID#compareTo (signed comparison)

@mihaibudiu mihaibudiu 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.

this is a breaking change, both in APIs and semantics, at the very least it has to be documented in the release notes

@sonarqubecloud

Copy link
Copy Markdown

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants