Add WriteBatch SliceParts APIs with user-defined timestampadd new apis

[sherriiiliu]

Current WriteBatch SliceParts APIs does not accept user-defined timestamp, and the following description of current APIs is not true because multiple WriteBatch APIs(e.g. Put, delete, etc.) accepting SliceParts args will return Status::InvalidArgument( "Cannot call this method on column family enabling timestamp") error if I set key as user key + timestamp as described in comment. We need to use SliceParts as argument to avoid temporary buffer usage for concatenating our values.

  // The following Put(..., const Slice& key, ...) API can also be used when
  // user-defined timestamp is enabled as long as `key` points to a contiguous
  // buffer with timestamp appended after user key. The caller is responsible
  // for setting up the memory buffer pointed to by `key`.

This patch adds new APIs for SliceParts that accept user-defined timestamp as this PR

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 244.2s.

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit a657e9a

---

Summary

The PR adds WriteBatch SliceParts APIs with user-defined timestamp support, mirroring the existing Slice-based timestamp APIs from PR #8946. The approach is sound: it fills a real gap in the API surface. The implementation is functionally correct but has significant code duplication and critically insufficient test coverage.

High-severity findings (2):

• [write_batch_test.cc] Tests only cover error paths (wrong timestamp size); no test verifies correct data encoding for the happy path.
• [write_batch.cc / write_batch_base.cc] Massive code duplication -- the key-with-ts assembly pattern is copy-pasted ~15 times across 3 files with no helper function.

Full review (click to expand)

Findings

🔴 HIGH

H1. No happy-path data verification tests -- write_batch_test.cc

• Issue: The new tests only verify that invalid timestamp sizes return IsInvalidArgument(). There is no test that verifies the actual data written to the batch is correct. Specifically:
  • No test that writes via the new SliceParts+ts API and then reads back (via Iterate, PrintContents, or CheckTimestampsInWriteBatch) to verify the key includes the timestamp correctly encoded.
  • No test that writes via the modified no-ts SliceParts API (which now appends a dummy timestamp), calls UpdateTimestamps(), and verifies the timestamps are properly replaced.
  • No test for WriteBatchInternal::HasKeyWithTimestamp() returning true after using the new APIs.
  • No test verifying needs_in_place_update_ts_ is correctly set/unset for the two different API styles.
• Root cause: The tests were modeled on the sanity-check section of the existing test but that section only validates error paths. The UpdateTimestamps test (line 1340) shows how to do end-to-end verification and should be extended or a new test added.
• Suggested fix: Add a test that:
  1. Creates a WriteBatch with a TS-enabled CF
  2. Calls Put(cf, SliceParts_key, SliceParts_value) (no-ts variant) -- verifies HasKeyWithTimestamp() is true, TimestampsUpdateNeeded() is true
  3. Calls UpdateTimestamps() with a real timestamp
  4. Iterates the batch and verifies the key contains the correct timestamp
  5. Similarly for the explicit-ts variant: calls Put(cf, SliceParts_key, ts, SliceParts_value), verifies TimestampsUpdateNeeded() is false, iterates and verifies
  6. Repeat for Delete, SingleDelete, DeleteRange, Merge

H2. Severe code duplication -- write_batch.cc, write_batch_base.cc

• Issue: The identical pattern of "allocate vector, copy parts, append ts, call internal method" is repeated 15+ times across three files:

  • 5 times in the modified no-ts SliceParts methods (Put, Delete, SingleDelete, DeleteRange, Merge) in write_batch.cc
  • 5 times in the new explicit-ts SliceParts methods in write_batch.cc
  • 5 times in the WriteBatchBase default implementations in write_batch_base.cc

  DeleteRange is even worse -- it duplicates the pattern twice per method (for begin_key and end_key).

• Root cause: Each method was implemented independently following the existing pattern without extracting a common helper.

• Suggested fix: Create a helper function (e.g., AppendTimestampToSliceParts) that takes const SliceParts& key, const Slice& ts, std::vector<Slice>& out and returns SliceParts. This would reduce each call site to ~2 lines instead of ~6.

🟡 MEDIUM

M1. std::vector<Slice> heap allocation on every call -- write_batch.cc

• Issue: Every call to these APIs when ts_sz > 0 allocates a std::vector<Slice> on the heap. The existing Slice-based variants use std::array<Slice, 2> (stack-allocated). While key.num_parts is runtime, in practice it's typically 1-4 slices.
• Suggested fix: Use RocksDB's existing autovector<Slice, 8> (from util/autovector.h) instead of std::vector<Slice>.

M2. Missing API documentation for new methods -- write_batch.h, write_batch_base.h

• Issue: The new SliceParts+ts methods have no documentation comments. Public APIs must be documented.
• Suggested fix: Add documentation comments describing the purpose, requirements, and relationship to the Slice-based ts APIs.

M3. No integration tests -- DB-level verification missing

• Issue: All tests are at the WriteBatch unit level. No integration test uses DB::Write() with batches built using the new APIs.
• Suggested fix: Add an integration test that opens a DB with a TS-enabled CF, writes using the new APIs, and reads back.

M4. WriteBatchBase defaults do unnecessary work -- write_batch_base.cc

• Issue: The base class defaults assemble key+ts into SliceParts, then call the non-ts SliceParts method, which flattens to Slice. A simpler approach: flatten SliceParts to Slice, then call the Slice-based ts method directly.
• Suggested fix: Flatten and call Put(cf, key_slice, ts, value_slice) directly.

🟢 LOW / NIT

L1. Comment removed from Slice-based Put -- write_batch.h:77-80

• Issue: The diff removes comments about manual timestamp handling from the Put(cf, const Slice& key, const Slice& value) API. That API still supports the pattern; only the SliceParts comment should be removed.
• Suggested fix: Keep the Slice-based API comment.

L2. Test value "ts" is misleading -- write_batch_test.cc

• Issue: "ts" looks like a valid timestamp but is intentionally wrong-sized (2 bytes vs 8 expected). Minor readability concern.

Cross-Component Analysis

Context	Assumptions hold?	Action needed?
WriteBatch (primary)	YES -- correctly sets flags	safe
WriteBatchWithIndex	YES -- falls through to Slice methods	safe
DB::Write	YES -- TimestampsUpdateNeeded check prevents dummy timestamps from reaching DB	safe

Assumption stress-test:

• "Dummy timestamps replaced before write" -- VERIFIED. DB::Write rejects batches where needs_in_place_update_ts_ is true (db_impl_write.cc:884-896).
• "Explicit-ts APIs bypass dummy path" -- VERIFIED. They don't set needs_in_place_update_ts_.

Positive Observations

• Approach correctly mirrors the Slice-based timestamp APIs
• Behavioral change (SliceParts now succeed on TS-enabled CFs) fills a real API gap
• MaybeTrackTimestampSize correctly unified after if/else
• Flag handling consistent with existing patterns

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase