Skip to content

Commit

Permalink
Propagate more ReadOptions to ApproximateOffsetOf/Sizes
Browse files Browse the repository at this point in the history
Summary: Unknown why these would ignore options like deadline and
read_tier. Setting total_order_seek=true is unnecessary because of the
disable_prefix_seek (= true) parameter to NewIndexIterator. This is only
used by the hash index, which uses total order seek if either the
ReadOption or the parameter is true. The parameter is arguably redundant
with the total_order_seek option, meaning it could be eliminated, but
I think this case is exceptional (compared to e.g. no_io):
* Prefix seek is particular to user iterators, though might be usable,
  carefully, for other read operations.
* The historical default of total_order_seek=false in a sense is "wrong
  result by default" so cannot be interpreted as an intent to force prefix
  seek in an operation for which it might be usual or give bad results.

Also added a generic release note to cover this and related PRs.

Test Plan: existing tests
  • Loading branch information
pdillinger committed Jun 12, 2024
1 parent d64eac2 commit d7e6b54
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 8 deletions.
12 changes: 4 additions & 8 deletions table/block_based/block_based_table_reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,8 @@ Status BlockBasedTable::Open(

// From read_options, retain deadline, io_timeout, rate_limiter_priority, and
// verify_checksums. In future, we may retain more options.
// TODO: audit more ReadOptions and do this in a way that brings attention
// on new ReadOptions?
ReadOptions ro;
ro.deadline = read_options.deadline;
ro.io_timeout = read_options.io_timeout;
Expand Down Expand Up @@ -2858,11 +2860,8 @@ uint64_t BlockBasedTable::ApproximateOffsetOf(const ReadOptions& read_options,

BlockCacheLookupContext context(caller);
IndexBlockIter iiter_on_stack;
ReadOptions ro;
ro.total_order_seek = true;
ro.io_activity = read_options.io_activity;
auto index_iter =
NewIndexIterator(ro, /*disable_prefix_seek=*/true,
NewIndexIterator(read_options, /*disable_prefix_seek=*/true,
/*input_iter=*/&iiter_on_stack, /*get_context=*/nullptr,
/*lookup_context=*/&context);
std::unique_ptr<InternalIteratorBase<IndexValue>> iiter_unique_ptr;
Expand Down Expand Up @@ -2905,11 +2904,8 @@ uint64_t BlockBasedTable::ApproximateSize(const ReadOptions& read_options,

BlockCacheLookupContext context(caller);
IndexBlockIter iiter_on_stack;
ReadOptions ro;
ro.total_order_seek = true;
ro.io_activity = read_options.io_activity;
auto index_iter =
NewIndexIterator(ro, /*disable_prefix_seek=*/true,
NewIndexIterator(read_options, /*disable_prefix_seek=*/true,
/*input_iter=*/&iiter_on_stack, /*get_context=*/nullptr,
/*lookup_context=*/&context);
std::unique_ptr<InternalIteratorBase<IndexValue>> iiter_unique_ptr;
Expand Down
1 change: 1 addition & 0 deletions unreleased_history/bug_fixes/read_options.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* Various read operations could ignore various ReadOptions that might be relevant. Fixed many such cases, which can result in behavior change but a better reflection of specified options.

0 comments on commit d7e6b54

Please sign in to comment.