Skip to content

Implement AsRef<[u8]>, Borrow<[u8]>, and Deref<Target=[u8]> for Key#494

Merged
ti-chi-bot[bot] merged 1 commit into
tikv:masterfrom
ADSteele916:key-u8-ref-impls
Jun 15, 2026
Merged

Implement AsRef<[u8]>, Borrow<[u8]>, and Deref<Target=[u8]> for Key#494
ti-chi-bot[bot] merged 1 commit into
tikv:masterfrom
ADSteele916:key-u8-ref-impls

Conversation

@ADSteele916

@ADSteele916 ADSteele916 commented Jun 22, 2025

Copy link
Copy Markdown
Contributor

These trait implementations make it easier to work with Key instances outputted by operations like scan and scan_keys without making unnecessary clones.

Summary by CodeRabbit

  • Refactor
    • Expanded the key type’s interoperability by supporting more standard Rust borrowing/access patterns (e.g., treating it directly as a byte slice).
    • Improved clarity in region lookup behavior by making key range selection more explicit, without changing results.
  • Tests
    • Updated integration test lookups to use the underlying byte representations directly for result verification.

@ti-chi-bot

ti-chi-bot Bot commented Jun 22, 2025

Copy link
Copy Markdown

Welcome @ADSteele916!

It looks like this is your first PR to tikv/client-rust 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!



Thank you, and welcome to tikv/client-rust. 😃

@ti-chi-bot ti-chi-bot Bot added dco-signoff: no Indicates the PR's author has not signed dco. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 22, 2025
@ti-chi-bot ti-chi-bot Bot added dco-signoff: yes Indicates the PR's author has signed the dco. and removed dco-signoff: no Indicates the PR's author has not signed dco. labels Jun 22, 2025
@ADSteele916

Copy link
Copy Markdown
Contributor Author

@pingyu could you or another maintainer please take a look at this PR?

@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0ad23f2c-0db2-4c14-85d7-b399a541c00e

📥 Commits

Reviewing files that changed from the base of the PR and between 739ff0c and f3ad9f7.

📒 Files selected for processing (3)
  • src/kv/key.rs
  • src/region_cache.rs
  • tests/integration_tests.rs
✅ Files skipped from review due to trivial changes (1)
  • src/region_cache.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/kv/key.rs

📝 Walkthrough

Walkthrough

Key gains three standard byte-access trait implementations (AsRef<[u8]>, Borrow<[u8]>, Deref<Target=[u8]>), and imports are updated accordingly. Two BTreeMap::range calls in RegionCache are then updated to carry explicit Key type parameters, using those new impls for disambiguation. Test assertions are updated to validate the new as_bytes() method.

Changes

Key byte-access traits and RegionCache explicit type params

Layer / File(s) Summary
Key: AsRef<[u8]>, Borrow<[u8]>, Deref impls
src/kv/key.rs
Adds std::borrow::Borrow and Deref to imports; implements AsRef<[u8]>, Borrow<[u8]>, and Deref<Target=[u8]> on Key, each returning a reference to the inner byte buffer. Adds a blank line before the existing impl AsRef<Key> for Key block.
RegionCache: explicit BTreeMap::range type params
src/region_cache.rs
get_region_by_key and add_region update BTreeMap::range calls to use range::<Key, _> explicit type parameters, enabled by the new Key byte-access trait impls. No logic changes.
Test validation of Key traits
tests/integration_tests.rs
Test assertions in txn_get_for_update are updated to access result values using key.as_bytes() instead of conversion expressions, validating the new AsRef<[u8]> trait on Key.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 Hop hop, the Key now borrows with grace,
AsRef and Deref join the byte-slice race,
The BTreeMap asks, "What type do you bring?"
Key, _ replies clearly — oh what a spring!
Tests hop along to verify the thing! 🌸

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and precisely describes the main change: implementing three specific trait implementations for the Key type.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@pingyu

pingyu commented Jun 14, 2026

Copy link
Copy Markdown
Collaborator

@pingyu could you or another maintainer please take a look at this PR?

@ADSteele916 Thanks for your contribution ! I'm sorry for the late.

Please fix the CI errors.

Signed-off-by: Alex Steele <45648397+ADSteele916@users.noreply.github.com>
@ADSteele916

Copy link
Copy Markdown
Contributor Author

@pingyu could you or another maintainer please take a look at this PR?

@ADSteele916 Thanks for your contribution ! I'm sorry for the late.

Please fix the CI errors.

No worries at all about the delay! Should be fixed now. I probably tested my changes locally with --all-targets but not --all-features. Let me know if there's anything else that needs changes.

@pingyu pingyu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM~

@ti-chi-bot ti-chi-bot Bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jun 15, 2026
@pingyu

pingyu commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

/cc @iosmanthus

@ti-chi-bot ti-chi-bot Bot requested a review from iosmanthus June 15, 2026 10:31
@ti-chi-bot ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 15, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jun 15, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iosmanthus, pingyu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot

ti-chi-bot Bot commented Jun 15, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-06-15 10:31:00.045566368 +0000 UTC m=+1387961.115883757: ☑️ agreed by pingyu.
  • 2026-06-15 10:31:40.885213096 +0000 UTC m=+1388001.955530485: ☑️ agreed by iosmanthus.

@ti-chi-bot ti-chi-bot Bot merged commit c82b10b into tikv:master Jun 15, 2026
8 checks passed
@pingyu

pingyu commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

@ADSteele916 Thank you !

@ADSteele916 ADSteele916 deleted the key-u8-ref-impls branch June 15, 2026 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved dco-signoff: yes Indicates the PR's author has signed the dco. lgtm size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants