ExternalTable support FS

[joshkang97]

Summary

Extends the ExternalTable plug-in interface so external table implementations have access to the same FileSystem RocksDB itself uses. Previously only ExternalTableOptions (read path) had fs.

Test Plan

• Existing tests pass

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 401.1s.

[meta-codesync]

@joshkang97 has imported this pull request. If you are a Meta employee, you can view this in D101384103.

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit b21901b

---

Code Review: ExternalTable support FS

PR: ExternalTable support FS | Author: joshkang97 | Changes: 3 files, +8/-4

Verdict: APPROVE with minor suggestions

The change is small, correct, follows established patterns, and is consistent with the existing API design. No bugs found.

---

Findings

[Medium] Missing Test Coverage for fs Field Propagation

File: table/table_test.cc:7240-7244

The test passes nullptr for fs and the test factory ignores the options entirely (const ExternalTableBuilderOptions& /*opts*/). No test verifies the fs value is correctly propagated to the external builder implementation.

Suggestion: Consider adding a test where the factory asserts options.fs is non-null and matches the expected FileSystem, validating end-to-end propagation through the adapter.

[Low] Constructor Parameter Ordering Inconsistency

File: include/rocksdb/external_table.h

In ExternalTableOptions, fs is the 3rd parameter. In ExternalTableBuilderOptions, fs is last (7th). Minor inconsistency between sibling structs.

[Suggestion] Pre-existing Design Note: Reference-to-shared_ptr Members

File: include/rocksdb/external_table.h:229-250

The struct stores const std::shared_ptr<FileSystem>& fs (reference, not copy). This follows the pre-existing pattern for all reference members in this struct. It is safe in current usage because:

1. In the adapter, the struct is stack-allocated and consumed immediately — topts.ioptions.fs outlives the call.
2. In the test, all temporaries survive until end of the full expression.

This pattern is fragile if an implementation ever stores the struct beyond the NewTableBuilder call, but that's a pre-existing design choice, not a bug introduced by this PR.

What the PR Gets Right

• Consistency with ExternalTableOptions (read-path) — achieves read/write symmetry
• Minimal scope — only necessary files touched
• Correct source — topts.ioptions.fs is the standard FileSystem access pattern
• EXPERIMENTAL API — breaking constructor change is acceptable
• Correct initialization order — fs declared and initialized after reason

---

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

[anand1976]

LGTM

[meta-codesync]

@joshkang97 has imported this pull request. If you are a Meta employee, you can view this in D101384103.

[meta-codesync]

@joshkang97 has imported this pull request. If you are a Meta employee, you can view this in D101384103.

[meta-codesync]

@joshkang97 merged this pull request in a12bd7d.