Skip to content

add UUID functions#24807

Open
daviszhen wants to merge 5 commits into
matrixorigin:mainfrom
daviszhen:0603-add-uuid
Open

add UUID functions#24807
daviszhen wants to merge 5 commits into
matrixorigin:mainfrom
daviszhen:0603-add-uuid

Conversation

@daviszhen
Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #24486

What this PR does / why we need it:

增加uuid函数: is_uuid, uuid_short, uuid_to_bin, bin_to_uuid

@qodo-code-review
Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

Copy link
Copy Markdown
Contributor

@XuPeng-SH XuPeng-SH left a comment

Choose a reason for hiding this comment

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

I reviewed this from correctness / behavior-compatibility / coverage angles, and there is one blocking issue:

  • uuid_short() is currently implemented as xxhash.Sum64(uuid.NewV7()). That is not MySQL-compatible. MySQL documents UUID_SHORT() as a 64-bit value built from (server_id << 56) + (startup_time << 24) + incrementing_counter, specifically to preserve uniqueness under documented conditions and to produce increasing values. Hashing a 128-bit UUID down to 64 bits removes both of those properties: collisions become possible by construction, and call order is no longer monotonic.

The official MySQL docs for UUID_SHORT() are explicit about this contract, and the current implementation does not match it. I think this needs either:

  1. a real UUID_SHORT() implementation with server/startup/counter semantics, or
  2. a different function name if the intention is to expose a hashed UUID-derived helper rather than MySQL-compatible behavior.

The current tests also only check uuid_short() is not null / > 0, so they would not catch the semantic mismatch above.

@daviszhen
Copy link
Copy Markdown
Contributor Author

I reviewed this from correctness / behavior-compatibility / coverage angles, and there is one blocking issue:

  • uuid_short() is currently implemented as xxhash.Sum64(uuid.NewV7()). That is not MySQL-compatible. MySQL documents UUID_SHORT() as a 64-bit value built from (server_id << 56) + (startup_time << 24) + incrementing_counter, specifically to preserve uniqueness under documented conditions and to produce increasing values. Hashing a 128-bit UUID down to 64 bits removes both of those properties: collisions become possible by construction, and call order is no longer monotonic.

The official MySQL docs for UUID_SHORT() are explicit about this contract, and the current implementation does not match it. I think this needs either:

  1. a real UUID_SHORT() implementation with server/startup/counter semantics, or
  2. a different function name if the intention is to expose a hashed UUID-derived helper rather than MySQL-compatible behavior.

The current tests also only check uuid_short() is not null / > 0, so they would not catch the semantic mismatch above.

删除了uuid_short. 一是不在issue需求. 二是 mysql的实现方案 在 mo 多cn中不好做.

Copy link
Copy Markdown
Contributor

@XuPeng-SH XuPeng-SH left a comment

Choose a reason for hiding this comment

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

I found one blocking compatibility issue.

UUID_TO_BIN() / BIN_TO_UUID() currently force any non-integer swap_flag through an INT64 cast in list_builtIn.go before execution. The implementation then only checks swapFlag != 0 in func_builtin.go. That is not MySQL-compatible: upstream MySQL evaluates the second argument with boolean coercion (val_bool()), so nonzero fractional values are true. In this patch, values such as 0.4 are narrowed to integer zero first, which means uuid_to_bin(u, 0.4) / bin_to_uuid(b, 0.4) will not swap even though MySQL does.

The current unit tests and BVT only cover integer / NULL swap flags, so this regression is not caught by coverage.

Copy link
Copy Markdown
Contributor

@aunjgr aunjgr left a comment

Choose a reason for hiding this comment

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

MySQL-compatible UUID utility functions: IS_UUID(), UUID_TO_BIN(), BIN_TO_UUID(). Clean implementation — swap-flag handling via uuidToBin matches MySQL 8.0 semantics (swaps time-low/time-mid/time-high-and-version groups). BVT covers documented UUID string forms, NULL handling, invalid UUID rejection, both swap modes. LGTM.

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

Labels

kind/enhancement kind/feature size/L Denotes a PR that changes [500,999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants