CNDB-15300: Add SSTableReader#getApproximatePositionsForRanges
#1993
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Checklist before you submit for review
|
pcmanus
force-pushed
the
CNDB-15300-approx-get-position
branch
2 times, most recently
from
7429e16 to
33e456d
Compare
|
@blambov: I'd appreciate a look, at least at the first commit, as it involves trie stuff. This isn't very complex, but would like to make sure I didn't made an incorrect assumption in there (and/or missed a better way to do this). |
blambov
reviewed
Sep 17, 2025
src/java/org/apache/cassandra/io/sstable/format/trieindex/TrieIndexSSTableReader.java
Outdated
Show resolved
Hide resolved
src/java/org/apache/cassandra/io/sstable/format/trieindex/TrieIndexSSTableReader.java
Outdated
Show resolved
Hide resolved
pcmanus
force-pushed
the
CNDB-15300-approx-get-position
branch
from
33e456d to
272748a
Compare
jasonstack
reviewed
Sep 19, 2025
There was a problem hiding this comment.
The changes look good, but I am not familiar with trie code. I will leave it to @blambov
blambov
approved these changes
Sep 19, 2025
src/java/org/apache/cassandra/io/sstable/format/trieindex/TrieIndexSSTableReader.java
Outdated
Show resolved
Hide resolved
src/java/org/apache/cassandra/io/sstable/format/trieindex/TrieIndexSSTableReader.java
Outdated
Show resolved
Hide resolved
pcmanus
force-pushed
the
CNDB-15300-approx-get-position
branch
from
272748a to
647368c
Compare
|
Note: cove coverage is not happy, and this seem to be mostly because I copied the |
In the context of CNDB-15300, adds a variant of the `SSTableReader#getPositionsForRanges` method that never read the data file to return its results, but in exchange may return positions that slightly "overshoot" the requested range. Put another way, the added method `SSTableReader#getApproximatePositionsForRanges` is such that if you call it on some range `R`, and you read the data within the returned positions, then the read data may start by one (at most) key (partition really) that sorts strictly before `R`, and may end by one (at most) key that stort strictly after `R`.
The STATS component deserialization method, `MetadataSerializer#deserialize` was creating a `RandomAccessReader` (RAR) to read the underlying file. But the deserialization does not do "random" accesses, it strictly deserialize sequentially. In principle, using a RAR to do sequential reads is fine (though slighly overkill), but it does mean that the method used on the underlying `FileChannel` will be an "absolute" read (that take its position as argument, instead of reading at the channel position), and tiered-storage extensions with custom file channel may be able to use simple/more optimal implementations when then know the file is read sequentially (only through "relative" read calls). Tl;dr, this replace the use of RAR by `FileInputStreamPlus`, which is essentially equivalent in this use case, but does only do relative reads.
Cove coverage complains it isn't tested, it is a bit involved to actually test, and we actually don't need (at least not yet) this method in those cases. In other words, this was kind of dead code, so removing with assertions to prevent future misuse.
pcmanus
force-pushed
the
CNDB-15300-approx-get-position
branch
from
6135469 to
1cee610
Compare
|
❌ Build ds-cassandra-pr-gate/PR-1993 rejected by Butler2 regressions found Found 2 new test failures
Found 2 known test failures |
michaelsembwever
pushed a commit
that referenced
this pull request
Sep 26, 2025
…Ranges` (#1993) This PR is in the context of riptano/cndb#15380, and is used by its PR riptano/cndb#15380. It adds a variant of the `SSTableReader#getPositionsForRanges` method that never read the data file to return its results, but in exchange may return positions that slightly "overshoot" the requested range. Put another way, the added method `SSTableReader#getApproximatePositionsForRanges` is such that if you call it on some range `R`, and you read the data within the returned positions, then the read data may start by one (at most) key (partition really) that sorts strictly before `R`, and may end by one (at most) key that sorts strictly after `R`. Additionally, the PR switches the reading of the `Statistics.db` component from using `RandomAccessReader` to using `FileInputStreamPlus`. This is essentially equivalent functionality wise (since the component is deserialized sequentially anyway, there is no random reads), but by making it more "clear" that it doesn't do random reads, it allows us to "direct download" this component like other related components on the CNDB side. See the last point of riptano/cndb#15380 for more details.
michaelsembwever
pushed a commit
that referenced
this pull request
Oct 14, 2025
…Ranges` (#1993) This PR is in the context of riptano/cndb#15380, and is used by its PR riptano/cndb#15380. It adds a variant of the `SSTableReader#getPositionsForRanges` method that never read the data file to return its results, but in exchange may return positions that slightly "overshoot" the requested range. Put another way, the added method `SSTableReader#getApproximatePositionsForRanges` is such that if you call it on some range `R`, and you read the data within the returned positions, then the read data may start by one (at most) key (partition really) that sorts strictly before `R`, and may end by one (at most) key that sorts strictly after `R`. Additionally, the PR switches the reading of the `Statistics.db` component from using `RandomAccessReader` to using `FileInputStreamPlus`. This is essentially equivalent functionality wise (since the component is deserialized sequentially anyway, there is no random reads), but by making it more "clear" that it doesn't do random reads, it allows us to "direct download" this component like other related components on the CNDB side. See the last point of riptano/cndb#15380 for more details.
michaelsembwever
pushed a commit
that referenced
this pull request
Oct 14, 2025
…Ranges` (#1993) This PR is in the context of riptano/cndb#15380, and is used by its PR riptano/cndb#15380. It adds a variant of the `SSTableReader#getPositionsForRanges` method that never read the data file to return its results, but in exchange may return positions that slightly "overshoot" the requested range. Put another way, the added method `SSTableReader#getApproximatePositionsForRanges` is such that if you call it on some range `R`, and you read the data within the returned positions, then the read data may start by one (at most) key (partition really) that sorts strictly before `R`, and may end by one (at most) key that sorts strictly after `R`. Additionally, the PR switches the reading of the `Statistics.db` component from using `RandomAccessReader` to using `FileInputStreamPlus`. This is essentially equivalent functionality wise (since the component is deserialized sequentially anyway, there is no random reads), but by making it more "clear" that it doesn't do random reads, it allows us to "direct download" this component like other related components on the CNDB side. See the last point of riptano/cndb#15380 for more details.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is in the context of https://github.com/riptano/cndb/pull/15380, and is used by its PR https://github.com/riptano/cndb/pull/15380.
It adds a variant of the
SSTableReader#getPositionsForRangesmethod that never read the data file to return its results, but in exchange may return positions that slightly "overshoot" the requested range.Put another way, the added method
SSTableReader#getApproximatePositionsForRangesis such that if you call it on some rangeR, and you read the data within the returned positions, then the read data may start by one (at most) key (partition really) that sorts strictly beforeR, and may end by one (at most) key that sorts strictly afterR.Additionally, the PR switches the reading of the
Statistics.dbcomponent from usingRandomAccessReaderto usingFileInputStreamPlus. This is essentially equivalent functionality wise (since the component is deserialized sequentially anyway, there is no random reads), but by making it more "clear" that it doesn't do random reads, it allows us to "direct download" this component like other related components on the CNDB side. See the last point of https://github.com/riptano/cndb/pull/15380 for more details.