Pass file metadata to IngestExternalFile to improve ingestion latency (#14837)

[joshkang97]

Summary:

External file ingestion (DB::IngestExternalFile[s]) re-opens every SST file and scans it -- footer, properties, index, filter, and the first/last data blocks -- to recompute the boundary keys, sequence-number bounds, and table properties before committing. For cold, I/O-bound files this scan dominates ingest latency, even when the file is moved/linked rather than copied.

This change lets a caller skip that work when it already has the file's metadata. SstFileWriter::Finish() now returns a PreparedFileInfo with the file size, table properties, and prepared internal smallest/largest bounds, and a new IngestExternalFileArg::file_infos field carries one PreparedFileInfo per file into IngestExternalFiles(). When set, ingestion reuses that metadata instead of re-opening and scanning each file. The file is still copied/linked, and the checksum is still verified when verify_checksums_before_ingest is set (the fast path opens the file only for that). Point-key and range-deletion bounds are folded into the same prepared bound pair, and user-defined-timestamp files (including the "UDT in Memtables only" format, whose boundary keys carry no timestamp) are handled.

Internally, ingestion-job metadata acquisition was split into GetIngestedFileInfoFromFileInfo (reuse caller metadata) and GetIngestedFileInfoFromFile (open + scan). Prepared boundary updates use RocksDB comparators, and the timestamp-stripping path pads prepared bounds back to the internal timestamp shape before installing the file.

The ingestexternalfile db_bench benchmark now also exposes --ingest_external_file_fill_cache so ingestion-read block-cache effects can be controlled independently while comparing the file-info fast path against the normal open-and-scan path.

A future PR will support creating PreparedFileInfo from a existing DB-generated file, so this optimization is not just limited to SstFileWriter.

Benchmark Results

Existing db_bench ingestexternalfile benchmark, release build, using an XFS flash-backed filesystem. Files are linked (move_files) so the measurement isolates the ingest path rather than file-copy throughput. These numbers used direct reads and fill_cache=false to avoid warming the block cache while comparing the file-info fast path against the normal open-and-scan path.

Each run used 1M keys/SST, one ingest call, and db_bench reported a 62.9 MB estimated file size. The comparison varied the number of files in that ingest call across 10, 30, and 50 files.

Files/batch	Config	Prepare P50	Run P50	Run P50/file	Total ingestion P50	Total P50 drop
10	Baseline	10.909 ms	7.593 ms	0.759 ms/file	18.502 ms	--
10	file_info=true	1.127 ms	7.541 ms	0.754 ms/file	8.668 ms	53.2%
30	Baseline	29.049 ms	23.161 ms	0.772 ms/file	52.210 ms	--
30	file_info=true	2.734 ms	23.384 ms	0.779 ms/file	26.118 ms	50.0%
50	Baseline	49.036 ms	39.224 ms	0.784 ms/file	88.260 ms	--
50	file_info=true	4.133 ms	39.417 ms	0.788 ms/file	43.550 ms	50.7%

With fill_cache=false and direct reads, the metadata fast path cuts Prepare() by roughly 90-92%. Run() is roughly 0.75-0.79 ms per file across these runs. End-to-end db_bench micros/op is almost unchanged because this benchmark includes SST generation and compaction work; use rocksdb.ingest.external.file.prepare.micros and rocksdb.ingest.external.file.run.micros to isolate ingestion.

Benchmark args: --benchmarks=ingestexternalfile --use_existing_db=0 --num=1000000 --compression_type=none --statistics --use_direct_reads=true --ingest_external_file_batch_size=<10|30|50> --ingest_external_file_num_batches=1 --ingest_external_file_use_file_info=<false|true> --ingest_external_file_fill_cache=false.

Reviewed By: xingbowang

Differential Revision: D107721261

[meta-codesync]

@joshkang97 has exported this pull request. If you are a Meta employee, you can view the originating Diff in D107721261.

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 340.7s.

[github-actions]

Codex Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit 8d5e7ff

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not create PATH aliases: 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]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 8d5e7ff

---

Summary

Well-designed optimization that reduces external file ingestion latency ~34% by reusing SstFileWriter metadata. The code is largely correct, with the two main code paths (fast and scan) producing equivalent results for all common scenarios. A few gaps in test coverage and a minor hot-path performance concern should be addressed.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🟡 MEDIUM

M1. Extra per-key string copy in AddImpl() hot path -- table/sst_file_writer.cc:130

• Issue: largest_internal_key.assign(encoded_ikey.data(), encoded_ikey.size()) is called on every Put()/Merge()/Delete(). This doubles the number of per-key string assignments compared to the baseline (which already does largest_key.assign()). For workloads inserting millions of keys, this adds non-trivial copying overhead even when the caller never requests PreparedFileInfo.
• Root cause: The tracking fields are unconditionally maintained regardless of whether the caller will use the prepared metadata.
• Suggested fix: Defer the largest_internal_key capture to Finish(). Since keys are added in strictly increasing order, the last key written to the builder is the largest. At Finish() time, the builder is still alive and the last key is deterministic. Alternatively, only track these fields when a flag (set in Open() or at construction time) indicates the caller intends to request PreparedFileInfo.

M2. Missing test coverage for UDT (user-defined timestamps) fast path -- db/external_sst_file_test.cc

• Issue: The fast path has dedicated logic for the "UDT in Memtables only" scenario (timestamp stripping in Finish(), timestamp padding in GetIngestedFileInfoFromFileInfo()), but no unit test exercises this path. If the padding logic produces keys that differ from the scan path, ingestion would silently produce incorrect file boundaries.
• Root cause: The existing UDT ingestion tests (e.g., TimestampedExternalSSTFileTest) do not use ingest_with_file_info=true.
• Suggested fix: Add a test that creates an SstFileWriter with persist_user_defined_timestamps=false (UDT-in-Memtable-only mode), writes keys, obtains PreparedFileInfo, ingests via file_infos, and verifies the data reads back correctly. Compare against the scan-path result.

M3. Missing test for range-del-only files on fast path -- db/external_sst_file_test.cc

• Issue: The stress test explicitly excludes range-del-only files from the fast path (!test_standalone_range_deletion), and there is no unit test covering this scenario. While the logic appears correct (MaybeUpdateRange will set boundaries from the range tombstone), this is an untested edge case.
• Root cause: The stress test exclusion suggests awareness of this gap but no corresponding unit test was added.
• Suggested fix: Either (a) add a unit test proving range-del-only files work on the fast path, or (b) add an explicit InvalidArgument guard rejecting range-del-only files with file_infos and document the limitation. Note: range-del-only files combined with UDT would hit an assertion in InternalKey::Encode() due to empty keys being padded -- this should be guarded.

M4. Missing test for multiple files in a single IngestExternalFiles call via file_infos -- db/external_sst_file_test.cc

• Issue: All unit tests ingest a single file via file_infos. The API supports multiple files per call, but multi-file ingestion on the fast path is only exercised by db_bench and potentially db_stress (which uses a single SST file per ingest).
• Root cause: Test gap.
• Suggested fix: Add a test that creates 2-3 SST files, obtains PreparedFileInfo for each, and ingests them all in a single IngestExternalFiles call.

🟢 LOW / NIT

L1. allow_db_generated_files + file_infos interaction -- db/external_sst_file_ingestion_job.cc:1095

• Issue: For allow_db_generated_files=true, the fast path uses table_properties.key_smallest_seqno / key_largest_seqno as seqno bounds. SstFileWriter always writes seqno=0, so these will always be 0. While this is technically correct (SstFileWriter files have seqno=0), allow_db_generated_files is designed for DB-generated files with non-zero seqnos. If someone manually constructs a PreparedFileInfo for a DB-generated file, the seqno bounds might be wrong.
• Suggested fix: Add a comment clarifying that file_infos is only supported for SstFileWriter-produced files, or add a validation check when allow_db_generated_files=true.

L2. TableProperties deep copy in Finish() -- table/sst_file_writer.cc:565

• Issue: prepared->table_properties = r->builder->GetTableProperties() performs a full deep copy of TableProperties including all string fields. This is a one-per-file cost so it's not on a hot path, but could be optimized with shared ownership.
• Suggested fix: Consider using std::shared_ptr<const TableProperties> if the builder supports it, or accept the copy as acceptable warm-path cost.

L3. IngestWithFileInfoRangeDeletion test could cover more scenarios -- db/external_sst_file_test.cc:419

• Issue: The range deletion test uses a single DeleteRange. Multiple non-contiguous range tombstones would exercise the boundary extension logic more thoroughly. While the logic is correct (MaybeUpdateRange takes min/max regardless of call count), a test with multiple tombstones would provide stronger confidence.
• Suggested fix: Add a variant with 2-3 non-contiguous range tombstones.

Cross-Component Analysis

Context	Executes?	Correct?	Notes
WritePreparedTxnDB	YES	YES	Seqno assignment in Run() unaffected
ReadOnly DB	NO	N/A	IngestExternalFile not available
allow_db_generated_files	YES	YES (with caveat L1)	SstFileWriter seqnos are always 0
User-defined timestamps	YES	YES	Padding logic verified correct
UDT in Memtables only	YES	YES (needs test M2)	Strip/pad logic consistent
write_global_seqno=true	NO	N/A	Correctly rejected in validation
Range-del-only files	YES	Likely correct but untested (M3)	MaybeUpdateRange handles it
Range-del-only + UDT	YES	BUG	Empty InternalKey::Encode() assertion
FIFO/Universal compaction	YES	YES	Affects Run(), not Prepare()
atomic_replace_range	YES	YES	Orthogonal to metadata acquisition
verify_checksums_before_ingest	YES	YES	Opens file only for verification

Key verified equivalences:

• MaybeUpdateRange takes min/max of boundaries regardless of call count, so single-synthetic-tombstone fast path equals per-tombstone scan path
• InternalKey::DecodeFrom() copies data (rep_.assign()), no lifetime concerns
• external_sst_file_global_seqno_offset is 0 in in-memory properties, safely handled since write_global_seqno is rejected
• GetTableProperties() returns by value, safe after Finish()
• Internal key encoding in SstFileWriter matches what TableReader returns

Positive Observations

• Clean separation of fast/scan paths with shared validation and checksum verification
• Good use of sync points for testing (GetIngestedFileInfo:ReadPath)
• Comprehensive db_stress integration with appropriate exclusions
• Well-designed benchmark in db_bench that isolates the optimization
• Thorough input validation (size mismatch, null pointers, write_global_seqno incompatibility)
• Ingestion timing histograms are a valuable addition independent of the fast path
• The code correctly handles the UDT timestamp lifecycle (strip in writer, pad in ingestion)

---

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

[github-actions]

Codex Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit e634e35

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not create PATH aliases: 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]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit e634e35

---

Summary

Well-designed performance optimization that adds a metadata fast-path to external SST file ingestion, cutting Prepare() latency by ~3x. The implementation is architecturally sound with clean separation between the fast-path (GetIngestedFileInfoFromFileInfo) and scan-path (GetIngestedFileInfoFromFile), proper validation, and good test coverage for the core scenarios. No high-severity correctness bugs found after thorough analysis.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. Missing test for range-deletion-only files via fast-path -- db/external_sst_file_test.cc

• Issue: No test covers a file that contains ONLY range deletions (no point keys) ingested via file_infos. This exercises a distinct code path in SstFileWriter::Finish() where smallest/largest come entirely from range deletion boundaries, and file_info.num_entries == 0.
• Suggested fix: Add a test that creates an SST with only DeleteRange() calls, finishes with PreparedFileInfo, and ingests via file_infos.

M2. Missing round-trip equivalence test -- db/external_sst_file_test.cc

• Issue: No test ingests the SAME file via both the fast-path and scan-path and compares the resulting IngestedFileInfo (or DB state) for equivalence. Such a test would be the strongest correctness guarantee that the two paths produce identical metadata.
• Suggested fix: Add a test that ingests identical files both ways and compares LiveFileMetaData (smallestkey, largestkey, size, etc.).

M3. Missing multi-file batch test via file_infos -- db/external_sst_file_test.cc

• Issue: All file_infos tests use a single file. No test covers multiple files in one IngestExternalFiles call via file_infos. The file_infos vector indexing logic in Prepare() should be validated with >1 file.
• Suggested fix: Add test with 2-3 files using file_infos, including overlapping keys to exercise files_overlap_ detection.

M4. SstFileWriter::Rep fields not reset in Open() -- table/sst_file_writer.cc

• Issue: The new fields smallest_internal_key, smallest_range_del_internal_key, and largest_range_del_internal_key are not explicitly reset in SstFileWriter::Open(). If a user reuses the same SstFileWriter instance across multiple files (Open() -> Finish() -> Open() -> Finish()), stale values from the first file could leak into the second. file_info IS reset (line 438), but the new InternalKey fields are not.
• Suggested fix: Reset these fields in Open() via default construction.

🟢 LOW / NIT

L1. PreparedFileInfo defined in internal header but exposed via public API -- include/rocksdb/options.h:2912

• Issue: Forward-declared in public options.h, defined in internal table/prepared_file_info.h. Opaque design is correct, but C API / Java JNI bindings cannot easily wrap this. Acceptable for initial implementation.

L2. TableProperties copy in Finish() -- table/sst_file_writer.cc:600

• Issue: Full TableProperties copy including string fields. Not on a hot path (once per file), so acceptable.

L3. Comment explaining max_ts for range deletion re-serialization -- table/sst_file_writer.cc:558-566

• Issue: When !strip_timestamp && ts_sz > 0, re-serialization with max_ts matches what NewRangeTombstoneIterator() returns. A brief comment explaining this would improve clarity.

Cross-Component Analysis

Context	Executes?	Safe?	Notes
WritePreparedTxnDB	YES	YES	Seqno reassigned in common path
UDT (persisted)	YES	YES	Boundaries carry timestamps
UDT (memtable only)	YES	YES	Padding via ExtractValueType is correct
FIFO compaction	YES	YES	Level assignment in common path
allow_db_generated_files	YES	YES	Handled explicitly
verify_checksums	YES	YES	Opens file in common path

Key claims verified: unique_id IS set (common path), start_ukey/limit_ukey IS set (common path), boundary computation IS equivalent (uses same SerializeKey/SerializeEndKey logic), write_global_seqno rejection IS enforced.

Positive Observations

• Clean dispatcher pattern with common post-processing
• Good validation (null pointers, size mismatches, write_global_seqno)
• Debug assertions verifying range-del timestamps
• Well-integrated stress test with configurable probability
• Test sync point verifying fast-path is taken

---

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

[github-actions]

Codex Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit 69a4b4e

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not create PATH aliases: 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]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 69a4b4e

---

Summary

Well-designed performance optimization that cleanly splits ingestion metadata acquisition into a fast-path (reuse SstFileWriter metadata) and scan-path. The boundary computation refactoring is thorough, with careful handling of UDT (both persisted and stripped), range deletions, and sequence number bounds. Good test coverage with unit tests, stress test integration, db_bench support, and crash test configuration.

High-severity findings (1):

• [sst_file_writer.cc:Finish] SstFileWriter::Finish() unconditionally computes PreparedFileInfo boundary data (InternalKey merging, range-del re-serialization, UDT timestamp handling) even when prepared_file_info is nullptr, adding overhead to all existing callers.

Full review (click to expand)

Findings

🔴 HIGH

H1. Unconditional boundary computation in Finish() penalizes existing callers -- sst_file_writer.cc:Finish

• Issue: The refactored Finish() always computes PreparedFileInfo boundary data -- InternalKey smallest/largest via update_file_boundaries, range deletion bound merging, UDT timestamp re-serialization -- even when prepared_file_info is nullptr. This adds computation (InternalKey comparisons, string allocations, RangeTombstone construction) to every SstFileWriter::Finish() call, including the 100+ existing call sites that do not use the feature.
• Root cause: The code unconditionally populates finished_prepared_file_info and associated temporaries before checking whether the caller wants it.
• Suggested fix: Gate the PreparedFileInfo-specific work behind if (prepared_file_info != nullptr). The ExternalSstFileInfo population (user key strings) can be done independently. Alternatively, accept the overhead if benchmarks show it's negligible for the Finish() cold path -- but document the decision.

🟡 MEDIUM

M1. Missing test for range-del-only file with file_infos -- external_sst_file_test.cc

• Issue: No test covers a file with only range deletions (zero point keys) ingested via the file_infos fast path. The IngestWithFileInfoRangeDeletion test includes a Put(Key(30)) alongside the DeleteRange. A range-del-only file exercises a different code path where smallest_internal_key comes entirely from range deletion bounds, and point-key tracking is skipped.
• Suggested fix: Add a test creating an SST with only DeleteRange calls, then ingest via file_infos.

M2. Missing test for allow_db_generated_files + file_infos (success path) -- external_sst_file_test.cc

• Issue: IngestWithFileInfoRejectsMissingDbGeneratedSeqnoBounds tests the rejection case. There is no positive test verifying correct ingestion with allow_db_generated_files=true and valid seqno bounds in PreparedFileInfo.
• Suggested fix: Add a test that creates a file with valid seqno bounds and successfully ingests it with allow_db_generated_files=true + file_infos.

M3. Missing test for multi-file batch with file_infos -- external_sst_file_test.cc

• Issue: All file_infos tests use a single file per ingestion call. The batch path (multiple files in IngestExternalFileArg::external_files) with matching file_infos is not tested. The index-based access file_infos[i] in the Prepare() loop should be verified with >1 file.
• Suggested fix: Test ingesting 2+ files in a single IngestExternalFiles call via file_infos.

M4. SstFileWriter::Rep::DeleteRangeImpl now uses InternalKey comparisons for boundary tracking -- sst_file_writer.cc:~240

• Issue: The old code compared user keys (internal_comparator.user_comparator()->Compare). The new code compares InternalKey objects with the full internal comparator (internal_comparator.Compare(range_del_start_bound, smallest_range_del_internal_key)). This is semantically correct and more precise, but it changes behavior: two range deletions with the same user key but different sequence numbers could produce different boundary decisions. For SstFileWriter (where all seqnos are 0), this is a no-op difference, but the change is worth documenting.
• Suggested fix: Add a brief comment noting that InternalKey comparison is used (and why it's safe with seqno=0).

M5. global_seqno_offset validation change is correct but subtle -- external_sst_file_ingestion_job.cc:~993

• Issue: The old code unconditionally errored when external_sst_file_global_seqno_offset == 0 for V2 files. The new code only errors when write_global_seqno && offset == 0. This is necessary because in-memory table properties from builder->GetTableProperties() have offset=0 (it's computed only when reading the file back). The change is safe because: (1) write_global_seqno=true is rejected when file_infos is set (db_impl.cc validation), and (2) AssignGlobalSeqnoForIngestedFile also checks write_global_seqno && offset == 0 at line 1436.
• Suggested fix: Add a comment explaining why the condition was relaxed, referencing the in-memory properties limitation.

M6. ExternalSstFileInfo boundary population changed semantics in Finish() -- sst_file_writer.cc

• Issue: The old Rep::AddImpl tracked file_info.smallest_key and file_info.largest_key as user keys assigned on each entry. The new code sets smallest_internal_key from r->smallest_internal_key (the first point key's InternalKey) and ikey (the last point key's InternalKey), then extracts user keys for finished_file_info. The ordering check was also changed from Compare(user_key, file_info.largest_key) to Compare(user_key, ikey.user_key()). Since ikey is set via ikey.Set(user_key, 0, value_type) in AddImpl, ikey.user_key() returns the same user_key. The semantics are equivalent.
• Suggested fix: None needed -- verified equivalent.

🟢 LOW / NIT

L1. shared_ptr<const PreparedFileInfo> overhead

• Issue: PreparedFileInfo uses shared_ptr with atomic ref-counting. Since Finish() is a cold path (once per file) and the ownership pattern is simple (one producer, one or few consumers), unique_ptr could avoid the overhead. However, shared_ptr simplifies user code (no explicit move into the vector of raw pointers), and the PR correctly uses shared_ptr<const> for safe sharing.
• Suggested fix: Acceptable as-is; document the rationale.

L2. IngestExternalFile single-CF API doesn't expose file_infos

• Issue: Users must use the multi-CF IngestExternalFiles(vector<IngestExternalFileArg>) API to use file_infos. The convenience single-CF IngestExternalFile doesn't have a file_infos parameter.
• Suggested fix: Consider adding an overload in a follow-up PR.

L3. PreparedFileInfo struct is not truly opaque

• Issue: PreparedFileInfo is forward-declared in options.h as opaque but defined in table/prepared_file_info.h with public fields including InternalKey (an internal type). Users who include prepared_file_info.h can see and modify the struct. This is acceptable since users would need to intentionally include an internal header, and the const qualifier on the shared_ptr prevents modification through the normal API.
• Suggested fix: None -- current approach is pragmatic.

L4. Crash test values for ingest_external_file_use_file_info_one_in

• Issue: Values [0, 1, 2] in db_crashtest.py provide good coverage: 0=never, 1=always, 2=50%. This is a reasonable distribution for a new feature.
• Suggested fix: None needed.

Cross-Component Analysis

Context	Fast path handles correctly?	Verification
UDT persisted	YES	Range dels re-serialized with max_ts in Finish(); tested in TimestampsPersistedIngestWithFileInfoRangeDeletionBounds
UDT not persisted (strip)	YES	Timestamps stripped in Finish(), padded back in GetIngestedFileInfoFromFileInfo; debug assertion in scan path validates max_ts invariant for range dels; tested in TimestampsNotPersistedIngestWithFileInfoRangeDeletionBounds
Range deletions	YES	Boundary extension tracked via SerializeKey/SerializeEndKey; tested in IngestWithFileInfoRangeDeletion
allow_db_generated_files	YES	Seqno bounds from table properties; rejection tested
verify_checksums	YES	Opens file just for verification; tested in IngestWithFileInfoVerifiesChecksum
write_global_seqno	REJECTED	Validated in db_impl.cc; tested in IngestWithFileInfoRejectsWriteGlobalSeqno
Concurrent ingestions	YES	shared_ptr is safe; stress test exercises concurrent paths
File modification between Finish/Ingest	Same risk as existing API	Both paths read the file at ingestion time if checksums enabled

Positive Observations

• Clean separation of concerns: GetIngestedFileInfoFromFileInfo (fast) vs GetIngestedFileInfoFromFile (scan) with shared post-processing in GetIngestedFileInfo wrapper. The common wrapper handles checksum verification, FD assignment, and start_ukey/limit_ukey computation uniformly.
• Thorough input validation in db_impl.cc: null pointer check, size mismatch check, write_global_seqno incompatibility.
• Debug assertions in the scan path (new #ifndef NDEBUG block) cross-validate the assumption that range tombstone keys use max timestamp, providing a safety net against future regressions.
• Well-integrated stress testing with configurable probability via ingest_external_file_use_file_info_one_in.
• db_bench integration provides a reproducible benchmark with clear methodology (link files, isolate ingest path).
• The IngestWithFileInfo test uses sync points to verify the scan path is actually skipped (read_path_count check).
• Range deletion boundary tracking refactored to use InternalKey throughout SstFileWriter, eliminating a user-key/internal-key impedance mismatch.

---

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

[github-actions]

Codex Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit 818103b

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not create PATH aliases: 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]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 818103b

---

Summary

Well-motivated optimization that eliminates redundant SST file scanning during ingestion by reusing metadata from SstFileWriter::Finish(). The design is sound, the API is backward-compatible, and the ~50% latency reduction is architecturally consistent. A few correctness issues need attention before merge.

High-severity findings (2):

• [sst_file_writer.cc:Finish] SstFileWriter::Finish() always computes PreparedFileInfo (including TableProperties copy) even when file_info is nullptr, adding unnecessary overhead to all callers who don't use the fast path.
• [external_sst_file_ingestion_job.cc:~1336] Debug assertion in slow path claims range tombstone keys always have max timestamp, but this is only true for the user_defined_timestamps_persisted=false case; files with persisted timestamps and a non-max user timestamp would fire this assertion.

The full review with 2 HIGH, 5 MEDIUM, and 4 LOW findings is written to review-findings.md.

---

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

[meta-codesync]

This pull request has been merged in ad43a5f.

[github-actions]

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit b5fcb6f

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not create PATH aliases: 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]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit b5fcb6f

---

Summary

Well-designed optimization that eliminates redundant SST file scanning during ingestion by reusing metadata from SstFileWriter::Finish(). The refactoring is clean, the validation is thorough, and test coverage is solid. One medium-severity behavioral inconsistency was found in the UDT-not-persisted timestamp padding path for range deletion boundary keys between the fast and scan paths.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

No high-severity findings.

🟡 MEDIUM

M1. UDT-not-persisted: range deletion start key timestamp padding differs between fast and scan paths -- db/external_sst_file_ingestion_job.cc:1117-1125

• Issue: In GetIngestedFileInfoFromFileInfo, the pad_timestamp lambda uses PadInternalKeyWithMaxTimestamp for keys with kTypeRangeDeletion value type. This applies to range deletion START keys. However, in the scan path, the TableReader opened with user_defined_timestamps_persisted=false causes the block iterator to pad min timestamp onto range deletion start keys. MaybeUpdateRange then stores these min-timestamp-padded keys.

  Result: FileMetaData.smallest will have [user_key | max_ts | seqno+type] via the fast path but [user_key | min_ts | seqno+type] via the scan path for range-deletion-dominated files with UDT-not-persisted.

• Impact: Affects sort position of FileMetaData.smallest in the LSM. The start_ukey/limit_ukey fields (used for overlap checking) are identical in both paths, so level assignment is unaffected. The fast path behavior (max ts) is arguably more correct for range tombstones.

• Suggested fix: Document this as intentional, or make both paths consistent.

M2. file_infos not plumbed through use_separate_prepare_calls in db_stress -- db_stress_tool/no_batched_ops_stress.cc:2520-2535

• Issue: When use_prepare_commit && use_separate_prepare_calls, individual file_arg objects don't copy file_infos, so the fast path isn't exercised in this code path.

• Suggested fix: Copy file_infos to each file_arg.

M3. Fast path trusts caller-supplied file_size without verification -- db/external_sst_file_ingestion_job.cc:1106

• Issue: file_to_ingest->file_size = prepared_file_info.file_size is set without disk verification. If the file is modified between Finish() and ingestion, the FileDescriptor will have incorrect size.

• Suggested fix: When verify_checksums_before_ingest opens the file, compare actual vs prepared file size.

M4. Debug-only assertions for PreparedFileInfo validity -- db/external_sst_file_ingestion_job.cc:1387-1393

• Issue: Post-dispatch validity checks (file_size > 0, !unset()) are assert() only. A malformed PreparedFileInfo could produce silently incorrect metadata in release builds.

• Suggested fix: Promote critical assertions to runtime Status::Corruption checks.

🟢 LOW / NIT

L1. Missing test for file with only range deletions via fast path

• No test covers num_entries == 0, num_range_del_entries > 0 with the fast path.

L2. Missing test for multi-file batch ingestion with file_infos

• All unit tests ingest single files. No test verifies multiple files in one call.

L3. db_crashtest.py has unrelated formatting changes

• Several reformatted lines appear to be auto-formatter artifacts.

L4. DeleteRangeImpl creates two InternalKey objects per call instead of one pair

• Minor performance difference; negligible since range deletions are few per file.

Cross-Component Analysis

Context	Affected?	Analysis
UDT persisted	Yes, tested	Range deletion bounds correctly use max timestamp
UDT not persisted	Yes, tested	M1: minor internal key timestamp difference
allow_db_generated_files	Partial	Seqno bounds from table_properties work correctly
write_global_seqno	Rejected	Correctly validated in PrepareFileIngestion
Concurrent ingestion	Safe	PreparedFileInfo is const-qualified

Positive Observations

• Clean refactoring of GetIngestedFileInfo into two paths with shared dispatcher
• SanityCheckTableProperties simplification improves testability
• Comprehensive test coverage including UDT, checksums, and error cases
• Good stress test and db_bench integration
• Useful debug assertion for range deletion timestamp consistency in the scan path

---

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