Skip to content

Expose AdvancedColumnFamilyOptions::memtable_batch_lookup_optimization in the C API#14776

Open
zaidoon1 wants to merge 1 commit into
facebook:mainfrom
zaidoon1:expose_memtable_batch_lookup_optimization_c_api
Open

Expose AdvancedColumnFamilyOptions::memtable_batch_lookup_optimization in the C API#14776
zaidoon1 wants to merge 1 commit into
facebook:mainfrom
zaidoon1:expose_memtable_batch_lookup_optimization_c_api

Conversation

@zaidoon1
Copy link
Copy Markdown
Contributor

Summary

AdvancedColumnFamilyOptions::memtable_batch_lookup_optimization (advanced_options.h) gates the skip-list memtable's batch-lookup optimization for MultiGet. When enabled, the search path is cached between consecutive keys, reducing per-key cost from O(log N) to O(log d) where d is the distance between consecutive keys.

The C++ field exists; the C API setter does not. This PR adds the missing pair, mirroring the existing rocksdb_options_{set,get}_memtable_huge_page_size shape exactly — the closest sibling on both axes:

  • C API: adjacent memtable knob, same rocksdb_options_t* receiver.
  • C++: same AdvancedColumnFamilyOptions parent struct, same immutability semantics.

Motivation

Without this setter, C API consumers and downstream bindings cannot opt into the batch-lookup optimization. Non-skip-list memtable implementations fall back to per-key lookups, so the flag is a no-op for them.

The field is immutable on the C++ side, so calling the setter on options that are already in use by an open DB has no effect on that DB — same constraint as the underlying C++ field. This matches the behavior of every other immutable-options setter in the C API.

No change to the C++ API.

…n in the C API

The skip-list memtable gained an opt-in batch-lookup optimization that
caches the search path between consecutive MultiGet keys, reducing
per-key cost from O(log N) to O(log d) where d is the distance between
consecutive keys. The optimization is gated by a new immutable
AdvancedColumnFamilyOptions field (default: false) which had no C API
setter, so the optimization was unreachable from C/Rust.

Add a setter/getter pair mirroring the existing
rocksdb_options_{set,get}_memtable_huge_page_size — the closest sibling
on both the C API side (adjacent memtable knob) and the C++ side (same
AdvancedColumnFamilyOptions parent struct).

The field is immutable on the C++ side, so the setter only takes effect
on options used to open a column family.

No change to the C++ API.
@meta-cla meta-cla Bot added the CLA Signed label May 23, 2026
@github-actions
Copy link
Copy Markdown

✅ clang-tidy: No findings on changed lines

Completed in 54.7s.

@github-actions
Copy link
Copy Markdown

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit c4d237d


Codex review failed before producing findings.

WARNING: proceeding, even though we could not update PATH: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

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

Commands:

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

@github-actions
Copy link
Copy Markdown

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit c4d237d


Summary

Clean, mechanical PR that correctly adds C API getter/setter for an existing C++ bool field following established patterns. One missing test case for copy independence.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. Missing copy-independence test — db/c_test.c
  • Issue: The "Copies should be independent" section of c_test.c (starting around line 3014) tests that mutating a copied rocksdb_options_t does not affect the original. The PR adds copy verification (checking the copied value is 1) but does not add the independence test (set copy to 0, verify copy is 0, verify original is still 1). Every other bool option in this section has this test — e.g., inplace_update_support (line 3340-3342), allow_ingest_behind (line 3015-3017), create_if_missing (line 3023-3025).
  • Root cause: Oversight — the PR followed the "verify copied value" pattern but missed the "mutate copy independently" pattern that follows later in the same test.
  • Suggested fix: Add to the "Copies should be independent" section:
    rocksdb_options_set_memtable_batch_lookup_optimization(copy, 0);
    CheckCondition(0 == rocksdb_options_get_memtable_batch_lookup_optimization(copy));
    CheckCondition(1 == rocksdb_options_get_memtable_batch_lookup_optimization(o));

🟢 LOW / NIT

L1. Verbose release note — unreleased_history/public_api_changes/c_api_memtable_batch_lookup_optimization.md
  • Issue: The release note is ~147 words with detailed technical explanation of the optimization's algorithm. Existing C API addition release notes in HISTORY.md are typically a single sentence (e.g., "Add rocksdb_options_add_compact_on_deletion_collector_factory_del_ratio").
  • Suggested fix: Shorten to:
    Added `rocksdb_options_set_memtable_batch_lookup_optimization()` and `rocksdb_options_get_memtable_batch_lookup_optimization()` to the C API, exposing the existing `AdvancedColumnFamilyOptions::memtable_batch_lookup_optimization` field.
    
L2. Java binding gap — observation only
  • Issue: The memtable_batch_lookup_optimization field is now exposed in the C API but not in the Java API. This is not a bug introduced by this PR — the Java binding was already missing before this change. Noted for completeness.

Cross-Component Analysis

Context Relevant? Assessment
WritePreparedTxnDB No Getter/setter only — no runtime behavior change
ReadOnly DB No Options set before open
User-defined timestamps No Orthogonal
All other contexts No Pure options plumbing

The PR adds no new code paths, no new logic, no new data structures. It is a thin passthrough from C to an existing C++ field. All correctness, concurrency, and behavioral guarantees are unchanged from the existing C++ implementation.

Positive Observations

  • Follows the established unsigned char convention for C API bool fields exactly
  • Placement in c.h, c.cc, and c_test.c is consistent with neighboring options
  • Tests verify both default value and set/get round-trip
  • Copy verification test is included
  • Release note file correctly placed in unreleased_history/public_api_changes/

ℹ️ 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

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant